Fix: stop emitting null format/pattern for DateTime fields (#938)#1048
Open
jbbqqf wants to merge 1 commit into
Open
Fix: stop emitting null format/pattern for DateTime fields (#938)#1048jbbqqf wants to merge 1 commit into
jbbqqf wants to merge 1 commit into
Conversation
…ow-code#938) DateTime fields declared with the "rfc"/"rfc822" format or a custom strftime format string were rendered with `"format": null` and, for custom formats without a metadata pattern, `"pattern": null`. The OpenAPI 3 schema requires those keywords to be strings when present, so the resulting spec failed openapi-spec-validator. Pop the inherited `date-time` format from the accumulator dict for the non-iso branches and only emit `pattern` when the user supplied one via metadata. A regression test exercises the three affected combinations and runs the full spec through `validate_spec` so any future null leakage is caught immediately. Fixes marshmallow-code#938.
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.
Summary
MarshmallowPlugincurrently emits"format": null(and, in the custom-strftime case,"pattern": null) forDateTimefields that use any non-iso format. The OpenAPI 3 schema requires those keywords to be strings when present, so the rendered document does not validate againstopenapi-spec-validator.This PR makes the
datetime2propertiesattribute function drop the inheriteddate-timevalue from the accumulator rather than overwriting it withnull, and only emits"pattern"when the user actually supplied a metadata pattern.Fixes #938.
Context
PR #815 (released in v6.4) introduced
datetime2propertiesand explicitly returned"format": Noneto "override" the default("string", "date-time")mapping fromDEFAULT_FIELD_MAPPING. That answers the question @lafrech raised on the abandoned PR #937 ("any idea why it stopped working as expected in version 6.4?"): the regression is in1ac6f55itself. The same PR is also the source of the"pattern": Nonefor custom formats.The OpenAPI 3.0 schema (and openapi-spec-validator) rejects both of those keys when their value is
null:A previous attempt (#937) tried to give every non-iso format its own canonical
formatvalue. That triggered a long discussion that never converged. The minimal change in this PR avoids that debate entirely: it just stops emitting the invalidnullkeys, which is what reviewer @bhperry already pointed out in#937 (comment).Changes
src/apispec/ext/marshmallow/field_converter.py:datetime2propertiesnow receives the accumulator dictretand callsret.pop("format", None)for therfc/rfc822and custom-format branches. For custom formats,patternis only added when present infield.metadata. The function still returns the per-formatexample/pattern/minso existing behavior is otherwise preserved.tests/test_ext_marshmallow_field.py:test_datetime2property_rfc,test_datetime2property_rfc822,test_datetime2property_custom_format, andtest_datetime2property_custom_format_missing_regexto assert the absence of the keys instead ofNonevalues.test_datetime2property_emits_valid_openapi_spec, a regression test that builds a spec with all three previously-broken cases, asserts no key is set toNone, and runs the fullvalidate_spec()round-trip.CHANGELOG.rst: added a bug-fix entry underunreleased.AUTHORS.rst: added myself.Reproduce BEFORE/AFTER yourself (copy-paste)
Note the BEFORE prints
format: Nonebecause the key is literallyNonein the dict (and validation fails); AFTER printsNonebecause.get()falls through (the keys are not in the dict, and validation passes). The wider repro is in the new regression test, which differentiates the two cases properly.What I ran locally
I also verified that the new regression test (
test_datetime2property_emits_valid_openapi_spec) fails onorigin/devwhen only the source file is reverted:Edge cases
fields.DateTime(){type: string, format: date-time}iso/iso8601fields.DateTime(format="iso"){type: string, format: date-time}rfc/rfc822fields.DateTime(format="rfc"){type: string, format: null, example, pattern}{type: string, example, pattern}timestampfields.DateTime(format="timestamp"){type: number, format: float, example, min}timestamp_msfields.DateTime(format="timestamp_ms"){type: number, format: float, example, min}patternfields.DateTime(format=..., metadata={"pattern": ...}){type: string, format: null, pattern: <user>}{type: string, pattern: <user>}patternfields.DateTime(format=...){type: string, format: null, pattern: null}{type: string}Cases 1, 2, 4, and 5 already produced valid OpenAPI and are untouched. Cases 3, 6, and 7 are the ones the issue is about and are now schema-valid.
Risk / blast radius
The change is local to
datetime2properties(~20 lines). The function signature gains a default-valuedretkeyword (kept default-None) and remains callable asdatetime2properties(field), so any user code that calls it directly is unaffected. Custom attribute functions that previously inspected the returned dict forformat/patternset toNonewould need to update — but as far as I can tell,Nonewas never a useful sentinel for downstream code and the original intent was clearly "this isn't a real format".Datefields are unaffected because the function explicitly only triggers onmarshmallow.fields.DateTime(andDateis not aDateTimesubclass in marshmallow 3+).Release note
PR drafted with assistance from Claude Code (Anthropic). The change was reviewed manually against apispec's source — including the original commit
1ac6f55(PR #815) that introduced the null-emitting branches in v6.4 and the discussion on the abandoned PR #937 — and the reproducer above is the one I used during development; reviewers can paste it verbatim.