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.

Sign up for GitHub

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

feat: Improved variable handling for config #332

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

feat: Improved variable handling for config #332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions ariadne_codegen/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import enum
import os
import re
from dataclasses import dataclass, field
from keyword import iskeyword
from pathlib import Path
Expand Down Expand Up @@ -50,6 +51,7 @@ def __post_init__(self):
assert_path_exists(self.schema_path)

self.remote_schema_headers = resolve_headers(self.remote_schema_headers)
self.remote_schema_url = resolve_schema(self.remote_schema_url)


@dataclass
Expand Down Expand Up @@ -276,20 +278,29 @@ def assert_string_is_valid_python_identifier(name: str):
def resolve_headers(headers: Dict) -> Dict:
return {key: get_header_value(value) for key, value in headers.items()}

def resolve_schema(value: str) -> str:
return _replace_env_vars(value)


def get_header_value(value: str) -> str:
env_var_prefix = "$"
if value.startswith(env_var_prefix):
env_var_name = value.lstrip(env_var_prefix)
return _replace_env_vars(value)


def _replace_env_vars(value: str) -> str:
pattern = re.compile(r"\${?([\w_]+)}?")
Copy link

Choose a reason for hiding this comment

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

I didn't add support for ${} because I think it's confusing to have it without proper matching. Try $TEST_VAR} or ${TEST_VAR. Also consider variables like $1WOOT. I think of simple expressions, mine (\$([A-z][A-z0-9_]*)) is a bit more optimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Environment variables tend to come from a shell such as bash or zsh where ${VAR} is a common usage and even required if you want a variable like ${foo}bar which wouldn't work with your suggestion.

I didn't do any complex matching to ease maintenance, using something like $TEST_VAR} would just break your own code and I don't know why one would do that. I guess worst case we hide a configuration issue since it would successfully replace it if $TEST_VAR is defined.

If we want to actually check for this I think just adding two options would be easiest, something like, \${([\w_]+)}|\$([\w_]+) would require either ${VAR} or $VAR.

Also consider variables like $1WOOT.

What should be considered you mean? I know it's not a valid variable in bash but do you mean if someone want's to use the literal we shouldn't replace it? Seems unlikely to me but can be fixed with something like \${([A-z][A-z0-9_]*)}|\$([A-z][A-z0-9_]*). It's a trade-off I guess that I thought wasn't worth it but I'm open to change it if the owners agree.

Previous owners didn't even want this feature in so we'll see if this will even be considered 😺

Copy link

Choose a reason for hiding this comment

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

Seems unlikely to me but can be fixed with something like \${([A-z][A-z0-9_]*)}|\$([A-z][A-z0-9_]*). It's a trade-off I guess that I thought wasn't worth it but I'm open to change it if the owners agree.

Yes, this type of regex is what I was alluding to with both points. I'm afraid you can't use multiple match groups with your approach. See #349 for a suggestion if proper ${} support is desired.

Previous owners didn't even want this feature in so we'll see if this will even be considered 😺

As I said, I don't think it's a good idea to stick with a weird, non-standard notion of variables that confuses users and doesn't save on parsing complexity.

If they stick to it, it would at least be worth documenting to avoid head-banging.

/cc @mat-sop


def replacer(match):
env_var_name = match.group(1)
var_value = os.environ.get(env_var_name)

if not var_value:
raise InvalidConfiguration(
f"Environment variable {env_var_name} not found."
)
return var_value

return value
return var_value

return pattern.sub(replacer, value)

def assert_class_is_defined_in_file(file_path: Path, class_name: str):
file_content = file_path.read_text()
Expand Down
54 changes: 51 additions & 3 deletions tests/test_settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,24 @@ def test_client_settings_without_schema_path_or_remote_schema_url_raises_excepti
ClientSettings(queries_path=queries_path)


@pytest.mark.parametrize(
"configured_header, expected_header",
[
("$TEST_VAR", "test_value"),
("Bearer: $TEST_VAR", "Bearer: test_value"),
("Bearer: ${TEST_VAR}", "Bearer: test_value"),
pytest.param(
"$NOT_SET_VAR",
"",
marks=pytest.mark.xfail(raises=InvalidConfiguration),
),
],
)
def test_client_settings_resolves_env_variable_for_remote_schema_header_with_prefix(
tmp_path, mocker
tmp_path,
mocker,
configured_header,
expected_header,
):
queries_path = tmp_path / "queries.graphql"
queries_path.touch()
Expand All @@ -143,10 +159,42 @@ def test_client_settings_resolves_env_variable_for_remote_schema_header_with_pre
settings = ClientSettings(
queries_path=queries_path,
remote_schema_url="https://test",
remote_schema_headers={"Authorization": "$TEST_VAR"},
remote_schema_headers={"Authorization": configured_header},
)

assert settings.remote_schema_headers["Authorization"] == expected_header


@pytest.mark.parametrize(
"configured_url, expected_url",
[
("$TEST_VAR", "test_value"),
("https://${TEST_VAR}/graphql", "https://test_value/graphql"),
("https://$TEST_VAR/graphql", "https://test_value/graphql"),
("https://TEST_VAR/graphql", "https://TEST_VAR/graphql"),
pytest.param(
"https://${NOT_SET_VAR}/graphql",
"",
marks=pytest.mark.xfail(raises=InvalidConfiguration),
),
],
)
def test_client_settings_resolves_env_variable_for_remote_schema(
tmp_path,
mocker,
configured_url,
expected_url,
):
queries_path = tmp_path / "queries.graphql"
queries_path.touch()
mocker.patch.dict(os.environ, {"TEST_VAR": "test_value"})

settings = ClientSettings(
queries_path=queries_path,
remote_schema_url=configured_url,
)

assert settings.remote_schema_headers["Authorization"] == "test_value"
assert settings.remote_schema_url == expected_url


def test_client_settings_doesnt_resolve_remote_schema_header_without_prefix(tmp_path):
Expand Down
Loading