Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 34 files
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="samples/server/others/kotlin-springboot/allOf-multilevel/gradle/wrapper/gradle-wrapper.properties">
<violation number="1" location="samples/server/others/kotlin-springboot/allOf-multilevel/gradle/wrapper/gradle-wrapper.properties:3">
P1: Gradle wrapper pinned to 8.1.1 is too old for JDK 21</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -0,0 +1,7 @@ | |||
| distributionBase=GRADLE_USER_HOME | |||
| distributionPath=wrapper/dists | |||
| distributionUrl=https\://services.gradle.org/distributions/gradle-8.1.1-bin.zip | |||
There was a problem hiding this comment.
P1: Gradle wrapper pinned to 8.1.1 is too old for JDK 21
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/others/kotlin-springboot/allOf-multilevel/gradle/wrapper/gradle-wrapper.properties, line 3:
<comment>Gradle wrapper pinned to 8.1.1 is too old for JDK 21</comment>
<file context>
@@ -0,0 +1,7 @@
+distributionBase=GRADLE_USER_HOME
+distributionPath=wrapper/dists
+distributionUrl=https\://services.gradle.org/distributions/gradle-8.1.1-bin.zip
+networkTimeout=10000
+validateDistributionUrl=true
</file context>
| distributionUrl=https\://services.gradle.org/distributions/gradle-8.1.1-bin.zip | |
| distributionUrl=https\://services.gradle.org/distributions/gradle-8.14-bin.zip |
There was a problem hiding this comment.
[Claude] Not fixing this one — the pin is correct here, and hand-editing it would break the build. Three reasons:
-
This file is generated output. It comes from
kotlin-spring/libraries/spring-boot/gradle-wrapper.properties.mustache, so the next./bin/generate-samples.shrun reverts any manual edit and the samples CI job then fails on a dirty tree. -
8.1.1 is deliberate, not stale. The template line is:
{{#useSpringBoot4}}gradle-8.14.5{{/useSpringBoot4}}{{^useSpringBoot4}}gradle-8.1.1{{/useSpringBoot4}}
This sample uses
useSpringBoot3, so 8.1.1 is the correct branch. Every non-SpringBoot4 kotlin-spring sample in the repo pins the same version; onlykotlin-spring-cloud-4has 8.14.5. Hardcoding 8.14 would make this the one inconsistent sample. -
CI never uses the committed pin. Both
samples-kotlin-server-jdk17andjdk21install Gradle 8.14 and runarguments: wrapperin the sample directory before building, which rewritesgradle-wrapper.properties, and only then run./gradlew build -x test. So the jdk21 job builds with Gradle 8.14 on Java 21 and the incompatibility never occurs.
The underlying concern is real in exactly one situation: running ./gradlew directly on a JDK 21 machine without that wrapper step. I hit that locally and worked around it by temporarily bumping to 8.14 to compile the sample. But that affects every non-SpringBoot4 kotlin-spring sample equally and is not introduced by this PR — a fix belongs in the three gradle-wrapper.properties.mustache templates, with all kotlin-spring samples regenerated, as its own change.
|
@wing328, could you help get this reviewed? |
|
@wing328 , could you help review this PR, and the following? A couple are alternatives to each other, as outlined in the description.
Thanks! |
69198b6 to
02ec314
Compare
…#18206) A schema that both inherits via allOf and is itself inherited from was rendered as a `data class`, which Kotlin cannot subclass, so the generated code did not compile. Such a mid-level model is now rendered as an `open class`: its own properties are marked `open`, and its children emit a parent constructor call. Because `open class` gives up the compiler-generated data class members, the template generates equals/hashCode/toString/copy from allVars. Destructuring (componentN) is not generated and is a known limitation of this rendering. Adds the allOf-multilevel sample (Animal <- Dog <- BigDog) to the kotlin-server sample workflows so the generated code is compiled in CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBn7VWUxZL6mTUbg21xNjm
02ec314 to
16acf5d
Compare
There was a problem hiding this comment.
4 issues found across 28 files
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="samples/server/others/kotlin-springboot/allOf-multilevel/README.md">
<violation number="1" location="samples/server/others/kotlin-springboot/allOf-multilevel/README.md:7">
P3: A Gradle wrapper is checked in, so this sentence incorrectly tells users they need Gradle installed. State that the Gradle wrapper is included and only the Maven wrapper is absent.</violation>
<violation number="2" location="samples/server/others/kotlin-springboot/allOf-multilevel/README.md:13">
P3: The run instructions can't work for this sample: the pom.xml declares only maven-source-plugin and kotlin-maven-plugin (no `spring-boot-maven-plugin`, unlike `samples/server/petstore/kotlin-springboot/pom.xml`), `build.gradle.kts` sets `tasks.bootJar { enabled = false }`, and the sample has no `@SpringBootApplication` main class. The produced jar has no Main-Class, so `java -jar target/openapi-spring-1.0.0.jar` (and the corresponding gradle command) fails with "no main manifest attribute". Either make the sample runnable (add the Spring Boot plugin plus a main class) or replace these instructions with build/compile-only steps.</violation>
</file>
<file name="modules/openapi-generator/src/main/resources/kotlin-spring/dataClass.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/kotlin-spring/dataClass.mustache:69">
P1: `copy` does not mirror the constructor property type: inline enums use `Class.Enum`, required nullable fields use `T?`, and `openApiNullable` fields use `JsonNullable<T>`. These mismatches make generated mid-level models fail Kotlin compilation; render the same type branches as the property partials.</violation>
</file>
<file name="samples/server/others/kotlin-springboot/allOf-multilevel/build.gradle.kts">
<violation number="1" location="samples/server/others/kotlin-springboot/allOf-multilevel/build.gradle.kts:20">
P1: Gradle Kotlin DSL requires `plugins {}` before other script statements, so this placement makes the sample fail during build configuration. Move the block immediately after the import.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| fun copy( | ||
| {{#vendorExtensions.x-open-class-vars}} | ||
| {{{name}}}: {{{dataType}}}{{^required}}?{{/required}} = this.{{{name}}}{{^-last}},{{/-last}} |
There was a problem hiding this comment.
P1: copy does not mirror the constructor property type: inline enums use Class.Enum, required nullable fields use T?, and openApiNullable fields use JsonNullable<T>. These mismatches make generated mid-level models fail Kotlin compilation; render the same type branches as the property partials.
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/resources/kotlin-spring/dataClass.mustache, line 69:
<comment>`copy` does not mirror the constructor property type: inline enums use `Class.Enum`, required nullable fields use `T?`, and `openApiNullable` fields use `JsonNullable<T>`. These mismatches make generated mid-level models fail Kotlin compilation; render the same type branches as the property partials.</comment>
<file context>
@@ -43,6 +43,33 @@
+
+ fun copy(
+{{#vendorExtensions.x-open-class-vars}}
+ {{{name}}}: {{{dataType}}}{{^required}}?{{/required}} = this.{{{name}}}{{^-last}},{{/-last}}
+{{/vendorExtensions.x-open-class-vars}}
+ ): {{classname}} = {{classname}}({{#vendorExtensions.x-open-class-vars}}{{{name}}} = {{{name}}}{{^-last}}, {{/-last}}{{/vendorExtensions.x-open-class-vars}})
</file context>
| {{{name}}}: {{{dataType}}}{{^required}}?{{/required}} = this.{{{name}}}{{^-last}},{{/-last}} | |
| {{{name}}}: {{#vendorExtensions.x-is-jackson-optional-nullable}}JsonNullable<{{#isEnum}}{{#isArray}}{{baseType}}<{{/isArray}}{{classname}}.{{{nameInPascalCase}}}{{#isArray}}>{{/isArray}}{{/isEnum}}{{^isEnum}}{{{dataType}}}{{/isEnum}}>{{/vendorExtensions.x-is-jackson-optional-nullable}}{{^vendorExtensions.x-is-jackson-optional-nullable}}{{#isEnum}}{{#isArray}}{{baseType}}<{{/isArray}}{{classname}}.{{{nameInPascalCase}}}{{#isArray}}>{{/isArray}}{{/isEnum}}{{^isEnum}}{{{dataType}}}{{/isEnum}}{{#isNullable}}?{{/isNullable}}{{^required}}{{^isNullable}}?{{/isNullable}}{{/required}}{{/vendorExtensions.x-is-jackson-optional-nullable}} = this.{{{name}}}{{^-last}},{{/-last}} |
| enabled = false | ||
| } | ||
|
|
||
| plugins { |
There was a problem hiding this comment.
P1: Gradle Kotlin DSL requires plugins {} before other script statements, so this placement makes the sample fail during build configuration. Move the block immediately after the import.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/others/kotlin-springboot/allOf-multilevel/build.gradle.kts, line 20:
<comment>Gradle Kotlin DSL requires `plugins {}` before other script statements, so this placement makes the sample fail during build configuration. Move the block immediately after the import.</comment>
<file context>
@@ -0,0 +1,49 @@
+ enabled = false
+}
+
+plugins {
+ val kotlinVersion = "1.9.25"
+ id("org.jetbrains.kotlin.jvm") version kotlinVersion
</file context>
|
|
||
| ## Getting Started | ||
|
|
||
| This document assumes you have either maven or gradle available, either via the wrapper or otherwise. This does not come with a gradle / maven wrapper checked in. |
There was a problem hiding this comment.
P3: A Gradle wrapper is checked in, so this sentence incorrectly tells users they need Gradle installed. State that the Gradle wrapper is included and only the Maven wrapper is absent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/others/kotlin-springboot/allOf-multilevel/README.md, line 7:
<comment>A Gradle wrapper is checked in, so this sentence incorrectly tells users they need Gradle installed. State that the Gradle wrapper is included and only the Maven wrapper is absent.</comment>
<file context>
@@ -0,0 +1,21 @@
+
+## Getting Started
+
+This document assumes you have either maven or gradle available, either via the wrapper or otherwise. This does not come with a gradle / maven wrapper checked in.
+
+By default a [`pom.xml`](pom.xml) file will be generated. If you specified `gradleBuildFile=true` when generating this project, a `build.gradle.kts` will also be generated. Note this uses [Gradle Kotlin DSL](https://github.com/gradle/kotlin-dsl).
</file context>
| This document assumes you have either maven or gradle available, either via the wrapper or otherwise. This does not come with a gradle / maven wrapper checked in. | |
| This document assumes you have Maven available or use the checked-in Gradle wrapper; no Maven wrapper is included. |
|
|
||
| To build the project using maven, run: | ||
| ```bash | ||
| mvn package && java -jar target/openapi-spring-1.0.0.jar |
There was a problem hiding this comment.
P3: The run instructions can't work for this sample: the pom.xml declares only maven-source-plugin and kotlin-maven-plugin (no spring-boot-maven-plugin, unlike samples/server/petstore/kotlin-springboot/pom.xml), build.gradle.kts sets tasks.bootJar { enabled = false }, and the sample has no @SpringBootApplication main class. The produced jar has no Main-Class, so java -jar target/openapi-spring-1.0.0.jar (and the corresponding gradle command) fails with "no main manifest attribute". Either make the sample runnable (add the Spring Boot plugin plus a main class) or replace these instructions with build/compile-only steps.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/others/kotlin-springboot/allOf-multilevel/README.md, line 13:
<comment>The run instructions can't work for this sample: the pom.xml declares only maven-source-plugin and kotlin-maven-plugin (no `spring-boot-maven-plugin`, unlike `samples/server/petstore/kotlin-springboot/pom.xml`), `build.gradle.kts` sets `tasks.bootJar { enabled = false }`, and the sample has no `@SpringBootApplication` main class. The produced jar has no Main-Class, so `java -jar target/openapi-spring-1.0.0.jar` (and the corresponding gradle command) fails with "no main manifest attribute". Either make the sample runnable (add the Spring Boot plugin plus a main class) or replace these instructions with build/compile-only steps.</comment>
<file context>
@@ -0,0 +1,21 @@
+
+To build the project using maven, run:
+```bash
+mvn package && java -jar target/openapi-spring-1.0.0.jar
+```
+
</file context>
|
tested locally and the result is good. let's give it a try thanks for the PR |
2 participants
Fixes #18206.
The bug
A schema that both inherits via
allOfand is itself inherited from was rendered as adata class. Kotlin cannot subclass a data class, so the generated code did not compile at all:The fix
Such a "mid-level" model — has a parent, has children, is not itself a discriminator root — is now rendered as an
open class, its own properties markedopen, and its children emit a parent constructor call:open classloses the data class members, so the template writes themAn
open classgets no compiler-generatedequals/hashCode/toString/copy, sodataClass.mustachegenerates them. Details worth a reviewer's eye:equalsuses an exact-class check (other?.javaClass != javaClass) rather thanis Dog. This keeps symmetry with the leaf data class:Dog(...) != BigDog(...)in both directions.copy()bind to the same properties the constructor takes them for, matching how sibling data classes behave.componentN()is not generated — destructuring is unavailable on mid-level models. Known limitation of this rendering.Known trade-off:
copy()slices subclassesDog.copy()returns aDog. Called through aDog-typed reference that actually holds aBigDog, it returns a plainDogand dropsBigDog's own fields:BigDog.copy()(the data class one, different arity) is unaffected, and the two never collide on the JVM. Vanilla Kotlin open classes have nocopy()at all, so this is a member the generator introduces rather than one it preserves.Maintainer decision: keep
copy()for parity with the data classes around it, or drop it fromopen classmodels to avoid the slicing footgun entirely? I lean toward keeping it — an ergonomiccopy()that behaves like every other generated model is worth more than the narrow parent-typed-reference case — but this is a legitimate design call and I am happy to remove it.Why
open classand not something elseKotlin makes this a forced choice: a
data classis final and cannot be a superclass, so a mid-level model must give up exactly one of three things.open class(this PR)ImplBigDog is DogrelationshipFlattening is what Swift, Rust and most Scala generators do (
supportsInheritance = false). It would make every model adata classagain with no hand-written members, but it drops the subtype relationship — and it is precisely what the reporters on #18206 are asking to escape ("without inheritance ... we must write a lot of redundant code"). It is also a per-generator flag shared by every Kotlin generator, so it could not be scoped to this case.Interface +
DogImplwould fix every downside here: all concrete types stay data classes, socopy()/destructuring/equalscome free and nothing can slice. It was rejected on source stability —Dogwould stop being constructible, so merely adding an unrelatedBigDogschema to a spec would break every existingDog(...)call site. Withopen class, that same spec change leavesDog(className = ..., breed = ...)compiling unchanged; onlycopy()/destructuring semantics shift.Worth noting the generator already picks the interface strategy where it is free: when a mid-level model has its own discriminator it renders as
interface Dog : Animal, andBigDog : Dogneeds no open class at all. That path is untouched here. The rule ends up coherent — interface when the type is abstract by nature,open classwhen it is concrete — and this PR only addresses the concrete case.Property names that are not Kotlin identifiers
The generated members emit property names unescaped and build
toString()by concatenation rather than string interpolation. Both matter for back-ticked names:`would not compile, and$2ndField`` is not valid Kotlin interpolation (it needs${...}). The `toString()` label also drops the back-ticks, matching what a `data class` prints. Covered by `testMultiLevelAllOfWithEscapedPropertyNames` against a dedicated spec.Depth
Chains deeper than three levels work: each mid-level model is
open, and each child emits a constructor call carrying its parent's full parameter list in order. Verified onA <- B <- C <- D(two stacked open classes) — generates and compiles.Unrelated pre-existing issue spotted
@paramlines in the generated KDoc HTML-escape back-ticked names (@param `2ndField`). This affects plaindata classmodels too and is not introduced here, so it is left alone — happy to file it separately.Verification
KotlinSpringServerCodegenTest: 270 passing, 2 new — one covering the whole shape —open classfor the mid-level model, leaf stays adata class, parent constructor call is emitted, all four generated members present, andcopy()'s parameter order matches the constructor; the other covering property names that need back-ticks.allOf-multilevelsample added to thesamples-kotlin-server-jdk17/jdk21workflows, so CI builds it. Verified locally withgradlew compileKotlin→BUILD SUCCESSFUL.For maintainers
KotlinSpringServerCodegenTest.java(both-added), and line 17 ofdataClass.mustacheif [kotlin-spring] seal allOf discriminator parent interfaces by default #23953 lands first — that PR edits the{{#discriminator}}branch of the same line while this one edits the{{^discriminator}}branch.allOf-multilevelis committed as this PR alone produces it:interface Animal(notsealed, that is [kotlin-spring] seal allOf discriminator parent interfaces by default #23953) and no discriminator default onclassName(that is [kotlin-spring] allOf discriminator children get default discriminator value #23952). Merging either of those changes this sample's output, so it will need a regeneration — the test assertions here were written to stay valid either way.copy()on open classes — see the trade-off above. The one design question in this PR.@paramlines HTML-escape back-ticked propertynames (
@param `2ndField`). This affects plaindata classmodels too, so it is out ofscope for this PR — happy to open a separate issue or PR if you would like it fixed.
🤖 Generated with Claude Code
https://claude.ai/code/session_01VBn7VWUxZL6mTUbg21xNjm