Skip to content

Fix kotlin-spring required property validation#23922

Open
ko4lax wants to merge 4 commits into
OpenAPITools:masterfrom
ko4lax:fix-kotlin-spring-required-validation
Open

Fix kotlin-spring required property validation#23922
ko4lax wants to merge 4 commits into
OpenAPITools:masterfrom
ko4lax:fix-kotlin-spring-required-validation

Conversation

@ko4lax
Copy link
Copy Markdown

@ko4lax ko4lax commented Jun 2, 2026

Required kotlin-spring properties now generate as nullable fields with @field:NotNull so Bean Validation can surface missing values instead of Jackson failing at constructor time. Includes regression coverage for the required-nullable 4-state test case.


Summary by cubic

Fixes kotlin-spring required property handling to avoid early Jackson failures while keeping the default model contract. By default, required fields stay non‑nullable and do not get @field:NotNull; enabling nullableRequiredForBeanValidation makes them nullable and adds @field:NotNull so Bean Validation catches missing values.

  • Bug Fixes

    • With useBeanValidation, required properties keep @JsonProperty(required = true) and remain non‑nullable without @NotNull by default.
    • Added nullableRequiredForBeanValidation (default false) to emit ? and @field:NotNull for required fields; updated docs and tests for the four required/nullable scenarios, including the opt‑in path.
  • New Features

    • Added a Spring Boot 3 Petstore compilation sample and config (kotlin-springboot-3-nullable-required) to exercise nullableRequiredForBeanValidation with useBeanValidation=true, verifying nullable types plus @field:NotNull compile correctly.

Written for commit 67289ca. Summary will update on new commits.

Review in cubic

Copilot AI review requested due to automatic review settings June 2, 2026 13:15
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR changes the kotlin-spring generator to make required properties nullable in their Kotlin type when bean validation is enabled, and adds an unconditional @field:NotNull annotation. The intent is to let Spring's bean validation catch missing required JSON fields rather than failing earlier at Jackson deserialization.

Changes:

  • Always emit @field:NotNull for required vars when useBeanValidation is enabled.
  • Force the Kotlin type of a required var to be nullable (?) whenever useBeanValidation is enabled, regardless of the OpenAPI nullable flag.
  • Updated the two corresponding test scenarios to assert the new nullable-with-@NotNull shape.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
modules/openapi-generator/src/main/resources/kotlin-spring/dataClassReqVar.mustache Adds @field:NotNull and forces nullable type for required vars when bean validation is enabled.
modules/openapi-generator/src/test/java/org/openapitools/codegen/kotlin/spring/KotlinSpringServerCodegenTest.java Updates scenarios 1 and 2 to expect the new nullable type plus @field:NotNull annotation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 2 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

@Picazsoo
Copy link
Copy Markdown
Contributor

Picazsoo commented Jun 3, 2026

This seems like a pretty significant trade off. I would definitely prefer if it was possible to use bean validations together with non-nullable types. Maybe it would be really better to handle it via some nullableRequiredForBeanValidation as is proposed. Because I can definitely tell that for me MismatchedInputException with a custom converter in @ControllerAdvice is preferred to having nulls suddenly all over the place.

Or maybe a different way could be to use a traditional class instead of data class in such case and have a constructor with nullable parameters and instantiating into private _fields and having non null getters with requireNonNull

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 4 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread docs/generators/kotlin-spring.md
@ko4lax ko4lax force-pushed the fix-kotlin-spring-required-validation branch from 1cf42f9 to 4c588b7 Compare June 4, 2026 04:22
@Picazsoo
Copy link
Copy Markdown
Contributor

Picazsoo commented Jun 5, 2026

I would recommend also adding a compilation sample as it is generally more thorough than just depending on the unit tests. I think it is not necessarily needed to create entirely new sample (as that increases the CI/CD pipelines runtime) - you might be able to find some existing config that tests some unrelated functionality and can therefore basically test your functionality side-by-side.

Adds a kotlin-spring compilation sample that exercises the new
nullableRequiredForBeanValidation flag with useBeanValidation=true.
Reuses the existing petstore.yaml input spec to keep CI impact minimal.

The generated sample compiles successfully with ./gradlew compileKotlin,
verifying that required fields produce valid Kotlin code with @field:NotNull
annotations and nullable types.
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 35 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="samples/server/petstore/kotlin-springboot-3-nullable-required/src/test/kotlin/org/openapitools/api/UserApiTest.kt">

<violation number="1" location="samples/server/petstore/kotlin-springboot-3-nullable-required/src/test/kotlin/org/openapitools/api/UserApiTest.kt:20">
P1: All tests are non-functional because they use `TODO()` placeholders that throw at runtime, so this file provides no executable regression coverage.</violation>
</file>

<file name="samples/server/petstore/kotlin-springboot-3-nullable-required/src/test/kotlin/org/openapitools/api/StoreApiTest.kt">

<violation number="1" location="samples/server/petstore/kotlin-springboot-3-nullable-required/src/test/kotlin/org/openapitools/api/StoreApiTest.kt:20">
P1: Test inputs are initialized with `TODO()`, so the tests throw `NotImplementedError` at runtime and cannot serve as regression coverage.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

@@ -0,0 +1,140 @@
package org.openapitools.api
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: All tests are non-functional because they use TODO() placeholders that throw at runtime, so this file provides no executable regression coverage.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/petstore/kotlin-springboot-3-nullable-required/src/test/kotlin/org/openapitools/api/UserApiTest.kt, line 20:

<comment>All tests are non-functional because they use `TODO()` placeholders that throw at runtime, so this file provides no executable regression coverage.</comment>

<file context>
@@ -0,0 +1,140 @@
+     */
+    @Test
+    fun createUserTest() {
+        val user: User = TODO()
+        
+        
</file context>

@@ -0,0 +1,74 @@
package org.openapitools.api
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Test inputs are initialized with TODO(), so the tests throw NotImplementedError at runtime and cannot serve as regression coverage.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/petstore/kotlin-springboot-3-nullable-required/src/test/kotlin/org/openapitools/api/StoreApiTest.kt, line 20:

<comment>Test inputs are initialized with `TODO()`, so the tests throw `NotImplementedError` at runtime and cannot serve as regression coverage.</comment>

<file context>
@@ -0,0 +1,74 @@
+     */
+    @Test
+    fun deleteOrderTest() {
+        val orderId: kotlin.String = TODO()
+        
+        
</file context>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants