swagger-core icon indicating copy to clipboard operation
swagger-core copied to clipboard

Ignore @NotNull annotations

Open nvervelle opened this issue 2 years ago • 6 comments

In ModelResolver, the list NOT_NULL_ANNOTATIONS is defined and contains the annotations that implies that the field is required.

Is there a way to disable this behavior, or remove some annotations from the list ? Or to ignore the annotation on some attributes ?

The assumption that a field annotated @NotNull should always be declared as required = true is incorrect and we should be able to disable this behavior. It can cause problem both for incoming data structure and for outgoing data structure.

Example of problems for incoming data structure

    @Schema(description = "Additional properties")
    @JsonProperty("additionalProperties")
    @JsonSetter(nulls = Nulls.AS_EMPTY)
    @NotNull
    Map<String, String> additionalProperties

The parameter additionalProperties should be marked as required = false, even with the @NotNull annotation : when the additionalProperties attribute is not provided, the @JsonSetter annotation will ensure that the parameter is properly initialized with an empty Map. So on the Java side, it's clearly a non-nullable attribute, but on the openapi spec side it should be required = false.

Example of problems for outgoing data structure

    @Schema(description = "Additional properties")
    @JsonProperty("additionalProperties")
    @JsonInclude(JsonInclude.Include.NON_EMPTY)
    @NotNull
    Map<String, String> additionalProperties

The parameter additionalProperties should be marked as required = false, even with the @NotNull annotation : when the additionalProperties parameter is an empty map, the @JsonInclude annotation will ensure that the attribute is not added to the payload. So on the Java side, it's a non-nullable attribute, but on the openapi spec side it should be required = false.

nvervelle avatar Jul 19 '22 08:07 nvervelle

One idea to allow disabling the NOT_NULL_ANNOTATIONS on a given attribute :

  • Add a attribute forceUseRequired on @Schema annotation, with a default value as false
  • Ignore the NOT_NULL_ANNOTATIONS when forceUseRequired is equal to true

I can submit a simple PR for this proposal.

Both examples in the issue description could then be easily fixed :

    @Schema(description = "Additional properties", forceUseRequired = true)
    @JsonProperty("additionalProperties")
    @JsonSetter(nulls = Nulls.AS_EMPTY)
    @NotNull
    Map<String, String> additionalProperties
    @Schema(description = "Additional properties", forceUseRequired = true)
    @JsonProperty("additionalProperties")
    @JsonInclude(JsonInclude.Include.NON_EMPTY)
    @NotNull
    Map<String, String> additionalProperties

nvervelle avatar Jul 20 '22 08:07 nvervelle

Thanks for looking into it, and yes a PR is welcome

A possible alternative to your proposal is still using a new @Schema field but also deprecating required, similar to what done with AccessMode

This would have the advantage of having a single field to define the behavior, something like:

RequiredMode requiredMode() default RequiredMode.AUTO;

with

    enum RequiredMode {
        AUTO,
        REQUIRED,
		FORCED_REQUIRED
        NOT_REQUIRED,
    }
```	

frantuma avatar Jul 20 '22 09:07 frantuma

Hi @frantuma, thanks for the comments.

I've submitted a PR #4216 with the additional property forceUseRequired.

For your proposal with requiredMode, I'm not sure I understand all the possible values :

  • AUTO will let the library decide based on its heuristics
  • NOT_REQUIRED will force the result to be required = false regardless of heuristics
  • REQUIRED or FORCED_REQUIRED will force the result to be required = true regardless of heuristics => what's the difference between the 2 options ? or why there's a FORCED_REQUIRED, but not a FORCED_NOT_REQUIRED ?

nvervelle avatar Jul 20 '22 09:07 nvervelle

@nvervelle, sorry the above was confusing and with an invalid value.. idea is the following:

  • AUTO will let the library decide based on its heuristics
  • NOT_REQUIRED will force the result to be required = false regardless of heuristics
  • REQUIRED will will force the result to be required = true

frantuma avatar Jul 20 '22 11:07 frantuma

Hi @frantuma, thanks for the clarification.

I've submitted a second PR #4221 implementing your suggestion.

nvervelle avatar Jul 20 '22 14:07 nvervelle

Hi @frantuma , any hope of validating the PR ? https://github.com/swagger-api/swagger-core/pull/4221

nvervelle avatar Sep 13 '22 15:09 nvervelle

Thanks a lot! merged #4286 which includes #4221

frantuma avatar Nov 02 '22 10:11 frantuma