fix(json-schema): annotate optional fields as Optional[T] so from_json accepts None#159
Open
valter-silva-au wants to merge 2 commits into
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
… across paths Addresses @adiadd's review on awslabs#150. The Optional[T] widening for non-required fields was correct but incomplete -- several consumers and a parallel producer didn't understand the widened annotation: - comparison: is_structured_field_type now recognizes Optional[StructuredModel], so compare_recursive keeps the nested metric breakdown for optional nested objects inside list items (was silently collapsing to flat counts) - widening condition is now `not is_required or default is None`, so a required field with an explicit "default": null is also widened - collapse the three widening sites into the sole dispatcher convert_property_to_field (also the convergence point with awslabs#127) - stickler-config path (field_converter.py) widens too, closing the to_stickler_config -> model_from_json round-trip hole - HTML report threshold extraction unwraps Optional[...] so nested thresholds no longer vanish for schema-built optional models - document the explicit-null contract (anyOf:[T,null]); nest the two regression tests that previously passed without the fix; tidy review nits
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 2 HotspotsFiles with the highest number of security findings:
Detailed FindingsShow 2 actionable findingsFinding 1: CKV2_GHA_1
Description: Finding 2: B102
Description: Code Snippet: Report generated by Automated Security Helper (ASH) at 2026-06-27T08:27:00+00:00 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
The core fix widens an optional field's annotation to
Optional[T]so theNonedefault is valid. Following @adiadd's review on #150, the widening is made consistent across every producer and consumer of the annotation:json_schema_field_converter.pycollapse into the sole dispatcherconvert_property_to_field— the convergence point with fix: support JSON Schema list-form nullable types #127. The condition isnot is_required or default is None, so a required field that declares an explicit"default": nullis also widened (the invariant is "a None default needs a nullable annotation").ConfigurationHelper.is_structured_field_typenow recognizesOptional[StructuredModel](reusing the existing_is_optional_structured_model). Without this, an optional nested object inside a list-of-objects item was routed down the non-hierarchical path and silently lost its nested metric breakdown incompare_recursive— exactly the population [BUG] from_json(process_rich_values=True) rejects None for optional fields that plain ModelClass(**data) accepts #149 targets.field_converter.py(used bymodel_from_json) widens too, closing a round-trip hole: schema →to_stickler_config→model_from_jsonpreviously rebuilt a fixed model back into a broken one.data_extractors.pythreshold extraction unwrapsOptional[...]so nested field thresholds no longer vanish from HTML reports for schema-built optional models.Required fields keep bare annotations.
Contract note (intentional, documented)
Widening optional fields to
Optional[T]means an explicitnullis now accepted for an optional typed field (e.g.{"note": null}for an optionalstring), where it previously raised, andmodel_json_schema()emitsanyOf: [T, null]for such fields. This is deliberate — model predictions emit explicit nulls and should score as a value rather than crash evaluation. It is documented indocs/docs/Advanced/dynamic-models.mdand pinned by tests.Testing
TestOptionalFieldNoneFromJson— nested/primitive/array optional fields round-trip through bothM(**data)andM.from_json(data, process_rich_values=True). The primitive/array cases are nested one level down so they exercise the real re-validation path (the previous top-level versions passed even without the fix; per @adiadd they did not pin the bug). Includes a required-field-with-"default": nullcase.TestOptionalNestedBreakdownPreserved—compare_recursivekeeps the nestedfieldsbreakdown for an optional nested object inside a list of objects.TestOptionalFieldExplicitNullContract— explicit-null acceptance through both construction paths + theanyOf:[T,null]schema shape.test_configuration_helper_optional_structured.py,test_field_converter_optional_roundtrip.py,test_data_extractors_optional_thresholds.pyfor the comparison, stickler-config round-trip, and HTML-threshold fixes respectively.__import__lambda, stale round-trip comment, helper pointer).Full suite: 1443 passed, 2 skipped (pre-existing skips), 0 failures.
ruff checkclean.Risk
Low–medium. The changes are additive — they teach consumers to recognize a shape (
Optional[T]) that was already half-handled elsewhere, and bring annotations into agreement with theNonedefaults that were already being assigned. The single-site refactor is behavior-identical (verified by the existing converter tests).By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.