feat: file upload cel expressions and constraints

[timdah]

• Add cel expressions and constraints request part to the file upload
• Create the cel expressions and add constraints to policy for validation
• Update tests

Note: Not really happy with all the string parsing around the multipart file upload, but swagger still struggles with List items and content-type: swagger-api/swagger-ui#6462

[paullatzelsperger]

pls remove wildcard imports. I suspect, this is an IntelliJ-autoformatter issue

[timdah]

Yes, will look through all changes again.

[paullatzelsperger]

About list types: the runtime-type of a method argument should never be dictated by the documentation tool. One way to document list types is to define the content type explicitly in OAS annotations:

@Operation(description = "...",
  requestBody = @RequestBody(content = @Content(schema = @Schema(implementation = FooBarSchema.class))),
  responses = {
    @ApiResponse(responseCode = "200", description = "...", content = @Content(array = @ArraySchema(schema = @Schema(implementation = BarBazObject.class))))
  }
)

Could you please update the controller accordingly?

[timdah]

@timdah timdah edited 2 minutes ago
About list types: the runtime-type of a method argument should never be dictated by the documentation tool. One way to document list types is to define the content type explicitly in OAS annotations:

@operation(description = "...",
requestBody = @RequestBody(content = @content(schema = @Schema(implementation = FooBarSchema.class))),
responses = {
@apiresponse(responseCode = "200", description = "...", content = @content(array = @ArraySchema(schema = @Schema(implementation = BarBazObject.class))))
}
)
Could you please update the controller accordingly?

Tried it this way as well, but the generated client still sends list multipart parts as application/octet-stream which is rejected by the controller. I think at this point the best way may be to refactor the String arguments to the correct type and don't use the generated clients method for the upload call, but the native Angular HttpClient.

[paullatzelsperger]

@timdah I think I edited your comment by mistake rather than responding. I tried to restore it as best I could.

Tried it this way as well, but the generated client still sends list multipart parts as application/octet-stream which is rejected by the controller. I think at this point the best way may be to refactor the String arguments to the correct type and don't use the generated clients method for the upload call, but the native Angular HttpClient.

I agree. Code generation should only be used as starting point, and we shouldn't adapt controller methods to it, instead, client code should be tuned to accurately correspond to API controller methods.

[paullatzelsperger]

creating CEL expressions (plural) here, and in line 111ff seems kinda iffy. My suggestion here would be to never pass null for the celExpressions list, but possibly an empty list, and then adding the CelExpression from line 113 to it, so that there is always 1...N cel expressions in the list.

This could be achieved by supplying a default value to the controller method.

[paullatzelsperger]

uargh, lets not put optionals in method signatures :)
better to leave the param as List<CelExpression>, and then, in the method body, do this:

file.getOriginalFilename(),
ofNullable(celExpressions).orElseGet(List::of),
policySet

notice, that the DataAccessService can never assume, that an incoming argument (here: the cel expressions) is modifiable. it should always instantiate a new ArrayList in its method body.

[timdah]

Okay, to my knowledge Jackson deserializes to a mutable ArrayList, so I thougt we could skip the instantiation of a new ArrayList. But I get it, this is not guaranteed and could change in future versions.
Will change it back.

[paullatzelsperger]

not just that, but putting concrete types in method signatures is a bit of a smell. Think testability. While it may not make an actual difference in controller methods, it will very much make a difference in any other class.

[paullatzelsperger]

don't put concrete collection types into method signatures. In the method body, the service should assume the celExpressions list to be immutable, and create a new ArrayList