smallrye-open-api icon indicating copy to clipboard operation
smallrye-open-api copied to clipboard

Support kotlin nullable/non-nullable types as if explicitly set @Schema(required=true/false)

Open lenalebt opened this issue 3 years ago • 12 comments

Referring to https://github.com/smallrye/smallrye-open-api/issues/714#issuecomment-789527358 :

Kotlin language has support for nullable types where you explicitly need to state whether they can be null or not. This should translate to @Schema(required=true/false).

To support this, it would be sufficient to check for the existence of org.jetbrains.annotations.{Nullable, NotNull} annotations. As far as I understand, https://github.com/smallrye/smallrye-open-api/issues/131 adds support for javax.validation.constraints.NotNull already, so this might be a minor addition?

Of course, I'd rather ike to have the kotlin compiler to support generating the javax.validation.constraints.NotNull annotations, but this currently seems not easily possible for reasons beyond my knowledge.

lenalebt avatar Mar 03 '21 08:03 lenalebt

Thanks for researching this @lenalebt . It seems like a reasonable addition to me. I'm thinking that in addition to required, the nullable attribute should also be affected by these annotations.

MikeEdgar avatar Mar 04 '21 12:03 MikeEdgar

Yep, that seems to make sense to me as well wrt nullable.

lenalebt avatar Mar 04 '21 15:03 lenalebt

Here's what I'm thinking -

  • @Nullable --> nullable: true, similar to using Optional
  • @NotNull --> property added to required array. Do not set nullable, the default value is already false

We are setting nullable to false for other Bean Validation constraints, despite the default being false. @phillip-kruger, what do you think about removing that? Maybe save that for the next MP version that supports OpenAPI 3.1.0 where it appears nullable is no longer an attribute of the JSON Schema.

MikeEdgar avatar Mar 07 '21 23:03 MikeEdgar

Sound good to me !

phillip-kruger avatar Mar 08 '21 02:03 phillip-kruger

@lenalebt - it turns out that these annotations use @Retention(RetentionPolicy.CLASS) (instead of RUNTIME) which are unfortunately not visible to the scanner... It's possible that at some point it could be supported, but that would require changes to the upstream Jandex library, but there are likely good reasons for excluding them there.

MikeEdgar avatar Mar 11 '21 13:03 MikeEdgar

Oh, too bad, that basically ruins that idea to just support those annotations.

Just out of curiosity - I don't know much about how this lib is structured in other areas. There is some special kotlin reflection that can return that information as well, whether a type is nullable or not. I have seen that there is a plugin system. How much work would it be to have an additional kotlin plugin to support this? I mean - that one could easily be implemented in kotlin then, using the native language features, right?

lenalebt avatar Mar 11 '21 20:03 lenalebt

Since we've been talking about these and the Jackson annotation, I've been thinking about a way to make the BeanValidationScanner less monolithic and more pluggable. I think something along those lines would work, provided the code you write in Kotlin to implement the plugin would be callable from the scanner, which it should be. We can leave this issue open to work through that and see if it's viable.

MikeEdgar avatar Mar 11 '21 21:03 MikeEdgar

The code of course needs to be callable. Just had a quick look over there. I don't know Jandex, and it seems to abstract away the underlying reflect classes? If I get a java java.lang.reflect.Field or java.lang.reflect.Method I should be able to find out whether it's nullable or not, like this:

fun markKotlinRequired(field: java.lang.reflect.Field): Boolean =
  field.kotlinProperty?.returnType?.isMarkedNullable == false

fun markKotlinRequired(method: java.lang.reflect.Method): Boolean =
  method.kotlinFunction?.returnType?.isMarkedNullable == false

Unsure though if I would get this?

lenalebt avatar Mar 11 '21 22:03 lenalebt

@lenalebt , I'm not sure either if that would work either. I imagine that Kotlin is somehow wrapping those reflection type and looking at the invisible annotations (which are still in the class file). Jandex extracts the class and annotation information into an efficient structure so that reflection can be avoided at runtime.

MikeEdgar avatar Mar 13 '21 12:03 MikeEdgar

Hi, is this still blocked? Do I understand correctly that you'd still have to annotate with @Schema(required = true) in Kotlin for now? For some reason, I'm unable to find, it doesn't have any effect for me...

@Schema(title = "foo") // <-- this has an effect
data class SomeDto(
    @Schema(required = true) // <-- this has no effect for me
    val name: String
)

b-thiswatch avatar Aug 29 '22 09:08 b-thiswatch

@b-thiswatch , it's still blocked for now. Can you share the output that is generated for that model?

MikeEdgar avatar Aug 29 '22 11:08 MikeEdgar

@MikeEdgar thanks for the update. I will prepare a demo later. For now, I have solved it with

@Schema(requiredProperties = ["name"])
data class SomeDto(
    val name: String
)

which I don't like very much, but it works.

b-thiswatch avatar Aug 29 '22 17:08 b-thiswatch

Unblocked now that we have Jandex 3. I'll start looking at this soon for the 3.1.0 and 2.3.1 releases.

MikeEdgar avatar Sep 06 '22 23:09 MikeEdgar