[DNM] Handle allOf and oneOf in json_schema_to_type#3840
Conversation
marvin-context-protocol
Bot
added
bug
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62c3f79d06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| merged: dict[str, Any] = {} | ||
| merged_properties: dict[str, Any] = {} | ||
| merged_required: list[str] = [] |
There was a problem hiding this comment.
The allOf merge path initializes merged/merged_properties from empty dicts and only copies fields from each allOf child, so any sibling properties/required already present on the current schema are dropped. In nested schemas that combine local fields with allOf, this now silently stops validating previously-required local fields (and omits them from the parsed object), which is a regression in validation behavior for composed objects.
Useful? React with 👍 / 👎.
| for subschema in schema["oneOf"]: | ||
| types.append(_schema_to_type(subschema, schemas)) |
There was a problem hiding this comment.
The oneOf loop always calls _schema_to_type(subschema, schemas) directly, unlike anyOf which special-cases object schemas with additionalProperties to produce dict[...]. As a result, a branch like {"type":"object","additionalProperties":{"type":"string"}} becomes an empty dataclass in oneOf, so arbitrary dict inputs validate as Root() and key/value data is discarded instead of being type-checked as a mapping.
Useful? React with 👍 / 👎.
strawgate
force-pushed
the
fix/json-schema-allof-oneof
branch
2 times, most recently
from
4112e18 to
39f8815
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39f88156e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(sub, bool): | ||
| return |
There was a problem hiding this comment.
false branches in allOf as unsatisfiable
Returning early for boolean sub-schemas silently ignores false inside allOf, but JSON Schema semantics require allOf to fail if any member is false. With the current logic, a schema like {"allOf": [false, {"type":"object", ...}]} still validates inputs against the object branch, so invalid payloads are accepted instead of being rejected.
Useful? React with 👍 / 👎.
| if "$ref" in sub: | ||
| sub = dict(_resolve_ref(sub["$ref"], schemas)) |
There was a problem hiding this comment.
When an allOf member is a $ref to a boolean schema (valid in draft-06+), this code does dict(_resolve_ref(...)), which raises TypeError for true/false targets. That makes json_schema_to_type crash on valid schemas such as {"$defs": {"Never": false}, "allOf": [{"$ref": "#/$defs/Never"}]} instead of producing an unsatisfiable type.
Useful? React with 👍 / 👎.
| merged_properties.update(schema.get("properties", {})) | ||
| merged_required.extend(schema.get("required", [])) |
There was a problem hiding this comment.
The allOf merge only copies sibling properties/required from the parent schema, so parent-level additionalProperties is dropped. For schemas that combine allOf with additionalProperties: true, this changes behavior from the normal object path (which builds a BaseModel with extras allowed) to a dataclass path that drops unknown keys, causing silent data loss for extra fields.
Useful? React with 👍 / 👎.
This comment was marked as low quality.
This comment was marked as low quality.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb5c0f536b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return object | ||
|
|
||
| if "type" not in schema and "properties" in schema: | ||
| if "type" not in schema and "properties" in schema and "allOf" not in schema: |
There was a problem hiding this comment.
The early return for schemas with implicit objects ("type" missing + "properties" present) only excludes allOf, so schemas that also include oneOf never reach the new union logic. In those cases, only the top-level properties are enforced and branch-specific oneOf constraints are skipped, which can accept invalid payloads in discriminator-style schemas.
Useful? React with 👍 / 👎.
| if resolved is False: | ||
| has_false = True | ||
| return | ||
| sub = dict(resolved) |
There was a problem hiding this comment.
When an allOf member contains $ref plus sibling keywords, sub = dict(resolved) overwrites the original branch and drops those siblings. Under JSON Schema 2020-12, adjacent keywords with $ref are evaluated together, so this can silently remove required fields or other constraints from referenced branches and under-validate inputs.
Useful? React with 👍 / 👎.
| isinstance(subschema, dict) | ||
| and subschema.get("type") == "object" | ||
| and not subschema.get("properties") | ||
| and subschema.get("additionalProperties") |
There was a problem hiding this comment.
The oneOf map-branch check relies on truthiness of additionalProperties; for additionalProperties: {} (valid and common for free-form maps), the condition is false and the branch is parsed as an object dataclass instead of dict[str, Any]. That causes oneOf unions to lose map semantics for unconstrained dictionary branches.
Useful? React with 👍 / 👎.
Previously these compositions silently resolved to Any, disabling all validation for schemas that use them. allOf is now merged into a single object schema; oneOf creates a Union type. 🤖 Generated with Claude Code Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix dict() crash when $ref resolves to boolean schema (draft-06+) - Treat allOf containing 'false' as unsatisfiable (reject all values) - Add tests for boolean ref, false sub-schema, and false-only allOf - additionalProperties preservation from parent noted as pre-existing limitation
- Guard property+oneOf schemas from premature dataclass return
- Recognize empty additionalProperties ({}) as allow-any in oneOf
- Boolean false allOf branches and sibling properties already handled
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
strawgate
force-pushed
the
fix/json-schema-allof-oneof
branch
from
fb5c0f5 to
08236c4
Compare
|
tl;dr (edited 2026-05-13 to reflect latest run): The latest CI run failed on the same unrelated issue — a pytest-timeout in Latest failure (run 25781683564, head Failure excerptPrevious analysis (kept for context — still applicable when this PR comes off DNM): Static analysis ( Fix: Append
After that, run |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08236c4345
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -453,7 +453,7 @@ def _schema_to_type( | |||
| if not schema: | |||
There was a problem hiding this comment.
For schemas that include the standard OpenAPI form {"type": "object", "allOf": [...]} or {"type": "object", "oneOf": [...]}, the top-level json_schema_to_type() returns through _object_schema_to_type() before this _schema_to_type() guard and the new composition handlers are never reached. That still accepts invalid payloads as dict[str, Any] or a plain object model instead of enforcing the composed branches, so typed object compositions need to be routed through the allOf/oneOf logic before the object shortcut.
Useful? React with 👍 / 👎.
| if has_false: | ||
| return _UnsatisfiableType # type: ignore[return-value] | ||
|
|
||
| if merged_properties: |
There was a problem hiding this comment.
When an allOf branch is an object map such as {"type": "object", "additionalProperties": {"type": "integer"}}, it has no properties, so merged_properties stays empty and the function falls through to Any. In that case TypeAdapter(json_schema_to_type(schema)).validate_python({"x": "bad"}) is accepted even though the composed map branch should enforce integer values.
Useful? React with 👍 / 👎.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 087d4b3b84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if "allOf" in sub: | ||
| for nested in sub["allOf"]: | ||
| _collect_allof(nested) | ||
| merged_properties.update(sub.get("properties", {})) |
There was a problem hiding this comment.
When the same property appears in multiple allOf branches, update() keeps only the last branch's schema instead of requiring the value to satisfy both schemas. For example, allOf with x constrained to enum: ["a"] in one branch and type: string in a later branch becomes just string, so { "x": "b" } is accepted even though it fails the composed schema; branch order can now silently broaden or narrow validation.
Useful? React with 👍 / 👎.
| if has_null: | ||
| return Union[(*types, type(None))] # type: ignore | ||
| return Union[tuple(types)] # type: ignore # noqa: UP007 |
There was a problem hiding this comment.
This returns a plain Union, which accepts values that match more than one branch instead of rejecting them as JSON Schema oneOf requires. In overlapping cases such as oneOf: [{"type":"integer"}, {"type":"number"}], the value 1 validates successfully even though it matches both branches, so schemas using oneOf for mutual exclusion are under-validated.
Useful? React with 👍 / 👎.
| else: | ||
| types.append(_schema_to_type(subschema, schemas)) |
There was a problem hiding this comment.
When a schema has sibling properties/required next to oneOf, this loop builds the result only from each child schema and drops the parent constraints. For a schema like { "required": ["id"], "properties": {"id": ...}, "oneOf": [...] }, any payload matching a child branch validates even when id is missing, but JSON Schema evaluates the sibling keywords together with the oneOf.
Useful? React with 👍 / 👎.
Bugs fixed:
- additionalProperties: {} now treated as true (per JSON Schema spec)
- Sibling properties override allOf children (local > inherited)
- additionalProperties: false wins when allOf children conflict
- anyOf guard added to typeless-properties shortcircuit
New test coverage:
- Empty additionalProperties schema (with and without properties)
- Sibling property precedence over allOf children
- additionalProperties intersection in allOf
- oneOf with null branch
- properties + anyOf not shortcircuited
- allOf with only required (no properties)
- oneOf with empty additionalProperties dict
- allOf with sibling additionalProperties: true
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 025ed3594f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| and "properties" in schema | ||
| and "allOf" not in schema | ||
| and "oneOf" not in schema | ||
| and "anyOf" not in schema |
There was a problem hiding this comment.
When an implicit object schema has sibling properties/required plus anyOf, this new guard sends it to the anyOf handler, but that handler builds a union only from the child schemas and drops the sibling object constraints. For example, { "required": ["id"], "properties": {"id": {"type": "string"}}, "anyOf": [{"properties": {"a": {"type": "integer"}}}] } now accepts {"a": 1} with no id, whereas the sibling keywords must still be evaluated alongside anyOf.
Useful? React with 👍 / 👎.
| if has_false: | ||
| return _UnsatisfiableType # type: ignore[return-value] | ||
|
|
||
| if merged_properties: |
There was a problem hiding this comment.
This allOf implementation only returns a merged schema when it collected object properties; if the branches are scalar, array, or map-only schemas, merged_properties stays empty and the function falls through to Any when there is no top-level type. A valid schema like {"allOf": [{"type": "string", "minLength": 2}]} therefore validates 1 or any other value, so non-object allOf compositions are completely under-validated.
Useful? React with 👍 / 👎.
| if ( | ||
| "additionalProperties" not in merged | ||
| and "additionalProperties" in schema | ||
| ): | ||
| merged["additionalProperties"] = schema["additionalProperties"] |
There was a problem hiding this comment.
When an allOf child sets additionalProperties: true and the parent schema sets additionalProperties: false, this condition skips the parent value because merged already has the child's setting. JSON Schema applies the sibling keyword too, so a parent-level false should still forbid extras; currently the merged model allows and preserves unknown keys for schemas such as {"additionalProperties": false, "allOf": [{"type": "object", "properties": {"a": {"type": "string"}}, "additionalProperties": true}]}.
Useful? React with 👍 / 👎.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9543964de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if "allOf" in sub: | ||
| for nested in sub["allOf"]: | ||
| _collect_allof(nested) | ||
| merged_properties.update(sub.get("properties", {})) |
There was a problem hiding this comment.
When an allOf child has additionalProperties: false, unioning every child’s properties into one merged schema makes that closed child allow properties that were only declared by sibling branches. For example, allOf with one branch declaring only a plus additionalProperties: false and another branch declaring b should reject {"a":"x","b":1} because b is an extra property for the closed branch, but the merged model includes both a and b and validates it, so these composed closed schemas are under-enforced.
Useful? React with 👍 / 👎.
2 participants
Summary
json_schema_to_type()handlesanyOfbut silently returnsAnyforallOfandoneOfschemas, disabling all validation. This is the most common schema composition pattern in OpenAPI 3.x specs.allOf(schema intersection) sub-schemas are now merged into a single object type. Properties and required fields from all sub-schemas are combined.$refsub-schemas are resolved before merging. NestedallOf(including$ref→allOf) is flattened recursively.oneOf(exactly-one-of) is now treated likeanyOf— creates a Union type. Pydantic's "first match" semantics are a reasonable approximation.Known limitations
Property constraint merging: if two allOf sub-schemas define the same property with different constraints, the second sub-schema's definition wins (last-writer from
dict.update). Merging per-property constraints (e.g., combiningminLengthfrom one withmaxLengthfrom another) is not implemented. This is rare in practice — the standard pattern uses allOf to combine different properties.Non-object allOf:
allOfwith scalar sub-schemas that add constraints (e.g.,[{type: string}, {minLength: 3}]) doesn't merge the constraints. Object composition (the dominant real-world use case) works correctly.Closes #3839
🤖 Generated with Claude Code