[BUG] Normalize OAS 3.1 nullable map type array under NORMALIZE_31SPEC - #24519
[BUG] Normalize OAS 3.1 nullable map type array under NORMALIZE_31SPEC#24519renechoi wants to merge 2 commits into
Conversation
A map schema that declares its own nullability with an OAS 3.1 type array (`type: ["null", object]` alongside `additionalProperties`) was never normalized. normalizeSchema strips the `null` entry for arrays and for schemas with properties, but the additionalProperties branch had no equivalent, so the schema kept both types, `nullable` was never set, and ModelUtils.getType() returned `null` first. Generators then resolved the property to a fictional `Null` / `ModelNull` model and dropped the import of the value schema, emitting output that does not compile. Extract the existing null-in-type-array handling into normalizeNullTypeArray31 and call it from the map branch as well. It stays narrower than processNormalize31Spec, which would replace a typeless JsonSchema with an empty schema and discard additionalProperties.
|
Reporter of #24517 here. Confirming against a production 3.1.1 spec (490 component schemas, 486 generated models) rather than just the minimal repro, in case a second data point helps review. The shape is what Swashbuckle emits for a nullable dictionary. Enabling (names elided; the shape is identical to the The flag looks safe to adopt as a prerequisite for this fix. Since the fix only takes effect under
No drift in nullability across 2507 properties, so the rule is not disruptive at scale on 3.1 input. Adjacent, and explicitly not blocking this PR — flagging only because it lives in the same code path and you may want it in the same pass or a separate issue: A property whose schema is a bare Happy to test a build of this branch against that spec if a real-world verification is useful before merge. |
|
Thanks — the Swashbuckle provenance is the useful part here. My repro is hand-written, so "an ordinary On the offer to test a build: yes please, if it isn't much trouble. Branch is
On the adjacent bare
|
| 7.16.0 | 7.17.0 → 7.24.0 | |
|---|---|---|
| typescript-axios | 'dyn'?: any | null;, no import |
'dyn'?: Null; + import type { Null } from './null' |
| java, webclient | JsonNullable<Object> dyn |
ModelNull dyn, no ModelNull.java |
So it isn't typescript-specific, and TypeScript is the quiet one — Java fails to compile, TS doesn't.
NORMALIZE_31SPEC is incidental to it. On 7.24.0 the output is byte-identical with and without the flag (diff -r clean). It's also byte-identical between 7.24.0 and this branch, so this PR neither causes nor fixes it — which matches your read that it isn't blocking.
Root cause, and why it's a different fix from this one. normalizeSchema grew this guard in 7.17.0 (absent at v7.16.0, present at v7.17.0), currently OpenAPINormalizer.java:940:
if (ModelUtils.isNullTypeSchema(openAPI, schema)) {
return schema;
}A bare type: "null" satisfies it, so the schema is handed straight back regardless of which rules are on. #23967 worked around this for map values by intercepting in the parent's map branch before recursing — which is why additionalProperties: {type: "null"} correctly yields { [key: string]: any | null; } today while the property form does not. The properties branch has no equivalent interception. This PR is about null inside a type array on a schema that has properties/additionalProperties; that path never reaches line 940.
Worth noting #23945 already pinned the map-value form at "7.17+" — same regression, and #23967 fixed one of its two surfaces.
On the silent-any part, I checked rather than assumed what Null degrades to:
const p: ParentBareNull = { dyn: 12345 };
const q: ParentBareNull = { dyn: 'a string instead' };
const callIt = (v: ParentBareNull) => v.dyn!.whateverMethod().deeply.nested;tsc --strict exits 0 on all of it, so the field is any wearing a type name.
Prior art: #23908 is the Java-side report, and #23961 proposes fixing it in AbstractJavaCodegen (open since June). Neither frames it at the normalizer level nor covers the TypeScript surface, and a per-generator fix leaves the other generators broken. I'll open it as its own issue with the bisect and the repro, and credit you for finding it — it wants #23967's shape of fix one level up. Happy to leave it to you instead if you'd rather file it.
|
Built First, a correction to my previous numbers. I reported "2507 nullable properties, unchanged with and without 1.
|
| without fix | with fix | |
|---|---|---|
| nullable property declarations | 2507 | 2513 |
| of those, map-typed | 0 | 6 |
| nullable declarations removed | — | 0 |
files importing the never-written Null |
5 | 2 |
Before the fix no nullable map in the whole client kept its | null — not one of six. Only one of those six was a hard compile error (the $ref case); the other five are primitive-valued maps that type-checked cleanly while silently dropping nullability. So this PR fixes a visible compile failure and a silent correctness bug on the same code path, and the silent one is the more common shape in this spec by 5 to 1.
Two suggestions from that, both minor:
- Worth saying so in the description. "Generators lose the import of the value schema" undersells it — nullability is lost on every nullable map,
$refor not. testNullableMapWithRefValueImportsReferencedModelasserts only the$refform. The normalizer-level test coversstringMap, but at the codegen level nothing pins the primitive-valued case, which is the silent half. A'someMap'?: { [key: string]: number; } | null;assertion would catch a future regression that keeps imports working while dropping| nullagain.
On the 7.24.0 comparison you asked for
I ran that too, and the drift concern turned out to be nil: v7.24.0 vs this branch differs in the same three files, so for this spec nothing moved between 7.24.0 and current master here. I used openapitools/openapi-generator-cli:latest (sha256:4ea782db…, also 7.25.0-SNAPSHOT) as the no-fix baseline so the diff isolates the fix rather than mixing in release drift. Both comparisons agree.
Also worth noting: the bug reproduces on current master, not only on the 7.24.0 release.
The bare type: "null" regression
Please do file it — the bisect to 7.17.0, the OpenAPINormalizer.java:940 early return, and the #23967 / #23945 / #23908 / #23961 prior art are the substance, and none of it is mine. Two things from this spec you are welcome to use: it occurs on 4 properties there, and after this PR it is what the 2 remaining Null imports are. Your v.dyn!.whateverMethod().deeply.nested check matches what I saw — it compiles, so nothing surfaces it. Tag me and I will add the measurements.
The build is still on disk, so if you push changes I can re-run both checks against the same spec cheaply.
The existing typescript-axios test only covers the $ref form of the nullable map, which is the form that fails to compile. The primitive-valued form was uncovered at the codegen level even though it is the more common shape: it compiles either way and only loses its `| null`. Replacing the fix with a $ref-only variant keeps testNullableMapWithRefValueImportsReferencedModel green, so that half needs its own assertion. Reported by rony-remedio while reviewing this PR against a production 3.1 spec.
|
Both checks against 486 real models are worth more than the samples run, thanks. Three things came out of it, one of them a correction back in your direction. Description rewrittenYou're right that "generators lose the import of the value schema" undersells it. It now separates the two consequences: nullability is dropped on every nullable map, and the missing import — hence the compile error — only happens when the value schema is a The primitive-valued testAdded in I wanted to know what it actually guards before claiming it guards anything, since the if (((Schema) schema.getAdditionalProperties()).get$ref() != null) {
normalizeNullTypeArray31(schema);
}
So at the codegen level it is the only guard on the quiet half, and the normalizer-level assertion is the only one below it. That's in the description now. One correction going the other way
There's nothing to adopt. That is also why your with/without table showed zero drift across 2507 properties: assuming both runs went through the CLI, the rule was already on in the "without" column, so there was nothing for the comparison to vary. The practical consequence is the opposite of a precondition — this change lands on every 3.1 spec by default, which is the part a reviewer should weigh. The bare
|
|
You're right, and I've confirmed it independently rather than just taking the source reading — the claim was mine to check.
My error was reading Two things follow, and both cut against what I said: My with/without table was vacuous. You spotted why: the rule was on in both columns, so there was nothing for the comparison to vary. "No drift across 2507 properties" measured nothing. Combined with the regex problem I already corrected, that table was wrong twice over — once in what it counted, once in what it was comparing. The remaining numbers in that comment (1 → 0 errors, 3 files, 2507 → 2513, 0 → 6 map-typed) come from generator-version comparisons, so they stand. The consequence is the opposite of what I implied. I framed the flag as a precondition consumers must adopt. In fact this lands on every 3.1 spec that goes through On |
Fixes #24517
Root cause
OpenAPINormalizer.normalizeSchemastrips the OAS 3.1"null"entry out of a type array for arrays (normalizeArraySchema->processNormalize31Spec) and, since #24139, for schemas withproperties. TheadditionalProperties(map) branch has no equivalent, so a map that declares its own nullability with a type array keeps both entries:nullableis therefore never set, andModelUtils.getType()returns the first entry of the type array, which is"null". That has two separate consequences:$ref, the property additionally loses that import and picks up an import of aNullmodel the generator never writes. That is what makes the output fail to compile.The first revision of this description mentioned only the second one ("generators resolve the property to a model that is never written and lose the import of the value schema"). That was the incomplete half. @rony-remedio measured the ratio on a production 3.1.1 spec (486 generated models): of its six nullable maps, one was the
$refform and five were primitive-valued — silently non-nullable, compiling cleanly, no symptom at all. The quiet failure is the common one; the compile error is the visible minority.This is also not specific to
typescript-axios— the cause is in the shared normalizer, so other generators produce uncompilable output from the same spec (javashown below).It is not a recent regression either. The fixture below generates identical
models/anddocs/on every release from 7.16.0 to 7.24.0 (typescript-axios, one jar per release). #24520 reaches the same never-generatedNullmodel through the baretype: "null"path and does bisect to 7.17.0; this one does not, so the map branch has simply never had the handling.Scope: not opt-in
NORMALIZE_31SPECis not a rule consumers have to enable.DefaultGeneratorsets it for every spec at 3.1.0 or above (DefaultGenerator.java:261-265),DefaultCodegen.getUseOpenapiNormalizer()returnstrue, and no generator overrides it. Measured rather than read off the source: generating the fixture below with 7.24.0 with and without--openapi-normalizer NORMALIZE_31SPEC=truegives byte-identical trees.So this change reaches every 3.1 spec by default, and cannot affect a 3.0 one.
Fix
Extracted the existing null-in-type-array handling from the
hasPropertiesbranch intonormalizeNullTypeArray31(Schema)and called it from the map branch as well.It deliberately stays narrower than
processNormalize31Spec: that method replaces aJsonSchemacarrying no explicit type with an empty schema, which would discardproperties/additionalProperties. That is the reason thehasPropertiesbranch never called it either, and the rationale now lives on the extracted method.Before / after
typescript-axios,withSeparateModelsAndApi=true, no normalizer flags (see above — the rule is on by default here).models/parent-nullable-map.ts, the$refform, which does not compile before:models/parent-nullable-primitive-map.ts, the quiet form, which compiles before and after:Type-checking the generated models with the command from the issue (
tsc --noEmit --strict --skipLibCheck --target es2020 --module esnext --moduleResolution bundler models/*.ts): before,error TS2304: Cannot find name 'Child'(exit 2); after, clean (exit 0).java(library=native),ParentNullableMap.java, from the same spec:The generated docs improve on the same specs as a side effect:
docs/ParentNullableMap.mdlinked the property to(.md)(an empty target) and now links to(Child.md), and the primitive-valued one drops the empty link entirely.Tests
New fixture
modules/openapi-generator/src/test/resources/3_1/issue_24517.yaml: the minimal spec from the issue, its nullable-array control, and a primitive-valued nullable map for the quiet form.OpenAPINormalizerTest.testIssue24517NullableMapWithRefValueUsingTypeArray— the map normalizes to a nullableobjectand keeps its$refvalue schema; the nullable array is asserted alongside as the control that already worked.TypeScriptAxiosClientCodegenTest.testNullableMapWithRefValueImportsReferencedModel— end to end: the generated model importsChild, the property is| null, and there is no import from./null.TypeScriptAxiosClientCodegenTest.testNullableMapWithPrimitiveValueKeepsNullability— end to end for the quiet form:'counts'?: { [key: string]: number; } | null;and no dangling./nullimport.OpenAPINormalizerTest.testOpenAPINormalizer31SpecNullMapAdditionalProperties— extended.3_1/issue_23945.yamlalready declared its maps astype: [object, "null"], but only the value schemas were asserted, so the map-level half of the same bug was uncovered.All four fail on master and pass with this change.
The last two are not redundant with each other. Replacing the fix with a
$ref-only variant —if (((Schema) schema.getAdditionalProperties()).get$ref() != null)around the new call, which is what a fix aimed only at the missing import would look like — leavestestNullableMapWithRefValueImportsReferencedModelgreen and failstestNullableMapWithPrimitiveValueKeepsNullabilityandtestOpenAPINormalizer31SpecNullMapAdditionalProperties. So at the codegen level the primitive-valued test is the only guard on the quiet half.Samples
No sample changed. Of the 246 distinct
inputSpecfiles referenced frombin/configs/**, none declares a schema carrying both anull-containing type array and anadditionalPropertiesschema, so the change cannot alter existing output. Verified rather than assumed: regenerating the 21 configs that use a 3.1 spec plus the 15typescript-axiosconfigs (36 generators) leavessamples/clean../mvnw -pl modules/openapi-generator testpasses locally on JDK 21 (4608 tests, 0 failures, 7 skipped).cc @nicokoenig (TypeScript / Axios)
PR checklist
master.Summary by cubic
Fix normalization for OAS 3.1 nullable map schemas (
type: ["null", object]) underNORMALIZE_31SPEC. Maps are now correctly marked nullable and keep their value schema, so generated code compiles.normalizeNullTypeArray31and applied it to map schemas to move"null"from the type array tonullable: true, and settype: "object"when appropriate.additionalPropertiesintact (including$refvalue schemas).typescript-axiosandjavafrom resolving to fakeNull/ModelNullmodels and dropping imports.$refmaps, plus a newtypescript-axiostest guarding primitive map values to keep| null. Minimal 3.1 fixture added; no sample changes.Written for commit b1f4c29. Summary will update on new commits.