fix(types): quote non-string Dict keys so generated output is valid JSON#1917
Conversation
|
📚 Documentation preview: https://dottxt-ai.github.io/outlines/pr-preview/pr-1917/ Preview updates automatically with each commit. |
Dict[int, str] (and other non-str key types: float, bool, date, time,
datetime) produced a regex that accepted unquoted keys, e.g. {1:"a"},
which is not valid JSON (json.loads requires all object keys to be
double-quoted strings).
_ensure_json_quoted only wrapped String/Alternatives terms (used for
Literal/Enum string values) in quotes. Bare Regex terms like
types.integer, types.number, types.boolean, types.date etc. were left
unquoted when used as a Dict key.
Wrap non-self-quoted Regex key terms in literal double quotes in
_handle_dict. types.string is excluded since its pattern already
includes quotes. Dict[str, ...] behavior is unchanged.
Updated the existing test_dsl_handle_dict test (which asserted the
buggy unquoted behavior) and added a regression test that checks the
generated regex only matches valid JSON via json.loads.
2eb993f to
cb930a3
Compare
RobinPicard
left a comment
There was a problem hiding this comment.
Thanks for the PR, it's a good idea to fix that. I just have a suggestion to improve it.
| raise TypeError(f"Dict must have exactly two type arguments. Got {args}.") | ||
| # Add dict support with key:value pairs | ||
| key_type = _ensure_json_quoted(python_types_to_terms(args[0], recursion_depth + 1)) | ||
| if isinstance(key_type, Regex) and key_type.pattern != types.string.pattern: |
There was a problem hiding this comment.
I think a better solution would be to add a parameter to _ensure_json_quoted to indicate whether to add quotation marks to Regex terms on top of String terms (default would be False). That way we benefit from the recursive functioning of this function in case the term is an Alternative and 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.
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).
The standalone isinstance(key_type, Regex) check in _handle_dict only caught a bare Regex key type. It missed the case where the key type resolves to an Alternatives of Regex terms instead (e.g. Dict[Literal[1, 2, 3], str], since int/float Literal members are kept as Regex, not String, so they go unquoted through the existing Alternatives recursion in _ensure_json_quoted). Adding a quote_regex parameter to _ensure_json_quoted reuses that same recursion for both cases, and keeps everything about quoting for JSON containers in one function instead of splitting it across two.
RobinPicard
left a comment
There was a problem hiding this comment.
The code change itself looks fine, but there are some changes to make in terms of style and the tests need to be better focused
| 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 |
There was a problem hiding this comment.
Let's keep all the imports at the top of the file when possible
There was a problem hiding this comment.
Moved to the module-level imports (json and Literal were both already imported at the top).
| only match strings that are valid JSON, i.e. the key must be quoted.""" | ||
| import json | ||
|
|
||
| dict_type = dict[int, str] |
There was a problem hiding this comment.
It's duplicating what's above
There was a problem hiding this comment.
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.
| # 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"}') |
There was a problem hiding this comment.
That does not make sense, we don't want to test that the json library works well.
There was a problem hiding this comment.
Agreed, removed the json.loads / JSONDecodeError lines. Kept just the assertion on our generated term structure.
| raise TypeError(f"Dict must have exactly two type arguments. Got {args}.") | ||
| # Add dict support with key:value pairs | ||
| key_type = _ensure_json_quoted(python_types_to_terms(args[0], recursion_depth + 1)) | ||
| # JSON object keys must always be double-quoted strings, even when the |
There was a problem hiding this comment.
I don't think we need such a long comment
There was a problem hiding this comment.
Trimmed it down to two lines.
| def _ensure_json_quoted(term: Term) -> Term: | ||
| """Wrap bare ``String`` terms in double quotes for JSON container contexts. | ||
| def _ensure_json_quoted(term: Term, quote_regex: bool = False) -> Term: | ||
| """Wrap bare ``String`` terms (and, if ``quote_regex``, ``Regex`` terms) in |
There was a problem hiding this comment.
Let's try to shorten it
There was a problem hiding this comment.
Shortened, kept the quote_regex explanation and the Alternatives note, dropped the rest.
- Shorten the _ensure_json_quoted docstring and the _handle_dict comment. - Drop the redundant dict[int, str] regex test (the structural assertion in test_dsl_handle_dict already covers that key-quoting case) and the json.loads/JSONDecodeError checks (those exercise the stdlib, not our code). - Keep one focused structural test for the Literal[int] key case, which is the Alternatives-of-Regex path the quote_regex change actually enables. Move its imports to the module top.
|
Pushed 4191032 addressing the review: shortened the _ensure_json_quoted docstring and the _handle_dict comment, moved the test imports up, dropped the redundant dict[int, str] regex test (already covered structurally) and the json.loads checks, and refocused the remaining test on the Literal[int] Alternatives case as a structural assertion. The quote_regex refactor you suggested is in d5c58af. Full test_dsl.py passes (57, minus the 2 pre-existing from_file temp-file failures unrelated to this change). |
|
Note on the red X: the 3.10 job failed inside the ollama integration tests ( |
| assert result.terms[1].term.terms[2] == types.string | ||
|
|
||
|
|
||
| def test_dsl_handle_dict_literal_int_key_quoted(): |
There was a problem hiding this comment.
Let's just include it in the test test_dsl_handle_dict as it's doing the same thing with a different value for dict_type
There was a problem hiding this comment.
Done in 13fe8dd. Folded it in as another dict_type block right after the dict[int, str] one, since the assertions are the same shape and it reads as a continuation rather than a separate concern.
Kept the two-line comment on it because the why isn't obvious from the code alone: Literal[1, 2, 3] reaches _handle_dict as Alternatives([Regex("1"), ...]) rather than a bare Regex, which is exactly the path the original version of this fix missed. tests/types/test_dsl.py: 58 passed.
Same assertion shape as the dict[int, str] block right above it, just a different dict_type, so it reads better as another case in the existing test than as a separate one.
Problem
Dict[K, V]whereKis notstr(e.g.int,float,bool,date,time,datetime) produces a regex that accepts unquoted keys, which is not valid JSON:JSON requires all object keys to be double-quoted strings regardless of the semantic key type. Any user constraining generation with a
Dict[int, ...]-style annotation (directly, or nested inside a Pydantic/dataclass field) can get output that failsjson.loads.Root cause
_ensure_json_quotedonly wrapsString/Alternatives-of-Stringterms (used forLiteral/Enumstring values) in quotes. BareRegexterms such astypes.integer,types.number,types.boolean,types.date,types.time,types.datetimeare left unquoted when used as a dict key, since they're valid unquoted in other JSON contexts (e.g. as a value).types.stringis the exception -- its pattern is already self-quoted ("[^"]*").Fix
In
_handle_dict, after computing the key term, wrap it in literal double quotes unless it's already self-quoted (types.string).Dict[str, ...]behavior is unchanged.Testing
test_dsl_handle_dict(it previously asserted the buggy unquoted behavior fordict[int, str]).test_dsl_handle_dict_non_string_key_produces_valid_json, which checks viajson.loadsthat the generated regex only matches valid JSON for both the quoted and unquoted key forms.tests/types/test_dsl.pysuite passes locally (59 passed).