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 flyway variables support via placeholder templater #4026

Merged
merged 9 commits into from Nov 13, 2022
2 changes: 1 addition & 1 deletion src/sqlfluff/core/config.py
Expand Up @@ -302,7 +302,7 @@ def _get_config_elems_from_file(cls, fpath: str) -> List[ConfigElemType]:
# Disable interpolation so we can load macros
kw: Dict = {}
kw["interpolation"] = None
config = configparser.ConfigParser(**kw)
config = configparser.ConfigParser(delimiters="=", **kw)
Copy link
Member

Choose a reason for hiding this comment

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

Why was this added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per the configparser doc, "The first occurrence of a delimiting substring on a line is considered a delimiter" and it assumes both ':' and '=' to be delimiters by default. This would mean that the defining a key called "flyway:database = test_db_name" would split key="flyway" and val="database=test_db_name" :(
I see that all our keys use '=' as delimiter and ':' is only used in section names hence made this change. But, If there's a better way to not impose this globally, pls do let me know

Copy link
Member

Choose a reason for hiding this comment

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

It appears the current tests don't test for this change. See my comment on test/fixtures/templater/placeholder_flyway_var/placeholder_flyway_var_a.sql for a suggestion on how to test it.

Copy link
Member

Choose a reason for hiding this comment

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

You mentioned adding another test. Will any of your tests fail if this delimiters change is removed? I think it's important to have a test for this.

# NB: We want to be case sensitive in how we read from files,
# because jinja is also case sensitive. To do this we override
# the optionxform attribute.
Expand Down
2 changes: 2 additions & 0 deletions src/sqlfluff/core/templaters/placeholder.py
Expand Up @@ -35,6 +35,8 @@
"dollar": regex.compile(
r"(?<![:\w\x5c])\${?(?P<param_name>[\w_]+)}?", regex.UNICODE
),
# e.g. USE ${flyway:database}.schema_name;
"flyway_var": regex.compile(r"\${(?P<param_name>\w+[:\w_]+)}", regex.UNICODE),
# e.g. WHERE bla = ?
"question_mark": regex.compile(r"(?<![:\w\x5c])\?", regex.UNICODE),
# e.g. WHERE bla = $3 or WHERE bla = ${3}
Expand Down
22 changes: 22 additions & 0 deletions test/core/config_test.py
Expand Up @@ -137,6 +137,28 @@ def test__config__load_toml():
}


def test__config__load_placeholder_cfg():
"""Test loading a sqlfluff configuration file for placeholder templater."""
c = ConfigLoader()
cfg = c.load_config_file(
os.path.join("test", "fixtures", "config", "placeholder"),
".sqlfluff-placeholder",
)
assert cfg == {
"core": {
"testing_val": "foobar",
"testing_int": 4,
},
"bar": {"foo": "barbar"},
"templater": {
"placeholder": {
"param_style": "flyway_var",
"flyway:database": "test_db",
}
},
}


def test__config__iter_config_paths_right_order():
"""Test that config paths are fetched ordered by priority."""
c = ConfigLoader()
Expand Down
18 changes: 18 additions & 0 deletions test/core/templaters/placeholder_test.py
Expand Up @@ -289,6 +289,22 @@ def test__templater_raw():
start_date="'2021-10-01'",
),
),
(
"USE ${flyway:database}.test_schema;",
"flyway_var",
"USE test_db.test_schema;",
{
"flyway:database": "test_db",
},
),
(
"SELECT metadata$filename, $1 FROM @stg_data_export_${env_name};",
"flyway_var",
"SELECT metadata$filename, $1 FROM @stg_data_export_staging;",
{
"env_name": "staging",
},
),
],
ids=[
"no_changes",
Expand All @@ -306,6 +322,8 @@ def test__templater_raw():
"numeric_dollar_with_braces_and_string",
"percent",
"ampersand",
"flyway_var",
"flyway_var",
Copy link
Member

Choose a reason for hiding this comment

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

Why does flyway_var appear twice?

Copy link
Contributor Author

@srjonemed srjonemed Nov 13, 2022

Choose a reason for hiding this comment

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

I saw some warning to the effect that ids list has less elements than the test sql's list
____________________________________________ ERROR collecting test/core/templaters/placeholder_test.py ____________________________________________ In test__templater_param_style: 17 parameter sets specified, with different number of ids: 16

I can remove and retest if above can be ignored

],
)
def test__templater_param_style(instr, expected_outstr, param_style, values):
Expand Down
10 changes: 10 additions & 0 deletions test/fixtures/config/placeholder/.sqlfluff-placeholder
@@ -0,0 +1,10 @@
[sqlfluff]
testing_val=foobar
testing_int=4

[sqlfluff:bar]
foo=barbar

[sqlfluff:templater:placeholder]
param_style = flyway_var
flyway:database = test_db
@@ -0,0 +1,5 @@
USE ${flyway:database}.test_schema;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any mention of flyway in the CI output. Perhaps you need to change something else to make this run? The placeholder templater tests are in this file: test/core/templaters/placeholder_test.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx, will take a look

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated with a test, pls take a look

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curious, could u let me know if this fixture is needed - not sure where/how it'd be used

Copy link
Member

Choose a reason for hiding this comment

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

It's currently not used because the placeholder templater doesn't have a test that reads from file fixtures. See this jinja test: https://github.com/sqlfluff/sqlfluff/blob/main/test/core/templaters/jinja_test.py#L568L624

Can you look at adding a similar test for the placeholder templater? This would test the config delimiter change you made in a realistic way.

Copy link
Member

Choose a reason for hiding this comment

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

BTW, sorry it took me a while to respond. I had actually written the comment above several days ago, but I forgot to submit the pending comments. Occasionally GitHub confuses me. 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the pointers, I'll have take a look soon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a test to config_test.py that should hopefully cover the delimiters codecov


CREATE OR REPLACE STAGE stg_data_export_${env_name}
URL = 's3://${s3_data_lake_bucket}/${env_name}/exports/stg_data_export'
STORAGE_INTEGRATION = s3_integ_main;