Required field in Schema annotation ignored in Kotlin

[k96payne]

Describe the bug

• Setting the required field to false in the @Schema annotation on a field in Kotlin gets ignored when the field is non-nullable

To Reproduce
Steps to reproduce the behavior:

• What version of spring-boot you are using? 2.5.12

• What modules and versions of springdoc-openapi are you using?
  implementation("org.springdoc:springdoc-openapi-webmvc-core:1.6.14")
implementation("org.springdoc:springdoc-openapi-kotlin:1.6.14")
implementation("org.springdoc:springdoc-openapi-security:1.6.14")

• What is the actual and the expected result using OpenAPI Description (yml or json)?
  The expected behavior is that the field will not be listed as required, but it is getting listed as required.

• Provide with a sample code (HelloController) or Test that reproduces the problem
  @field:Schema(required = false, description = "Should not be required") val nonNullableWithDefault: String = "a default value",

Expected behavior

I would expect the field to not be marked as required. We should be able to have full control over the spec.

Additional context

This was brought up in both #1468 and #1285 but dismissed. Based on some comments of others in the threads and my own opinion, this does appear to be a bug from our perspective. I wanted to bring this up one more time. If it is decided that it will not get addressed, maybe we can at least get some documentation added around the behavior with Kotlin?

[samlindsaylevine]

@Clubfan22 analyzed the underlying issue in #1285 as being that the KotlinAnnotationIntrospector takes priority over the SwaggerAnnotationIntrospector, and thus the @Schema annotation is never even considered.

The following is a possible, moderately ugly, workaround that registers a new AnnotationIntrospector at even higher priority:

@Configuration
class SpringDocRequiredFix(objectMapperProvider: ObjectMapperProvider,
        // Make this a dependency so that this gets instantiated afterwards and our AnnotationIntrospector is guaranteed
        // to end up at the top of the priority order.
                           @Suppress("UNUSED_PARAMETER") unused: SpringDocKotlinConfiguration) {
  init {
    objectMapperProvider.jsonMapper().registerModule(SpringDocRequiredFixModule())
  }
}

private class SpringDocRequiredFixModule : SimpleModule() {
  override fun setupModule(context: SetupContext) {
    context.insertAnnotationIntrospector(RespectSchemaRequiredAnnotationIntrospector())
  }
}

private class RespectSchemaRequiredAnnotationIntrospector : NopAnnotationIntrospector() {
  override fun hasRequiredMarker(m: AnnotatedMember): Boolean? {
    val schemaAnnotation = m.getAnnotation(Schema::class.java)

    return schemaAnnotation?.required
  }
}

This workaround does have the downside that if a field is non-nullable, and should be required, and does have a @Schema annotation, the developer has to explicitly set "required = true" in the @Schema annotation (since its required property defaults to false).

The awesomest possible solution would be if we could detect fields that were non-nullable but had a Kotlin default and automatically consider them not-required. This is difficult to do via reflection since Kotlin default values are just compiled into constructor overloads. In the absence of such an awesome solution, some solution that lets us at least explicitly define "required" by hand is necessary, even if it is as ugly as the above workaround.

Thank you for your consideration!

[bnasslahsen]

@samlindsaylevine Thank you for your analysis.
@k96payne, I have added the workaround.

Let me know if it works for you.

[samlindsaylevine]

Thanks for your work in this matter @bnasslahsen, I appreciate your contributions and the overall usefulness and high quality of this library!

I do worry a little bit that my inelegant workaround may pose a disruption to existing users of the library - in particular, someone who had written (similarly to your unit test)

class DemoRequest {
	// Note particularly - no explicit specification of "required" in @Schema annotation; no default on this non-nullable field
	@field:Schema(description = "Should be required")
	val nonNullableWithNoDefault: String
}

previously would have had springdoc-openapi correctly indicate that this field was required; but after my workaround, at least, springdoc-openapi would mark it as not-required; and the developer would now have to explicitly mark "required = true" to get back to the pre-existing behavior.

Naturally I defer to you in whether this behavior is too disruptive or too breaking a change for existing library users; I just wanted to be sure that you were aware of this deficiency of my moderately ugly workaround, and ensure that I was not leading you down a bad path by accident. And perhaps your implementation may have avoided this problem; I haven't tested to verify.

[bnasslahsen]

@samlindsaylevine,

Definitely agree. It's a breaking change. But it gives more flexibility
If the developer adds @Schema - it's by default required false! so we are more consistent with the swagger annotation. No ?

[samlindsaylevine]

Definitely agree. It's a breaking change. But it gives more flexibility
If the developer adds @Schema - it's by default required false! so we are more consistent with the swagger annotation. No ?

I agree that this new behavior is fundamentally reasonable! I just wanted to be sure you know about the discrepancy, in case you needed to include notification of the change in release notes, or any other such steps.

Thanks again!

[pepperbob]

👍 Thanks - this fix essentially works around FasterXML/jackson-module-kotlin#609

[mikehalmamoj]

I'm afraid I don't think the new behaviour is reasonable. I'd say that if a field is non-nullable then it's required regardless of whether it has a @Schema annotation or not. (Isn't required deprecated anyway?).

Sorry if I sound grumpy but we're using openapi generator for Kotlin to generate some API clients and it relies on the required attribute to determine if they are nullable or not. Now I have to go and nag the API provider to add the required attribute to all of their non-nullable @Schema fields - or ditch the generator.

Also a bit confused why you knowingly put out a breaking change on a patch version. And didn't mention it in the release notes. 🤷

[pepperbob]

@mikehalmamoj thanks for pointing out that required is deprecated - I see that requiredMode gives more control because it's not the binary yes/no anymore.

I'd say that if a field is non-nullable then it's required regardless

This is true unless you specify default values for example.

So I didn't test it yet, but does that mean using requiredMode should have fixed this in the first place?

[mikehalmamoj]

Hmmm I think default values is the only counter example but from reading other issue threads it sounds difficult to solve due to the way default values are compiled.

And I'm not sure about requiredMode tbh.

I'll stop whining and try to put something concrete together over the weekend on #2185. In the meantime I need to workaround the current version.