[BUG][Java-Spring] Collection Bean Validation is not working with JsonNullable and Optional

[MelleD]

Bug Report Checklist

• Have you provided a full/minimal spec to reproduce the issue?
• Have you validated the input using an OpenAPI validator (example)?
• Have you tested with the latest master to confirm the issue still exists?
• Have you searched for related issues/PRs?
• What's the actual output vs expected output?

Description

As discussed here, bean validation does not work with Optional and JsonNullable if the type is a collection.
OpenAPITools/jackson-databind-nullable#49. The @Valid is on the wrong position.
Example

  @Valid 
   private final JsonNullable<Set<MyDto2>> beansNullable = JsonNullable.of( Set.of( "" ) );

The @Valid must into the Set. Then the validation works.

 
   private final JsonNullable<Set<@Valid MyDto2>> beansNullable = JsonNullable.of( Set.of( "" ) );

openapi-generator version

Latest

OpenAPI declaration file content or url

If you post the code inline, please wrap it with

openapi: 3.0.1
info:
  title: test
  version: 1.0.0
paths:
  /test:
    get:
      summary: test
      operationId: test
      responses:
        200:
          $ref: '/components/responses/MyDto'

components:
  responses:
    AspectDto:
      description: ""
      content:
        application/json:
          schema:
            $ref: '/components/schemas/MyDto'
  schemas:
    MyDto:
      type: object
      properties:
        labels:
          type: array
          description: dtos
          uniqueItems: true
          items:
            $ref: '/components/schemas/MyDto2'
          maxItems: 10
          nullable: true
    MyDto2:
      type: object
      properties:
        label:
          type: string
          nullable: false

Suggest a fix

After a small research. I found a way into the SpringCodgen generation. The Java Convention is very ugly and i found no way to fix in the mustache template, cause the data type is set directly by the generator.

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

   private void replaceBeanValidationCollectionType( final CodegenProperty codegenProperty ) {
      if ( !codegenProperty.isContainer || !useBeanValidation ) {
         return;
      }
      final String dataType = codegenProperty.datatypeWithEnum;
      if ( StringUtils.isEmpty( dataType ) ) {
         return;
      }
      codegenProperty.datatypeWithEnum = dataType.replace( "<", "<@Valid " );
      codegenProperty.dataType = codegenProperty.dataType.replace( "<", "<@Valid " );
   }

The downside is the generated code doesn't look very fancy. Setters are also affected.
In addition, the @Valid for wrapper classes (Long, Integer etc.) and String is unnecessary. But the solution would work quickly and is not much effort. Since the code is generated, I could live with the (unnecessary) additional annotation.

The second possibility would be to introduce a data type for getter and setter, then you could distinguish between them.
In addition, the data type have to be evaluated, if it is a complex data type or not

What do you think? Do you have any other suggestions?

@cachescrubber @welshm @atextor @manedev79 @javisst @borsch @banlevente @Zomzog
@wing328 @tofi86

[atextor]

I think the @Valid annotations on the numeric types don't hurt at all. Your solution is straightforward and does not add unnecessary complexity, so I'd vote for it.

[welshm]

I think this likely could be fixed in the mustache, but this solution also seems simple (although maybe misleading when reading the mustache files since it's just called dataType not dataTypeWithPossibleValidation).

I am in favor of this solution though

[MelleD]

Thanks for the quick answer 👍

@welshm just for my interest. How can this currently fixed in mustache?
That's only possible if we introduce another variable, right?

I think i have also override fromParameter for the Optional part. Anybody know if i need also this one
fromFormProperty? Not sure what this is :)

[welshm]

Thanks for the quick answer 👍

@welshm just for my interest. How can this currently fixed in mustache?

That's only possible if we introduce another variable, right?

Yeah, another variable would need to be added I think.

[tofi86]

@MelleD Thanks for taking the lead on this issue!

I understand that "fixing" JsonNullable collection validation with OpenAPITools/jackson-databind-nullable#35 (which caused OpenAPITools/jackson-databind-nullable#49) was a misinterpretation of me how @Valid should work on wrapped items.

But looking at your proposed solution …

codegenProperty.datatypeWithEnum = dataType.replace( "<", "<@Valid " );
codegenProperty.dataType = codegenProperty.dataType.replace( "<", "<@Valid " );

… does moving the @Valid annotation from the wrapper to the collection item still handles the following example you mentioned in OpenAPITools/jackson-databind-nullable#49 (comment) (and which was also one of my initial intentions to file OpenAPITools/jackson-databind-nullable#35)?

@Valid
@Size( min = 2 )
public JsonNullable<Set<MyBean2>> getBeans2() {
   return beans2;
}

With your proposed change, this will be generated as …

@Size( min = 2 )
public JsonNullable<Set<@Valid MyBean2>> getBeans2() {
   return beans2;
}

… and without trying I guess the @Size constraint isn't validated anymore, is it?

[MelleD]

… and without trying I guess the @SiZe constraint isn't validated anymore, is it?

No the @SiZe is working as well, because it's for the container.
The inside @Valid is for the inner type MyBean2

See output of this Test
beans2[].name: must not be null
beans2: size must be between 2 and 2147483647

public class JsonNullableTest extends WebBaseTest {

   public static class MyBean {

      @Valid
      private final JsonNullable<Set<@Valid MyBean2>> beans2 = JsonNullable.of( Set.of( new MyBean2() ) );

      @Valid
      @Size( min = 2 )
      public JsonNullable<Set<MyBean2>> getBeans2() {
         return beans2;
      }

   }

   public static class MyBean2 {
      @NotNull
      private String name;
   }

   @Autowired
   ValidationService validationService;

   @Test
   void test() {
      final MyBean bean = new MyBean();

      try {
         validationService.validate( bean );
      } catch ( final ConstraintViolationException e ) {
         e.getConstraintViolations().stream()
               .forEach( constraintViolation -> System.out.println( constraintViolation.getPropertyPath() + ": " + constraintViolation.getMessage() ) );
      }

   }

}

@atextor @welshm one good new. Found a way just to @Valid for model types and not for String and primitive type.
One pitfall i didn't find a solution. I would not add @Valid on a ResponseEntity e.g ResponseEntity<List<@Valid MyBean2>>.
Therefore i added a check when the base name is response to skip the @Valid appending.
BUT if you have a model and choose a name for the attribute e.g. response. Then the bug is still valid :)
e.g.

  schemas:
    ResponseTest:
      type: object
      properties:
        response:
          type: array
          description: dtos
          uniqueItems: true
          items:
            $ref: '#/components/schemas/ResponseTest2'
          maxItems: 10
          nullable: true

For me it's okay, but yes a ugly edge case. I find no solution

[MelleD]

First try to fix it. Feedback welcome. Then i can take another round next week.
Thanks a bunch

[welshm]

Might be worth documenting the edge-case in the Spring codegen documentation somewhere?

[MelleD]

Fix bean validation for Collection and add unit test #14736

Yeah sure where is the right place?