Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for embedded variables in headers #349

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zyv
Copy link

@zyv zyv commented Feb 11, 2025

Hey,

Thanks for this awesome generator!

Unfortunately, I can't use it to fetch GitHub's own schemas because they require Authorization headers in the form of Bearer token. Currently, the generator only supports variable expansion if it's the only element in the header value. However, I can't hardcode the token in my pyproject.toml to work around this limitation.

So I added support for embedded and multiple variables (not that I need multiple variables, but you basically get that for free with my approach, so why not...). There may be situations where it won't work like shell, but I would prefer simplicity over handling all possible corner cases.

Hope this helps you and others!

@bombsimon
Copy link
Contributor

This looks like a duplicate of #332

Did you consider storing Bearer $YOUR_TOKEN in a variable instead? See #231 (comment)

@zyv
Copy link
Author

zyv commented Feb 11, 2025

This looks like a duplicate of #332

Sorry about that! I should have searched for other PRs before opening mine.

As far as I'm concerned, it doesn't really matter which PR gets merged. I'll take a look at your code.

Did you consider storing Bearer $YOUR_TOKEN in a variable instead? See #231 (comment)

Yes, I did - but it's an ugly and clumsy hack. All tools (GitHub CLI, SDKs, etc.) standardize on GITHUB_TOKEN, and if I set GITHUB_TOKEN to Bearer ... it will break them. I can only set a second variable specifically for Ariadne like export ARIADNE_GITHUB_TOKEN="Bearer $GITHUB_TOKEN".

But what's worse is the very annoying and confusing default behavior. I've never seen a tool that claims to support "variables", but only resolves a variable if it's the only content of the string. This is not documented and extremely counterintuitive.

In fact, it's so confusing that I didn't even know what to search for and ended up debugging the code to figure out why I was getting 401 or 403 no matter what I tried. At that point, implementing proper variable support, including tests, only took a quarter of an hour, and I didn't bother looking for issues.

So if you decide to reject the PR, please at least document the weird notion of the variable directly where variables are mentioned, to save others searching and re-implementing the same.

Comment on lines +283 to 284
for env_var_name in re.findall(r"\$([A-z][A-z0-9_]*)", value):
var_value = os.environ.get(env_var_name)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for env_var_name in re.findall(r"\$([A-z][A-z0-9_]*)", value):
var_value = os.environ.get(env_var_name)
for env_var_name in re.findall(r"\$([A-z][A-z0-9_]*|{[A-z][A-z0-9_]*})", value):
var_value = os.environ.get(env_var_name.removeprefix("{").removesuffix("}"))

... to support ${VARIABLES} if desired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants