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

Making primitive types required via customizers?

Open dbarvitsky opened this issue 1 year ago • 0 comments

First off, thank you very much for doing all this hard work and maintaining Swagger. I ran into a peculiar problem with 2.2.0, which, by the look of it seems to be still relevant for the newer versions, and thought it might be an opportunity for improvement.

Original problem

By default swagger-core (2.2.0) does not mark scalar properties (boolean, int, ...) as required, unless they are annotated with one of the ModelResolver.NOT_NULL_ANNOTATIONS annotations. The following will result in getPropertyA() being optional:

public interface ScalarTypesIssue {
  boolean getPropertyA();
 }

This is not incorrect per se (the implementation could be done in such way that the property will not be serialized if it isn't set, and so from the REST API's perspective it may still be optional). However, it is rather unpractical since most POJOs have the getters and setters of the same type, and boolean or int typically imply a required value.

This can be worked around by adding one of the not-null annotations:

import javax.validation.constraints.NotNull;

public interface ScalarTypesIssue {
  @NonNull
  boolean getPropertyA();
}

This workaround, however, is hard to apply when you do not control the type definitions (in our case they were generated by another tool that treats the scalar types as required). This may also not be feasible if the required-ness of a property is determined elsewhere, i.e. by a naming convention, various monadic types, and suchlike madness.

Attempted solution

I tried using the customizers for fixing this behavior as they seemed to be a good fit. The customizer would inspect the type and, if it is indeed primitive, append to the parent's required.

This, however, did not work as expected. Namely in this situation:

public interface ScalarTypesIssue {
  boolean getPropertyA();
  boolean getPropertyB();
}

only the propertyA would become required.

This happens for two reasons:

  1. the required-ness in OpenAPI is not context-free (unfortunately). Only the parent schema knows which properties are required, so making this decision while inspecting the property looks a bit sketchy.
  2. the ModelResolver and ModelContext short-circuit the type inspection by caching already discovered schemas with their AnnotatedTypes as cache keys. When the getPropertyB() is introspected, the boolean type is already present in the ModelContext's cache, therefore it is just re-used and customizers are not called on it. The ModelResolver applies the required-ness detection logic based on annotations (here is where ModelResolver.NOT_NULL_ANNOTATIONS comes into play), but that does not seem to be customizable to the point needed for my use case.

Eventually I made it work by creating a customizer that introspects the type again, re-discovers all of its properties, and populates the required as necessary. This effectively double-scans every POJO and is less than ideal.

The code is pretty long, but I can come up with a distilled version if this is required.

Questions / suggestions

  1. Is there a better approach to implementing customizations like this?
  2. Does the introspection short-circuiting in ModelContext.resolve work as expected here? It appears that AnnotatedType and by extension customizers are designed to be contextual, however the ModelContext.resolve implementation treats the types as context-free (type equivalency is determined by a subset of annotations and type alone, see AnnotatedType.hashCode, and ignores the context in which the type is used). This makes the result of introspection dependent on which path along the schema graph is traversed first, which is non-deterministic in practice. Perhaps the customizers should be context-free (type customizations are not dependent on the context where the type is used), or perhaps there should be a way to "post-process" a type in the context where it is used independently of other uses?
  3. Perhaps treating primitive types as required could be added to the config as an option?

Thank you very much for looking into this!

dbarvitsky avatar Jun 26 '23 20:06 dbarvitsky