Repository navigation
fix: name root self-references after the generated root class - #5671
Conversation
This comment has been minimized.
This comment has been minimized.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
marvin-context-protocol
Bot
added
the
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: 6a2dee5658
ℹ️ 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".
| # Handle self-reference | ||
| if ref == "#": | ||
| return ForwardRef(_sanitize_name(schema.get("title", "Root"))) | ||
| return ForwardRef(_conversion.get().root_class_name) |
There was a problem hiding this comment.
When a nested object has the same sanitized title as the root (or as the explicit name= override), this ForwardRef resolves in the nested class's namespace to the nested class rather than the document root. For a "$ref": "#" reached through anyOf, items, or the Pydantic-model path, validation then enforces the nested object's fields, so it can accept inputs that violate the root schema and reject valid root instances. Use an unambiguous root identity (or unique generated names) instead of the potentially duplicated class name.
Useful? React with 👍 / 👎.
| _Conversion( | ||
| root_hash=root_hash, | ||
| root_size=root_size, | ||
| root_class_name=_sanitize_name(name or schema.get("title", "Root")), |
There was a problem hiding this comment.
When the same schema is converted with two different name= overrides and a nested generated object contains a generic root reference, this value changes but the nested class cache key does not: _create_dataclass and _create_pydantic_model key it by the schema/root hashes and its own name, while the schema-derived root hash omits the override. The second conversion therefore reuses (for example) a cached ForwardRef('First') while building Second, leaving TypeAdapter(Second) not fully defined or binding nested values to the first class if that name is available. Include root_class_name in generated-class cache keys.
Useful? React with 👍 / 👎.
Rename a nested class that would share the root's generated name, so pydantic binds "$ref": "#" to the root class instead of capturing it in the nested class's own name. Also include the root class name in generated-class cache keys, so converting the same schema under two different name= overrides no longer reuses a nested class whose forward references name the first conversion's root.
|
Both findings confirmed and fixed in b1d79f5. P1 (same-name binding): confirmed — pydantic binds a class's own name to that class when resolving its annotations, so a nested class sharing the root's name captured every P2 (cache pollution): confirmed — nested classes bake the root class name into their forward references, but the cache key omitted it. The root class name is now part of generated-class cache keys. Covered by |
BrickerP
left a comment
BrickerP
left a comment
There was a problem hiding this comment.
Read the diff at b1d79f5 against main and ran the module on its own (pydantic 2.x) with some probe schemas, comparing main against this head.
What's sound. Tracking root_class_name on _Conversion and returning ForwardRef(root_class_name) from the ref == "#" branch of _convert_schema fixes the bug at its source. The old code named the ref after the bare {"$ref": "#"} node, which never has a title, so the ref could not resolve. Building the name as name or title or "Root" matches how _create_dataclass/_create_pydantic_model name the root, so the name= override case works too. The second commit is also right. Pydantic binds a class's own name before the root, so a nested class with the root's name (_nested_class_name → Node_2) would capture # refs, and renaming it stops that. Adding root_class_name to the _classes cache key stops a First forward ref from being reused while building Second. I confirmed that these now work where they failed on main: $defs recursion back to the root (Tree → Branch → #), root "my node" with nested "my-node" (both sanitize to my_node), and an untitled root with an untitled nested object that refs #. That last one used to validate # against the nested shape; now it validates against the root shape.
Residuals (the first two are new in this PR):
- Regression: untitled
type: ["object", "null"]root.{"type": ["object","null"], "properties": {"name": {...}, "next": {"anyOf": [{"$ref": "#"}, {"type": "null"}]}}}validates onmain(classRoot). On this branchTypeAdapterraises "not fully defined" onOptional[Root_2]. The cause is the type-list branch of_convert_schema, which builds the object class fromtype_schema = dict(schema). That copy fails theschema is not schemasidentity check in_nested_class_name, so the root class itself gets renamedRoot_2while#still points toRoot. The titled form was already broken onmainand still is (Node_2). An explicit root flag, or treating a copy that differs only intypeas the root, would fix both. - Regression: non-string
titlenow crashes every conversion._convert_normalizednow calls_sanitize_name(name or schema.get("title", "Root"))up front for every schema, including scalar and dict-only roots that never create a class.{"type": "string", "title": null},{"type": "string", "title": 5}and{"type": "object", "title": 5}all converted onmain. Here they raiseTypeError: expected string or bytes-like objectfromre.sub. These schemas are malformed, but they can arrive from untrusted servers. Computing the name lazily, or only usingtitlewhenisinstance(title, str), avoids the crash. - Pre-existing, same bug class, not covered: the shortcut in
_build_dataclass(elif prop_schema.get("$ref") == "#": field_type = ForwardRef(sanitized_name)) uses the current class's name. On the root that matches, but a nested dataclass with a direct"root": {"$ref": "#"}property points the ref at itself. With rootDoc(requiredvalue) and nestedChild(requiredtag),{"child": {"root": {"tag": "x"}}}is accepted and a valid root shape is rejected. Using_conversion.get().root_class_namethere fixed both cases for me. Theref == "#"early return at the top of_build_dataclassprobably needs the same change. - Pre-existing: an untitled root with
propertiesbut notypegoes through_create_dataclass(schema, schema.get("title", "<unknown>"), ...). The class is namedfield_unknownwhile#points toRoot, so it's still "not fully defined". The titled typeless form is fixed by this PR. - CI:
static_analysisis red becausety checkflags the new tests (test_advanced.py:290,:291,:349,:355): thechild.__args__[0] if hasattr(child, "__args__") else childunwrap on the result ofget_dataclass_field(...).type. A small typed helper or acastwould make it pass. - Nit: the
_2suffix isn't checked against other names. A nested object really titledNode_2next to a renamed one gives two classes with the same__name__. That's harmless for#resolution but confusing in error messages and generated schemas.
The approach is right. I'd fix the first two regressions and the ty failure before merge, and ideally route the _build_dataclass direct-property shortcut through root_class_name in the same PR, since it's the same bug.
- convert the root schema itself (not a copy) for the object member of a root type list, so the generated class keeps the name that "$ref": "#" resolves to; untitled ["object", "null"] roots no longer build Root_2 against refs to Root, and the titled form now resolves where main also failed - only treat a string title as a class-name source in _convert_normalized, so malformed titles (null, numbers) no longer crash conversions that previously succeeded - route _build_dataclass's direct "#" shortcuts through root_class_name so a direct root reference on a nested class no longer points at the nested class itself - add an unwrap_optional test helper so ty check passes on the tests
|
Thanks @BrickerP — everything you flagged is addressed in 953953f, including the same-bug
The two pre-existing items (untitled root with
|
|
@zzstoatzz — quick coordination question on this one. #5779 now carries this PR's root-reference binding and tests, with BlueX888 credited as co-author. Would you prefer that I close #5671 in #5779's favor, or keep the focused fix here and land it first? Happy to close it once you confirm. Two facts that may matter for sequencing:
Also, #5779's body says |
BrickerP
left a comment
BrickerP
left a comment
There was a problem hiding this comment.
APPROVE
Re-reviewed at 953953f. The follow-up commit clears the residuals I flagged at b1d79f5:
- type-list roots now convert the root schema itself for the
objectmember, so untitled["object","null"]keeps the name"$ref": "#"resolves to (and the titled form that was broken on main now works too) - non-string
titlevalues are ignored for class naming instead of crashing in_sanitize_name _build_dataclass's direct"#"shortcuts go throughroot_class_nameunwrap_optionalunblocksty checkon the new tests
The _2 suffix collision and typeless untitled roots remain edge cases outside this fix's scope. Good to merge from my side.
(cherry picked from commit 2c3a2de)
Description
json_schema_to_typereturned a broken type for any recursive object schema that carries a roottitle: validating the result withTypeAdapterraisedPydanticUserError: ... is not fully defined. Untitled recursive schemas already worked; the titled form — the one most schema producers emit — did not.Why: a
{"$ref": "#"}reference resolves to the root schema, whose generated class is named after the roottitle(falling back toRoot). Theref == "#"branch in_convert_schema(fastmcp_slim/fastmcp/utilities/json_schema_type.py:766onmain) built the forward reference from the ref node instead. A bare{"$ref": "#"}node has notitle, so the forward reference was always namedRootwhile the class was named e.g.Node, and the reference never resolved. The mismatch hit every generic reference position (anyOf, arrayitems) and all fields of schemas that build a Pydantic model (properties+additionalProperties: true), which go through the generic handling rather than_build_dataclass's direct self-reference check.Changes:
_Conversionnow tracksroot_class_name, the sanitized name of the class generated for the root schema — the explicitnameoverride, else the roottitle, elseRoot— the same derivation_create_dataclass/_create_pydantic_modelalready use.ref == "#"branch returnsForwardRef(root_class_name), so every reference position targets the class that will actually be generated. This also fixes the case where an explicitname=override makes the class name differ from the roottitle.TestCircularReferencescover a titled root self-referenced viaanyOf, via arrayitems, via a direct property on a schema withadditionalProperties: true(the Pydantic-model branch), and with an explicitname=override.typelist is converted from the root schema itself, so["object", "null"]roots keep the root identity and name that"#"references resolve to; a non-string roottitleis ignored instead of crashing the conversion; and_build_dataclass's direct"#"shortcuts route throughroot_class_name, so a direct root reference on a nested class no longer points at the nested class itself.Testing:
mainwithPydanticUserError: ... is not fully definedand pass with this change:uv run pytest tests/utilities/json_schema_type/test_advanced.py -k self_reference→6 passed, 72 deselecteduv run pytest tests/utilities/json_schema_type/→419 passed, 1 skippeduv run pytest tests/utilities/ tests/client/test_elicitation.py tests/client/test_elicitation_enums.py→1313 passed, 1 skipped(elicitation builds its response type throughjson_schema_to_type_adapter)uv run prek run --all-files→ validate-pyproject, prettier, ruff check, ruff format, ty check, gitleaks, codespell all Passed (loq reports pre-existing, unenforced repo-wide size findings).Fixes #5669
Contribution type
Checklist
uv run prek run --all-filesand all checks pass