[BUG] Missing Java Bean Validation annotations when using `allOf` instead of $ref directly

jaaufauvre commented 4 years ago

Bug Report Checklist

[x] Have you provided a full/minimal spec to reproduce the issue?
[x] Have you validated the input using an OpenAPI validator (example)?
[x] Have you tested with the latest master to confirm the issue still exists?
[x] Have you search for related issues/PRs?
[x] What's the actual output vs expected output?
[ ] [Optional] Sponsorship to speed up the bug fix or feature request (example)

Description

When using:

components:
  schemas:
    Name:
      type: string
      maxLength: 50
      minLength: 1
      description: "A name"
    Cat:
      type: object
      description: "A cat with a name"
      properties:
        name:
          allOf:
            - $ref: '#/components/schemas/Name'
          description: "Name of this cat"
          example: "Kitty"

The generated code is:

@ApiModelProperty(example = "Kitty", value = "Name of this cat") 
public String getName() {
   return name;
}

Instead of:

@ApiModelProperty(example = "Kitty", value = "Name of this cat")
@Size(min=1,max=50) 
public String getName() {
   return name;
}

openapi-generator version

OpenAPI Generator 4.3.1 and 5.0.0-alpha.

OpenAPI declaration file content or url

openapi: 3.0.3
info:
  title: My animal API
  version: "1.0"
servers:
  - url: https://api.example.com
paths:
  /cats/:
    post:
      operationId: createCat
      requestBody:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/Cat'
      responses:
        201:
          description: ""
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Cat'
  /dogs/:
    post:
      operationId: createDog
      requestBody:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/Dog'
      responses:
        201:
          description: ""
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Dog'
components:
  schemas:
    Name:
      type: string
      maxLength: 50
      minLength: 1
      description: "A name"
    Cat:
      type: object
      description: "A cat with a name"
      properties:
        name:
          allOf:
            - $ref: '#/components/schemas/Name'
          description: "Name of this cat"
          example: "Kitty"
    Dog:
      type: object
      description: "A dog with a name"
      properties:
        name:
          allOf:
            - $ref: '#/components/schemas/Name'
          description: "Name of this dog"
          example: "Fido"

Generation Details

Plugin configuration:

<plugin>
    <groupId>org.openapitools</groupId>
    <artifactId>openapi-generator-maven-plugin</artifactId>
    <version>4.3.1</version>
    <executions>
        <execution>
            <id>code-from-openapi-document</id>
            <goals>
                <goal>generate</goal>
            </goals>
            <configuration>
                <inputSpec>${project.basedir}/src/main/resources/spec.yaml</inputSpec>
                <generatorName>spring</generatorName>
                <library>spring-boot</library>
                <apiPackage>com.acme.animals.controller</apiPackage>
                <modelPackage>com.acme.animals.dto</modelPackage>
                <generateApis>true</generateApis>
                <configOptions>
                    <interfaceOnly>true</interfaceOnly>
                    <dateLibrary>java8</dateLibrary>
                    <enableBuilderSupport>true</enableBuilderSupport>
                    <skipDefaultInterface>true</skipDefaultInterface>
                    <performBeanValidation>true</performBeanValidation>
                </configOptions>
            </configuration>
        </execution>
    </executions>
</plugin>

Steps to reproduce

Run mvn clean generate-sources
The generated classes contain:

  @ApiModelProperty(example = "Kitty", value = "Name of this cat")
  public String getName() {
    return name;
  }

ibazavan commented 4 years ago

Ran into the exact same issue, but when using a multipart request as follows:

post:
  tags:
    - Details
  summary: Post details
  operationId: postDetails
  requestBody:
    content:
      multipart/form-data:
        schema:
          type: object
          properties:
            details:
              $ref: '#/components/schemas/detailsV1'
            file:
              type: string
              format: binary
          required:
            - details
            - file

This will generate:

@Override
    public ResponseEntity<ResponseV1> postDetails(DetailsV1 details, @Valid MultipartFile file) {
        return null;
    }

Which will be missing the '@Valid' annotation for the JSON object.

robertc-dat commented 3 years ago

Still seeing this issue with OpenAPI Generator 5.2.0

tommyb82 commented 3 years ago

We have just encountered this issue (or a very closely related one) via the Maven plugin, openapi-generator-maven-plugin v5.1.1 & 5.2.0 - Spring generator. We are missing the @Pattern annotation (and example attribute in @ApiModelProperty) on the agreementID property in our generated AgreementDetails class, where pattern is specified on the referenced base simple type AgreementId, e.g.

    AgreementId:
      type: string
      pattern: RM[0-9]{4}(.[0-9]{1,2})?
      example: RM1234
      readOnly: true

    AgreementDetails:
      type: object
      properties:
        agreementID:
          allOf:
            - $ref: '#/components/schemas/AgreementId'
          readOnly: false

pschichtel commented 3 years ago

I looked into this quickly yesterday. It's definitely not a template issue, the input model of the template is already lacking the values for the limits. We have some models in our code base that have this issue and other seemingly identical models that work perfectly fine.

Initially I also assumed that this could be related to allOf, however none of our affected models are used directly as part of an allOf and not even the containing models are part of an allOf directly.

dmivankov commented 3 years ago

Some extra info:

allOf (object 1, object 2) keeps constraints on non-intersecting fields
allOf (object 1, object 2) picks just one (from last option it seems) constraint for intersecting fields (note that all other choices are also generated in ModelAllOf[N].java outputs)
allOf (primitive type) discards all constraints
allOf (primitive type, primitive type) discards all constraints

Quick hack could be to make DefaultCodegen.unaliasSchema handle allOf(single $ref) specifically. This won't help for combining constraints on primitive types, but that also currently doesn't work for allOf(several object types)

After unaliasSchema goes ModelUtils.syncValidationProperties and sees schema instanceOf ComposedSchema which doesn't show constraints by itself.

ComposedSchema.getType() == null so DefaultCodegen.fromProperty calls into DefaultCodegen.getSchemaType and gets the primitive type out. Notably to get that it traverses allOf options and can see constraints, also there it picks just one branch.

Looks like a better fix could be making ModelUtils.syncValidationProperties join constraints in allOf ComposedSchema (and maybe also find minimal common constraints in oneOf/anyOf case)

dmivankov commented 3 years ago

As a quick workaround, extending generator (tried with JavaClientCodegen) with

  @Override
  public CodegenProperty fromProperty(String name, Schema p) {
    CodegenProperty result = super.fromProperty(name, p);
    syncValidationProperties(p, result);
    return result;
  }

  private void syncValidationProperties(Schema schema, IJsonSchemaValidationProperties target) {
    if (!(schema instanceof ComposedSchema) || target == null) {
      return;
    }
    // rely on ModelUtils.syncValidationProperties not re-applying empty constraints
    // don't handle allOf (min=2, min=1), the last one wins for now
    Optional.ofNullable(((ComposedSchema) schema).getAllOf())
        .orElse(Collections.emptyList())
        .stream()
        .map(p -> ModelUtils.unaliasSchema(openAPI, p, importMapping))
        .forEach(p -> ModelUtils.syncValidationProperties(p, target));
  }

seems to work for some cases at least

dmivankov commented 2 years ago

for newer versions workaround needs to use new overload

  @Override
  public CodegenProperty fromProperty(String name, Schema p, boolean required, boolean schemaIsFromAdditionalProperties) {
    CodegenProperty result = super.fromProperty(name, p, required, schemaIsFromAdditionalProperties);
    syncValidationProperties(p, result);
    return result;
  }

  /**
   * Workaround for https://github.com/OpenAPITools/openapi-generator/issues/7342
   * extended version of
   * {@link org.openapitools.codegen.utils.ModelUtils#syncValidationProperties(Schema, IJsonSchemaValidationProperties)}
   */
  private void syncValidationProperties(@Nullable Schema schema,
      @Nullable IJsonSchemaValidationProperties target) {
    if (!(schema instanceof ComposedSchema) || target == null) {
      return;
    }
    // rely on ModelUtils.syncValidationProperties not re-applying empty constraints
    // don't handle allOf (min=2, min=1), the last one wins for now
    Optional.ofNullable(((ComposedSchema) schema).getAllOf())
        .orElse(Collections.emptyList())
        .stream()
        .map(p -> ModelUtils.unaliasSchema(openAPI, p, importMapping))
        .forEach(p -> ModelUtils.syncValidationProperties(p, target));
  }

OpenAPITools / openapi-generator