fix: only set additionalProperties on object schemas#1909
Conversation
fd1f2cd to
7ea83e6
Compare
RobinPicard
left a comment
There was a problem hiding this comment.
Thanks for opening a PR! There's a conflict to solve with the main main as the file you are modifying has been updated. Let's remove the jsonpath_ng dependency too as you are removing the only use of this library.
| # appearing anywhere would wrongly add the keyword to non-object | ||
| # schemas, e.g. a string field with ``const``/``default``/``title`` | ||
| # equal to ``"object"``, which the OpenAI API then rejects. | ||
| if node.get("type") == "object" and "additionalProperties" not in node: |
There was a problem hiding this comment.
This misses nullable object schemas whose type is ["object", "null"]. Those nested objects keep no additionalProperties, so the strict OpenAI/Mistral schema is rejected; the equivalent check needs to handle lists containing "object" too.
set_additional_properties_false_json_schema keyed off any value equal
to the string 'object' (match.value == 'object') and then mutated that
node's parent. This wrongly injected 'additionalProperties': False into
non-object schemas whenever a scalar value happened to equal 'object' --
for example a string field emitted by pydantic as {'type': 'string',
'const': 'object'} for a Literal['object'] discriminator, or a field
with a 'default'/'title'/'description' of 'object'.
The OpenAI structured-output API rejects 'additionalProperties' on
non-object schemas, so a legitimate model would fail with a 400 error.
Rewrite the helper to recurse over the schema and set
'additionalProperties' only on dicts whose 'type' is 'object', matching
the JSON Schema keyword rather than an incidental string value. Nested
object schemas are still handled. Add regression tests.
7ea83e6 to
8f77a16
Compare
|
Thanks @RobinPicard! Rebased onto main so the conflict is resolved, and removed the @Sanjays2402 good catch on nullable objects — the check now also matches |
ErenAta16
left a comment
There was a problem hiding this comment.
Reproduced the old bug directly before checking the fix, since the whole thing hinges on whether the old jsonpath walk really did hit non-object schemas.
It does. Running the old $..* version against the PR's own repro ({"type": "string", "const": "object", "title": "object"} nested under an object) adds additionalProperties: False straight onto the string schema, because $..* visits every value in the tree, "object" shows up as the value of const and title, and match.context.value is then the string schema itself. So the fix is addressing a real 400 from the OpenAI side, not a theoretical one.
The rewritten _walk keys off node.get("type") instead of any value that happens to equal "object", which is the correct signal. Checked it against a few cases beyond the one in the test:
- The repro's string-with-
const: "object"field: correctly left untouched. - Plain nested object: still gets
additionalProperties: False. - Nullable object with
"type": ["object", "null"]: still gets it, the list branch is preserved. - A property literally named
objectwith a string type: correctly left untouched (the key is walked as a value, has no matchingtype, so nothing is added).
One thing worth noting for whoever merges: _walk mutates schema in place and returns the same object, same as the old version did, so behavior for existing callers is unchanged there. Dropping jsonpath_ng entirely is a nice side benefit given this was its only use.
Looks correct and well-scoped to me.
|
📚 Documentation preview: https://dottxt-ai.github.io/outlines/pr-preview/pr-1909/ Preview updates automatically with each commit. |
What this fixes
set_additional_properties_false_json_schema(used by the OpenAI backend to make a schemastrict-compatible) sometimes injects"additionalProperties": falseinto non-object schemas. The OpenAI structured-output API rejectsadditionalPropertieson anything that isn't an object, so a legitimate schema fails with a400 invalid_request_error.Reproduction
Any scalar value equal to the string
"object"triggers it — aLiteral["object"]field (emitted asconst), or a field whosedefault/title/descriptionis"object".Root cause
It keys off the string value
"object"appearing anywhere in the schema and then mutates that node's parent, instead of keying off the JSON Schema keyword{"type": "object"}.Fix
Recurse over the schema and set
additionalPropertiesonly on dicts whosetypeis"object". Nested object schemas are still handled; already-presentadditionalPropertiesvalues are preserved. This also drops thejsonpath_ngusage in this helper in favour of a plain recursive walk.Test evidence
Added two regression tests in
tests/models/test_utils.py:test_set_additional_properties_false_json_schema_string_value_object— fails onmain, passes with the fix (a string field withconst/title"object"must not getadditionalProperties).test_set_additional_properties_false_json_schema_nested_objects— documents that nested object schemas still getadditionalProperties.ruff checkandmypy --allow-redefinitionboth pass on the changed files.