search

swagger-akka-http / swagger-scala-module

Swagger support for scala
Apache License 2.0
10 stars 10 forks source link

Support `requiredMode` Schema Annotation #224

Closed leedsalex closed 1 year ago

leedsalex commented 1 year ago

In the latest release of Swagger Core (as of v2.2.5) they deprecated the required annotation property and replaced it with requiredMode. I believe support for this change will need to be added to this project.

See this PR for the implimentation: https://github.com/swagger-api/swagger-core/pull/4286

pjfanning commented 1 year ago

there are no failing tests - can you provide a test that shows there is an issue?

pjfanning commented 1 year ago

I am not very interested in maintaining this project - see the swagger-akka-http readme - since Akka is no longer OSS - I'm not very interested in it. When Pekko is out, I will completely abandon Akka.

pjfanning commented 1 year ago

Ok - since this module is independent of Akka, I've created #225 - I'll need to look a bit more at this before merging.

It's messy because the deprecated required() is still independent or requiredMode() so swagger-scala-module will still need to read both. requiredMode() is used in preference to required() - the latter is only checked if requiredMode() == Auto (the default mode)

pjfanning commented 1 year ago

Design Proposal

When users add swagger annotations (Schema, ArraySchema, Parameter), they can override whether a model property is required or not. Whether a model property is required or not, is usually based on the type of the related Scala field (Ie fields of type Option[T] are optional - other fields are required).

The annotations mentioned above have a required() setting which is boolean and defaults to false. It is impossible to know if the user explicitly set false or if the are using the annotations to override other settings but don't intend to affect the required setting.

swagger v2.2.5 introduces a requiredMode() setting on the Schema and ArraySchema annotations. The modes are Required, Not_Required and Auto - the latter is the default. This is better - but the required() setting, while deprecated, is still there and is set independently of the requiredMode().

The current treatment of required()=false is described in the readme. SwaggerScalaModelConverter.setRequiredBasedOnAnnotation will continue to affect how required()=false is interpreted.

requiredMode() required() Effect on Model Property
Required this value is ignored Model property is overridden to be required.
NotRequired this value is ignored Model property is overridden to be not required.
Auto (default) true Model property is overridden to be required.
Auto (default) false (default) if SwaggerScalaModelConverter.setRequiredBasedOnAnnotation is true (current default), then the model property is overridden to be required. Otherwise, whether the model property is required is not depends on the data type and whether a default value is set.

Aim is to change the default for if SwaggerScalaModelConverter.setRequiredBasedOnAnnotation to false in a release in a few months time. I'm afraid that some existing users will get unexpected changes in behaviour if these changes are rolled out too quickly.

@SamTheisens could you review this design? This proposal changes how SwaggerScalaModelConverter.setRequiredBasedOnAnnotation(false) works - this setting no longer ignores the annotations altogether - it means it just ignores required()=false (which can be set inadvertently)

pjfanning commented 1 year ago

swagger-scala-module v2.9.0 was released with this change