-
Notifications
You must be signed in to change notification settings - Fork 802
fix(types): quote non-string Dict keys so generated output is valid JSON #1917
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
Changes from 1 commit
cb930a3
d5c58af
4191032
13fe8dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -839,21 +839,50 @@ def test_dsl_handle_dict(): | |
| incorrect_dict_type = dict[int, str, int] | ||
| _handle_dict(get_args(incorrect_dict_type), recursion_depth=0) | ||
|
|
||
| # correct type | ||
| dict_type = dict[int, str] | ||
| # correct type with a str key: no extra quoting needed, types.string | ||
| # is already self-quoted | ||
| dict_type = dict[str, int] | ||
| result = _handle_dict(get_args(dict_type), recursion_depth=0) | ||
| assert isinstance(result, Sequence) | ||
| assert len(result.terms) == 3 | ||
| assert result.terms[0] == String("{") | ||
| assert isinstance(result.terms[1], Optional) | ||
| assert isinstance(result.terms[1].term, Sequence) | ||
| assert len(result.terms[1].term.terms) == 4 | ||
| assert result.terms[1].term.terms[0] == types.integer | ||
| assert result.terms[1].term.terms[0] == types.string | ||
| assert result.terms[1].term.terms[1] == String(":") | ||
| assert result.terms[1].term.terms[2] == types.string | ||
| assert result.terms[1].term.terms[3] == KleeneStar(Sequence([String(", "), types.integer, String(":"), types.string])) | ||
| assert result.terms[1].term.terms[2] == types.integer | ||
| assert result.terms[1].term.terms[3] == KleeneStar(Sequence([String(", "), types.string, String(":"), types.integer])) | ||
| assert result.terms[2] == String("}") | ||
|
|
||
| # non-str key (e.g. int): JSON object keys must always be double-quoted | ||
| # strings, so the bare `types.integer` regex must be wrapped in quotes | ||
| # even though the Python key type is `int`. | ||
| dict_type = dict[int, str] | ||
| result = _handle_dict(get_args(dict_type), recursion_depth=0) | ||
| quoted_int_key = Sequence([String('"'), types.integer, String('"')]) | ||
| assert result.terms[1].term.terms[0] == quoted_int_key | ||
| assert result.terms[1].term.terms[2] == types.string | ||
|
|
||
|
|
||
| def test_dsl_handle_dict_non_string_key_produces_valid_json(): | ||
| """Regression test: Dict[int, str] (and other non-str key types) must | ||
| only match strings that are valid JSON, i.e. the key must be quoted.""" | ||
| import json | ||
|
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. Let's keep all the imports at the top of the file when possible
Contributor
Author
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. Moved to the module-level imports (json and Literal were both already imported at the top). |
||
|
|
||
| dict_type = dict[int, str] | ||
|
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. It's duplicating what's above
Contributor
Author
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. Right, dropped it. The dict[int, str] key-quoting is already covered by the structural assertion in test_dsl_handle_dict, so this one was redundant. |
||
| result = _handle_dict(get_args(dict_type), recursion_depth=0) | ||
| pattern = to_regex(result) | ||
|
|
||
| # unquoted key: not valid JSON, must not match | ||
| assert _re.fullmatch(pattern, '{1:"a"}') is None | ||
| with pytest.raises(json.JSONDecodeError): | ||
| json.loads('{1:"a"}') | ||
|
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. That does not make sense, we don't want to test that the
Contributor
Author
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. Agreed, removed the json.loads / JSONDecodeError lines. Kept just the assertion on our generated term structure. |
||
|
|
||
| # quoted key: valid JSON, must match | ||
| assert _re.fullmatch(pattern, '{"1":"a"}') is not None | ||
| json.loads('{"1":"a"}') # does not raise | ||
|
|
||
|
|
||
| def test_ensure_json_quoted_string(): | ||
| """String terms are wrapped in double-quote delimiters.""" | ||
|
|
||
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.
I think a better solution would be to add a parameter to
_ensure_json_quotedto indicate whether to add quotation marks toRegexterms on top ofStringterms (default would beFalse). That way we benefit from the recursive functioning of this function in case the term is anAlternativeand we have a better containment of the issue (everything related to adding quotation marks in a single function).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.
Good catch, and it's more than a style improvement: the isinstance(key_type, Regex) check I had only handles a bare Regex, it misses Dict[Literal[1, 2, 3], str] entirely, since int Literal members go through _handle_literal into Alternatives([Regex("1"), Regex("2"), Regex("3")]), not a top-level Regex. Confirmed that produces unquoted numeric keys with the old code, same bug through a different path.
Added quote_regex to _ensure_json_quoted (d5c58af) and dropped the separate check, so it goes through the same Alternatives recursion String quoting already uses. Added a regression test for the Literal[int] case specifically. Full test_dsl.py passes except two pre-existing, unrelated failures on this Windows box (temp-file PermissionError in the from_file tests, reproduces identically on main without my changes).