fix(json-schema): annotate optional fields as Optional[T] so from_json accepts None#150
Conversation
…n accepts None
from_json_schema() built optional fields (those absent from `required`) with
`default=None` but a NON-nullable annotation (bare `str`, not `Optional[str]`).
Plain `ModelClass(**data)` tolerated the omitted field, but
`from_json(process_rich_values=True)` did not: the rich-value path round-trips
nested objects through `from_json(...).model_dump()`
(ConfigurationHelper._process_nested_structured_data), which materializes the
`None` default and re-validates it against the bare annotation, raising a
pydantic ValidationError ("Input should be a valid string ... input_value=None").
Fix widens `field_type` to `Optional[field_type]` whenever the field is not
required, mirroring the existing `default = ... if is_required else None`
decision, at all three conversion sites:
- primitive types (convert_property_to_field)
- nested objects (_handle_nested_object)
- arrays (_handle_array_type)
Required fields keep their bare annotation. Downstream configuration_helper
already handles `Optional[...]` correctly; it simply never received them.
Existing converter assertions that checked bare `is str`/`is int`/List[...] for
NOT-REQUIRED fields are updated to unwrap the Optional wrapper (required fields
stay bare). Adds regression coverage for the awslabs#149 repro and for the
optional->Optional annotation at all three sites.
Fixes awslabs#149
Security Scan ResultsASH Security Scan Report
Scan Metadata
SummaryScanner ResultsThe table below shows findings by scanner, with status based on severity thresholds and dependencies:
Top 1 HotspotsFiles with the highest number of security findings:
Detailed FindingsShow 1 actionable findingsFinding 1: CKV2_GHA_1
Description: Report generated by Automated Security Helper (ASH) at 2026-06-08T08:24:34+00:00 |
There was a problem hiding this comment.
apologies for the delayed review - thanks for this! just a couple things:
blocking:
Optional[NestedModel]isn't recognized byis_structured_field_type, socompare_recursivesilently drops the nested metric breakdown for optional object fields inside array items, verified empirically against base. see the inline comment in_handle_nested_object.
worth resolving before merge:
- the parallel stickler-config path (
field_converter.py) still has the identical bug, andto_stickler_configround-trips a fixed model back into a broken one. inline comment inconvert_property_to_field. requiredfields with an explicit"default": nullstill crash the same way; the widening condition probably wantsnot is_required or default is None. inline atjson_schema_field_converter.py:147.- the 3-site widening collapses cleanly to one site in
convert_property_to_field, which is also the convergence point with #127. inline in_handle_array_type. - two of the three regression tests pass without the fix; only the nested one actually pins the bug. inline in
test_json_schema_integration.py. - explicit-null payloads are now accepted where they used to raise, an observable contract change with no test or release-note line. inline on the new test class.
- HTML report threshold extraction also misses Optional-wrapped fields; fine as a filed follow-up. inline near the array return.
nits (non-blocking):
test_json_schema_field_converter.py:111: theget_origin(req_type) is not typing.Unionassert is redundant right afterreq_type is str.- the test helper
_unwrap_optional(test_json_schema_field_converter.py:16-28) near-duplicatesStructuredModel._unwrap_optional(structured_model.py:1357); fine for test independence but a pointer comment would help future-us. test_export_roundtrip.py:234's "Optional nature is not preserved in round-trip" comment is now partially stale for the JSON-Schema direction.
on the #127 overlap you flagged: agreed it's real, and the single-site widening above is the concrete convergence point that makes both PRs compose cleanly.
appreciate it!
| # round-trips through from_json(...).model_dump(), which materializes | ||
| # the None default and re-validates it against the annotation. | ||
| field_type = NestedModel | ||
| if not is_required: |
There was a problem hiding this comment.
blocking: this widening leaks into annotation-based dispatch that doesn't understand Optional[NestedModel]. is_structured_field_type (configuration_helper.py:152-168) only scans the Union branch for List[...], so the elif inspect.isclass(annotation) arm never runs for a union and Optional[NestedModel] returns False. structured_list_comparator.py:264-266 uses that (via structured_model.py:647) to route sub-fields of list-of-objects items into _handle_hierarchical_field, so an optional object field inside an array item now takes the non-hierarchical path.
verified on this head vs base: a schema with people: array of {name (required), addr: object (optional)} through compare_recursive used to return fields.people.fields.addr with the full nested breakdown (fields.city, derived precision/recall/f1, raw_similarity_score); post-fix addr collapses to flat counts only. top-line scores are unchanged (dispatch there is runtime-isinstance), so nothing fails loudly, the detailed metric tree just silently degrades for any schema where an object inside an array item isn't in required, which is exactly the population #149 targets.
fix is small: add an Optional[StructuredModel] case to the Union branch of is_structured_field_type; _is_optional_structured_model at configuration_helper.py:386 already handles this exact shape. worth a regression test asserting compare_recursive keeps the nested fields breakdown for an optional nested object inside a list of objects.
| # None default is a valid value. See issue #149: the rich-value path | ||
| # round-trips through from_json(...).model_dump(), which materializes | ||
| # the None default and re-validates it against the annotation. | ||
| if not is_required: |
There was a problem hiding this comment.
so the fix is asymmetric: the stickler-config path still builds the exact broken shape. field_converter.py:54 resolves the annotation from the type string and field_converter.py:85-87 sets default = None for non-required fields without widening, so model_from_json(config) with "required": false still crashes from_json(process_rich_values=True) exactly as in #149. and since to_stickler_config emits "required": false for these fields (json_schema_field_converter.py:505-507), schema -> to_stickler_config -> model_from_json round-trips a fixed model back into a broken one.
ideally both paths get fixed together since they converge on the same ModelFactory; at minimum worth filing a follow-up issue so the round-trip hole is tracked.
| @@ -147,7 +147,14 @@ def convert_property_to_field( | |||
| default = property_schema.get("default", ... if is_required else None) | |||
There was a problem hiding this comment.
small gap in the widening condition: a field that is in required but carries an explicit "default": null keeps the bare annotation and the None default, and i verified from_json(..., process_rich_values=True) still raises the same ValidationError for that shape. the invariant being repaired is "a None default needs a nullable annotation", so the condition arguably wants to be not is_required or default is None rather than keying off required alone.
| # None default is a valid value. See issue #149: the rich-value path | ||
| # round-trips through from_json(...).model_dump(), which materializes | ||
| # the None default and re-validates it against the annotation. | ||
| if not is_required: |
There was a problem hiding this comment.
worth collapsing the three identical widening blocks into one site. convert_property_to_field (:118-133) is the sole dispatcher and convert_properties_to_fields (:88-91) its only caller, so wrapping once there is behavior-identical and deletes two of the three 6-line blocks (:151-156, :364-370, :432-437) plus the triplicated comment. it also gives #127 (which touches the same type-resolution lines for explicit-nullable) a single clean convergence point. Optional[Optional[T]] collapses, so double-wrapping during the refactor is benign.
| ) | ||
|
|
||
| return field_type, field |
There was a problem hiding this comment.
related consumer that the widening breaks, lower stakes than the comparison path: HTML report threshold extraction at data_extractors.py:199 (hasattr(field_type, '__fields__')) and :205 (__origin__ is list) both miss Optional[NestedModel] / Optional[List[NestedModel]], and the except at :212 swallows it, so nested field thresholds silently vanish from HTML reports for schema-built models. fine as a follow-up if filed.
| # The two paths must agree. | ||
| assert plain.model_dump() == rich.model_dump() | ||
|
|
||
| def test_optional_primitive_field_none_via_from_json_rich_values(self): |
There was a problem hiding this comment.
heads up that this test and the array one at :239 pass even without the fix (verified by running them against base): top-level omitted fields never re-validate their defaults, so only the nested test at :181 is a genuine pre-fix failure. as written they're annotation coverage, not regressions, and the docstrings overstate that. two options: nest them (an array with an optional inner field inside object items did fail pre-fix and is the untested real path through configuration_helper.py:92-113), or relabel them so the next reader doesn't assume they pin the bug.
| assert result["overall_score"] == 1.0 | ||
|
|
||
|
|
||
| class TestOptionalFieldNoneFromJson: |
There was a problem hiding this comment.
one externally observable behavior change with no coverage: M(**{"b": None}) for an optional {"type": "string"} field previously raised and now succeeds (verified). that's more permissive than JSON Schema semantics (null is invalid for "type": "string" regardless of required) and diverges from utils/json_schema_validator.py's jsonschema-based validation. i'd say it's defensible, even desirable here, since LLM predictions emit explicit nulls and those used to crash evaluation instead of scoring as FN/TN, but it deserves an explicit-null test through both construction paths and a release-note line. same for the model_json_schema() shape change (these fields now emit anyOf: [T, null]).
| ("opt_str", lambda t: t is str), | ||
| ( | ||
| "opt_obj", | ||
| lambda t: ( |
There was a problem hiding this comment.
nit: the __import__(...).StructuredModel in t.__mro__ lambda here is needlessly obscure; sibling tests in this file import StructuredModel directly and use issubclass.
|
Superseded by #159. I renamed the branch to follow a Thanks @adiadd, the review was spot on. All of it is addressed in #159:
Continuing the conversation on #159. |
2 participants
Issue #, if available: #149
Problem
from_json(process_rich_values=True)rejectsNonefor optional fields that plainModelClass(**data)accepts. A model built from a JSON Schema raisespydantic ValidationError("Input should be a valid string ... input_value=None") when an optional field is omitted and rich-value processing is on.Root cause
When a JSON-Schema field is optional (absent from
required),from_json_schema()builds a Pydantic field withdefault=Nonebut a non-nullable annotation (barestr, notOptional[str]). Plain construction tolerates the omitted field, but the rich-value path round-trips nested objects throughfrom_json(...).model_dump()(ConfigurationHelper._process_nested_structured_data).model_dump()materializes theNonedefault, and re-validation checks it against the bare annotation — which fails. The default-vs-annotation mismatch is the bug.Fix (3 sites)
In
json_schema_field_converter.py, widenfield_typetoOptional[field_type]whenever the field is not required, mirroring the existingdefault = ... if is_required else Nonedecision, at: primitive (convert_property_to_field), nested object (_handle_nested_object), and array (_handle_array_type). Required fields keep bare annotations.configuration_helper.pyalready handlesOptional[...]correctly — it just never received them.Testing
Added a regression test (
TestOptionalFieldNoneFromJson) covering nested/primitive/array optional fields, asserting bothM(**data)andM.from_json(data, process_rich_values=True)succeed and agree. Added a focused converter test asserting optional fields →Optional[...]and required fields stay bare. Updated existing converter assertions to unwrap the Optional wrapper for not-required fields only. Full suite: 1080 passed, 2 skipped (pre-existing skips), 0 failures.Risk
Low. Annotation-only change for not-required fields; required-field behavior is unchanged. Export round-trip and None-handling suites pass.
Fixes #149
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.