[core] keep property type when an allOf part redefines it without a type - #24586
[core] keep property type when an allOf part redefines it without a type#24586SubhamAshok wants to merge 4 commits into
Conversation
An allOf part that only adds constraints to an inherited property, for example 'nullable: true', replaced the typed schema wholesale and the property degraded to Object. Merge the constraints onto the existing typed schema instead. Fixes OpenAPITools#4128 for OpenAPI 3.x input. Swagger 2.0 input still degrades because the 2.0 to 3.0 converter itself injects 'type: object' before the generator runs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| Schema existing = targetProperties.get(name); | ||
| if (existing != null && incoming != null | ||
| && !ModelUtils.isAnyType(existing) && isConstraintOnlySchema(incoming)) { | ||
| Schema merged = ModelUtils.cloneSchema(existing, specVersionGreaterThanOrEqualTo310(openAPI)); |
There was a problem hiding this comment.
P2: The new merge only preserves a hand-picked subset of constraints. isConstraintOnlySchema() matches any typeless schema, so a part that only overrides e.g. maxLength, pattern, format, minimum/maximum or default on an inherited property enters the merge branch, but putProperty copies only nullable/description/deprecated/readOnly/writeOnly/extensions onto the clone — every other keyword is silently dropped while the type is kept. The generated model/validation will therefore be weaker than the OpenAPI spec declares (the constraint is lost instead of degraded). Consider either copying the remaining validation/format fields onto merged, or narrowing isConstraintOnlySchema to only the keywords that putProperty actually merges, so the Javadoc's 'constraints are applied' claim holds and no constraint silently disappears.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java, line 3667:
<comment>The new merge only preserves a hand-picked subset of constraints. isConstraintOnlySchema() matches any typeless schema, so a part that only overrides e.g. `maxLength`, `pattern`, `format`, `minimum`/`maximum` or `default` on an inherited property enters the merge branch, but putProperty copies only nullable/description/deprecated/readOnly/writeOnly/extensions onto the clone — every other keyword is silently dropped while the type is kept. The generated model/validation will therefore be weaker than the OpenAPI spec declares (the constraint is lost instead of degraded). Consider either copying the remaining validation/format fields onto merged, or narrowing isConstraintOnlySchema to only the keywords that putProperty actually merges, so the Javadoc's 'constraints are applied' claim holds and no constraint silently disappears.</comment>
<file context>
@@ -3642,13 +3642,67 @@ protected void addProperties(Map<String, Schema> properties, List<String> requir
+ Schema existing = targetProperties.get(name);
+ if (existing != null && incoming != null
+ && !ModelUtils.isAnyType(existing) && isConstraintOnlySchema(incoming)) {
+ Schema merged = ModelUtils.cloneSchema(existing, specVersionGreaterThanOrEqualTo310(openAPI));
+ if (incoming.getNullable() != null) {
+ merged.setNullable(incoming.getNullable());
</file context>
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| * True when the schema defines no type of its own: no type, no $ref, no items, | ||
| * no properties, no composition and no enum. | ||
| */ | ||
| private static boolean isConstraintOnlySchema(Schema schema) { |
There was a problem hiding this comment.
I would argue that both this method and the copy part of the one above are more suited for ModelUtils.
ModelUtils also already has copyMetadata, which is what we do above but to a smaller extend. So placing them next to each other is beneficial to highlight what different scenarios that exist with regards to transferring metadata related to different types of inheritance.
There was a problem hiding this comment.
Agreed, done in 9973312. New ModelUtils.copyConstraints sits next to copyMetadata and delegates to it for the shared subset. The put logic stays in DefaultCodegen since it needs the spec version for cloneSchema, but the transfer and the guard (now the existing isMetadataOnlySchema) live in ModelUtils.
Review feedback: the merge kept the type but only carried a few constraint fields. Copy the remaining validation, format and metadata keywords too, so no declared constraint is dropped. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Review feedback: reuse copyMetadata instead of a second hand-written copy list. New ModelUtils.copyConstraints delegates to copyMetadata and adds the validation keywords it does not cover. Guard now uses the existing isMetadataOnlySchema. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
4 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java:3670">
P2: Generated properties lose vendor extensions from the inherited schema whenever the constraint-only overlay has its own extension, because `copyConstraints` replaces rather than merges the extension map. Preserving the clone's extensions and overlaying incoming entries would retain both schemas' `x-*` metadata.</violation>
</file>
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java:2773">
P2: An allOf property overlay containing `not` is silently lost during the new merge. Because `isMetadataOnlySchema` does not classify `not` as a type-defining keyword, `putProperty` takes the merge branch, but `copyConstraints` never transfers `incoming.getNot()`, so the resulting property no longer enforces that constraint. Including `not` in the copied constraints (or excluding it from the metadata-only merge) would preserve the overlay semantics.</violation>
<violation number="2" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java:2774">
P2: Generated validation can be weakened when the incoming allOf constraint is looser than the inherited one because this merge overwrites existing bounds instead of intersecting them. Combining numeric, length, item, and property bounds using the most restrictive value would preserve the schema’s allOf semantics.</violation>
<violation number="3" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java:2775">
P2: A typeless allOf overlay using `const` loses its value constraint during merging because this copy routine never transfers `const`. Copying `const` (or classifying it as a replacement keyword) would keep generated schemas faithful to the input.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| && !ModelUtils.isAnyType(existing) | ||
| && ModelUtils.isMetadataOnlySchema(incoming) && incoming.getEnum() == null) { | ||
| Schema merged = ModelUtils.cloneSchema(existing, specVersionGreaterThanOrEqualTo310(openAPI)); | ||
| ModelUtils.copyConstraints(incoming, merged); |
There was a problem hiding this comment.
P2: Generated properties lose vendor extensions from the inherited schema whenever the constraint-only overlay has its own extension, because copyConstraints replaces rather than merges the extension map. Preserving the clone's extensions and overlaying incoming entries would retain both schemas' x-* metadata.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java, line 3670:
<comment>Generated properties lose vendor extensions from the inherited schema whenever the constraint-only overlay has its own extension, because `copyConstraints` replaces rather than merges the extension map. Preserving the clone's extensions and overlaying incoming entries would retain both schemas' `x-*` metadata.</comment>
<file context>
@@ -3664,103 +3664,16 @@ private void putProperties(Map<String, Schema> targetProperties, Map<String, Sch
- if (incoming.getExtensions() != null) {
- incoming.getExtensions().forEach((k, v) -> merged.addExtension(String.valueOf(k), v));
- }
+ ModelUtils.copyConstraints(incoming, merged);
targetProperties.put(name, merged);
} else {
</file context>
There was a problem hiding this comment.
Fixed in da0796a, extensions now merge per key, overlay wins on conflicts. Test asserts markers from both parts survive.
| */ | ||
| public static void copyConstraints(Schema from, Schema to) { | ||
| copyMetadata(from, to); | ||
| if (from.getFormat() != null) { |
There was a problem hiding this comment.
P2: A typeless allOf overlay using const loses its value constraint during merging because this copy routine never transfers const. Copying const (or classifying it as a replacement keyword) would keep generated schemas faithful to the input.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java, line 2775:
<comment>A typeless allOf overlay using `const` loses its value constraint during merging because this copy routine never transfers `const`. Copying `const` (or classifying it as a replacement keyword) would keep generated schemas faithful to the input.</comment>
<file context>
@@ -2762,6 +2762,48 @@ public static void copyMetadata(Schema from, Schema to) {
+ */
+ public static void copyConstraints(Schema from, Schema to) {
+ copyMetadata(from, to);
+ if (from.getFormat() != null) {
+ to.setFormat(from.getFormat());
+ }
</file context>
| if (from.getFormat() != null) { | |
| if (from.getConst() != null) { | |
| to.setConst(from.getConst()); | |
| } | |
| if (from.getFormat() != null) { |
There was a problem hiding this comment.
Fixed in da0796a. Overlays carrying const keep the old replace behavior, const reads as a value redefinition rather than a constraint.
| * @param to schema to copy to | ||
| */ | ||
| public static void copyConstraints(Schema from, Schema to) { | ||
| copyMetadata(from, to); |
There was a problem hiding this comment.
P2: Generated validation can be weakened when the incoming allOf constraint is looser than the inherited one because this merge overwrites existing bounds instead of intersecting them. Combining numeric, length, item, and property bounds using the most restrictive value would preserve the schema’s allOf semantics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java, line 2774:
<comment>Generated validation can be weakened when the incoming allOf constraint is looser than the inherited one because this merge overwrites existing bounds instead of intersecting them. Combining numeric, length, item, and property bounds using the most restrictive value would preserve the schema’s allOf semantics.</comment>
<file context>
@@ -2762,6 +2762,48 @@ public static void copyMetadata(Schema from, Schema to) {
+ * @param to schema to copy to
+ */
+ public static void copyConstraints(Schema from, Schema to) {
+ copyMetadata(from, to);
+ if (from.getFormat() != null) {
+ to.setFormat(from.getFormat());
</file context>
There was a problem hiding this comment.
I think this one is out of scope here. Intersecting bounds would need constraint resolution logic, and overlay-wins is the existing convention in this area (mergeProperties, the normalizer). Before this PR the inherited bounds were dropped entirely, so this is not a regression. Happy to open a follow-up issue for intersection semantics if maintainers want it.
| * @param from schema to copy from | ||
| * @param to schema to copy to | ||
| */ | ||
| public static void copyConstraints(Schema from, Schema to) { |
There was a problem hiding this comment.
P2: An allOf property overlay containing not is silently lost during the new merge. Because isMetadataOnlySchema does not classify not as a type-defining keyword, putProperty takes the merge branch, but copyConstraints never transfers incoming.getNot(), so the resulting property no longer enforces that constraint. Including not in the copied constraints (or excluding it from the metadata-only merge) would preserve the overlay semantics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java, line 2773:
<comment>An allOf property overlay containing `not` is silently lost during the new merge. Because `isMetadataOnlySchema` does not classify `not` as a type-defining keyword, `putProperty` takes the merge branch, but `copyConstraints` never transfers `incoming.getNot()`, so the resulting property no longer enforces that constraint. Including `not` in the copied constraints (or excluding it from the metadata-only merge) would preserve the overlay semantics.</comment>
<file context>
@@ -2762,6 +2762,48 @@ public static void copyMetadata(Schema from, Schema to) {
+ * @param from schema to copy from
+ * @param to schema to copy to
+ */
+ public static void copyConstraints(Schema from, Schema to) {
+ copyMetadata(from, to);
+ if (from.getFormat() != null) {
</file context>
There was a problem hiding this comment.
Same fix in da0796a, not is excluded from the merge, old replace behavior applies.
Review feedback. Extensions from both parts now survive, overlay wins per key. Overlays carrying const or not keep the old replace behavior. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2 participants
Relates to #4128, fixes its OpenAPI 3.x case.
An allOf part that only constrains an inherited property:
replaced the typed schema wholesale, so the field degraded to Object (
JsonNullable<Object>with openApiNullable).Fix: if the incoming property schema has no type of its own (no type, $ref, items, properties, composition, enum) and the existing one is typed, merge the constraints onto a clone of the existing schema instead of replacing it. Both merge points in DefaultCodegen.
Intentional overrides unchanged: a part that repeats a type still replaces. allOfDuplicatedProperties passes.
Swagger 2.0 input (the original report) still degrades: the 2.0 to 3.0 converter injects
type: objectbefore the generator runs. Parser issue, not fixable here.Tested: new DefaultCodegenTest case, full module test suite green, all 700+ sample configs regenerated across every generator family, zero output changes.
PR checklist
Summary by cubic
Keep property types when an
allOfpart redefines a property without a type, merging nullable/format/validation constraints and vendor extensions instead of replacing the schema. Fixes Object degradation for OpenAPI 3.x.allOfproperty overrides into the existing typed schema inDefaultCodegen, preserving the type while copying validation, format, metadata, and extensions.$ref,items,properties, composition,enum,const, ornot; explicit type repeats still override.ModelUtils.copyConstraintsand guarded withModelUtils.isMetadataOnlySchema; extensions are merged per key (overlay wins); updated merge paths to useputProperties/putProperty.testAllOfNullableWithoutTypeKeepsTypewith a new OAS 3.0 fixture; vendor extensions from both parts are kept; no sample outputs changed. Swagger 2.0 still degrades due to the converter injectingtype: object.Written for commit da0796a. Summary will update on new commits.