Repository navigation
fix: keep numeric/boolean owner and tag strings instead of crashing or dropping them #2376
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
Open
deepanshupal
wants to merge
7
commits into
elementary-data:master
Choose a base branch
from
deepanshupal:DeepanshuPal-patch-1
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
43a3a85
fix: keep numeric/boolean owner and tag strings instead of crashing
deepanshupal 1dce664
fix: keep numeric/boolean scalar tokens when unpacking owner and tag …
deepanshupal 677295e
test: cover numeric/boolean owner and tag strings
deepanshupal 2c93241
Merge branch 'master' into DeepanshuPal-patch-1
NoyaArie afdb10d
fix: normalize quoted scalars and stringify alert list tokens
deepanshupal b1c988b
fix: share scalar normalization with report parser
deepanshupal 7511906
test: cover reviewer scalar list and caller compatibility cases
deepanshupal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,11 @@ def try_load_json(value: Optional[Union[str, dict, list]]): | |
| return None | ||
|
|
||
|
|
||
| def normalize_scalar_token(raw: str, decoded: Any) -> str: | ||
| """Use decoded JSON strings; keep other scalar tokens as written.""" | ||
| return (decoded if isinstance(decoded, str) else raw).strip() | ||
|
|
||
|
|
||
| def unpack_and_flatten_str_to_list(list_as_str: str) -> List[str]: | ||
| """ | ||
| if given a simple token like "marketing" -> return ["marketing"] | ||
|
|
@@ -33,7 +38,10 @@ def unpack_and_flatten_str_to_list(list_as_str: str) -> List[str]: | |
|
|
||
| if isinstance(list_unpacked, list): | ||
| return list_unpacked | ||
| return [] # edge case of a string of an empty dict or IDK | ||
| if isinstance(list_unpacked, dict): | ||
| return [] # edge case of a string of an empty dict or IDK | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This drops a dict to |
||
| # A JSON scalar such as 12345 or true is just a plain token. | ||
| return [normalize_scalar_token(list_as_str, list_unpacked)] | ||
|
|
||
|
|
||
| def sum_lists(list_of_lists: List[List]) -> List: | ||
|
|
@@ -44,7 +52,7 @@ def sum_lists(list_of_lists: List[List]) -> List: | |
|
|
||
|
|
||
| def unpack_and_flatten_and_dedup_list_of_strings( | ||
| list_maybe_jsoned: Optional[Union[List[str], str]] | ||
| list_maybe_jsoned: Optional[Union[List[str], str]], | ||
| ) -> List[str]: | ||
| if not list_maybe_jsoned: | ||
| return [] | ||
|
|
@@ -54,11 +62,16 @@ def unpack_and_flatten_and_dedup_list_of_strings( | |
| elif isinstance(list_maybe_jsoned, list): | ||
| ret = sum_lists( | ||
| [ | ||
| unpack_and_flatten_str_to_list(x) | ||
| unpack_and_flatten_str_to_list(x) if isinstance(x, str) else [x] | ||
| for x in list_maybe_jsoned | ||
| if isinstance(x, str) | ||
| ] | ||
| ) | ||
| # Owners/tags need strings, but the general parser also handles sample rows. | ||
| ret = [ | ||
| item.strip() if isinstance(item, str) else json.dumps(item) | ||
| for item in ret | ||
| if isinstance(item, (str, int, float, bool)) | ||
| ] | ||
| return list(set(ret)) | ||
|
|
||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| import pytest | ||
|
|
||
| from elementary.utils.json_utils import ( | ||
| unpack_and_flatten_and_dedup_list_of_strings, | ||
| unpack_and_flatten_str_to_list, | ||
| ) | ||
| from elementary.utils.schema import ExtendedBaseModel | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("token", ["12345", "1.5", "true", "false"]) | ||
| def test_unpack_and_flatten_keeps_json_scalar_tokens(token): | ||
| assert unpack_and_flatten_str_to_list(token) == [token] | ||
| assert unpack_and_flatten_and_dedup_list_of_strings(token) == [token] | ||
|
|
||
|
|
||
| def test_unpack_and_flatten_still_handles_lists_and_commas(): | ||
| assert unpack_and_flatten_str_to_list('["a", "b"]') == ["a", "b"] | ||
| assert unpack_and_flatten_str_to_list("a, b") == ["a", "b"] | ||
| assert unpack_and_flatten_str_to_list("{}") == [] | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("token", ["12345", "1.5", "true"]) | ||
| def test_load_var_to_list_keeps_json_scalar_tokens(token): | ||
| assert ExtendedBaseModel._load_var_to_list(token) == [token] | ||
|
|
||
|
|
||
| def test_load_var_to_list_still_handles_lists_and_strings(): | ||
| assert ExtendedBaseModel._load_var_to_list('["a"]') == ["a"] | ||
| assert ExtendedBaseModel._load_var_to_list("prod") == ["prod"] | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "token, expected", | ||
| [ | ||
| ('"@alice"', ["@alice"]), | ||
| ('"prod"', ["prod"]), | ||
| ('" prod "', ["prod"]), | ||
| (" prod ", ["prod"]), | ||
| (" 12345 ", ["12345"]), | ||
| (" true ", ["true"]), | ||
| ], | ||
| ) | ||
| def test_scalar_tokens_match_in_report_and_alerts(token, expected): | ||
| assert ExtendedBaseModel._load_var_to_list(token) == expected | ||
| assert unpack_and_flatten_and_dedup_list_of_strings(token) == expected | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("value", ["[12345]", '[1, "a"]', [1, "a"]]) | ||
| def test_alert_numeric_list_items_are_strings(value): | ||
| from elementary.utils.strings import prettify_and_dedup_list | ||
|
|
||
| result = unpack_and_flatten_and_dedup_list_of_strings(value) | ||
| expected = ["12345"] if value == "[12345]" else ["1", "a"] | ||
| assert sorted(result) == expected | ||
| assert prettify_and_dedup_list(result) == ", ".join(expected) | ||
|
|
||
|
|
||
| def test_general_list_parser_preserves_sample_rows(): | ||
| rows = [{"a": 1}, {"b": True}] | ||
| assert unpack_and_flatten_str_to_list('[{"a": 1}, {"b": true}]') == rows | ||
| assert ExtendedBaseModel._load_var_to_list(rows) is rows | ||
|
|
||
|
|
||
| def test_existing_dict_and_comma_policies_are_preserved(): | ||
| assert unpack_and_flatten_and_dedup_list_of_strings('{"a": 1}') == [] | ||
| assert ExtendedBaseModel._load_var_to_list('{"a": 1}') == ['{"a": 1}'] | ||
| assert sorted(unpack_and_flatten_and_dedup_list_of_strings("a, b")) == ["a", "b"] | ||
| assert ExtendedBaseModel._load_var_to_list("a, b") == ["a, b"] | ||
| assert ExtendedBaseModel._load_var_to_list(["a", "a", "b"]) == ["a", "a", "b"] | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("value", ["[12345]", '[1, "a"]', [1, "a"]]) | ||
| def test_report_owner_lists_match_alert_strings(value): | ||
| from elementary.monitor.fetchers.models.schema import ArtifactSchema | ||
|
|
||
| expected = ["12345"] if value == "[12345]" else ["1", "a"] | ||
| assert ArtifactSchema(owners=value).owners == expected | ||
| assert sorted(unpack_and_flatten_and_dedup_list_of_strings(value)) == expected | ||
|
|
||
|
|
||
| def test_dependency_nodes_keep_commas_order_and_duplicates(): | ||
| from elementary.monitor.fetchers.models.schema import ExposureSchema | ||
|
|
||
| assert ExposureSchema(depends_on_nodes="a, b").depends_on_nodes == ["a, b"] | ||
| assert ExposureSchema(depends_on_nodes=["a", "a", "b"]).depends_on_nodes == [ | ||
| "a", | ||
| "a", | ||
| "b", | ||
| ] |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The list branch has the same problem as the scalar case for owners and tags:
"[12345]"returns an int, and mixed lists fail whenprettify_and_dedup_listsorts them. Note thattest_rows_samplealso comes through here and needs its list of dicts returned as is, so the conversion to strings belongs inunpack_and_flatten_and_dedup_list_of_stringsrather than here.