openapi-generator icon indicating copy to clipboard operation
openapi-generator copied to clipboard

[BUG][JAVA] Not specifying additionalProperties should generate the Map

Open ncelerier opened this issue 1 year ago • 13 comments

Bug Report Checklist

  • [x] Have you provided a full/minimal spec to reproduce the issue?
  • [ ] 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 searched 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

Hello,

Regarding additionalProperties and the specification here: https://json-schema.org/understanding-json-schema/reference/object#additionalproperties, I think there might be a bug in the Java spring generator.

I'm not specifying the additionalProperties property and in this case, because of the specification mentioning:

By default any additional properties are allowed.

I would expect the JsonWebKey generated class to contain the Map<String, Object> additionalProperties (and get/put methods) to be generated. But it is not !

It seems the only way to generate this map is to use either:

additionalProperties: true

Or:

additionalProperties:
  type: string

Which I would like to avoid and shouldn't be necessary.

Am I missing something ?

Thank you for your help !

openapi-generator version

7.10.0 (current latest) 7.11.0-SNAPSHOT (2b891f6da9db9cf24fe0186f5635d550c958c657)

OpenAPI declaration file content or url

My src/main/resources/swagger/swagger.yaml file:

swagger: '2.0'
info:
  version: '1.0'
  title: FooBar
paths:
  /jwks:
    get:
      operationId: getJwks
      responses:
        200:
          description: "Success"
          schema:
            type: "array"
            items:
              $ref: "#/definitions/JsonWebKey"
definitions:
  JsonWebKey:
    title: JSON Web Key
    type: object
    properties:
      kid:
        type: string
    # No additionalProperties specified
Generation Details

I'm using this pom.xml: https://gist.github.com/ncelerier/1729b37b6f9d37aefc8af33926fed07b

Steps to reproduce

Running mvn clean install, then looking at the generated file if the Map is generated or not: target/generated-sources/openapi-server/src/main/java/com/example/models/JsonWebKey.java

Related issues/PRs
Suggest a fix

I can suggest something but I would like to know first if this is really a bug or if I am missing something.

ncelerier avatar Nov 19 '24 15:11 ncelerier

did you try setting disallowAdditionalPropertiesIfNotPresent to false? (e.g. --additional-properties disallowAdditionalPropertiesIfNotPresent=false in CLI)

https://openapi-generator.tech/docs/generators/java/

wing328 avatar Nov 21 '24 05:11 wing328

Hi @wing328, I have this property in the pom.xml :

<configOptions>
  <disallowAdditionalPropertiesIfNotPresent>false</disallowAdditionalPropertiesIfNotPresent>
</configOptions>

Could it be that it is not taken into account ?

ncelerier avatar Nov 21 '24 08:11 ncelerier

I think there might be a bug in the Java spring generator.

Ah. I can only assure this option works for java client generator (jersey2, jersey3, okhttp-gson) but not the spring generator. Sorry.

One way to check is to remove the option or set the option to true to see if there's any change in the output

wing328 avatar Nov 21 '24 08:11 wing328

For the java client generator, I tried with this and indeed the Map is generated:

<generatorName>java</generatorName>
<configOptions>
  <disallowAdditionalPropertiesIfNotPresent>false</disallowAdditionalPropertiesIfNotPresent>
</configOptions>

However, if I set additionalProperties: false in my schema, the Map is still generated whereas it shouldn't.

ncelerier avatar Nov 21 '24 13:11 ncelerier

@ncelerier Please can you help confirm if this is the case for the default library and the java generator?

DavidGrath avatar Dec 23 '24 03:12 DavidGrath

Hello, sorry for this late response.

Forget what I said about the java client generator and additionalProperties: false : the Map is not generated which is the expected behaviour. Sorry, I think I mixed things up.

Here is a recap of the current behaviour using version 7.11.0 :

generatorName additionalProperties value additionalProperties field/methods present Result
spring Not specified No :x: Should be present
spring additionalProperties: false No :heavy_check_mark:
spring additionalProperties: true Yes, Map<String, Object> :heavy_check_mark:
spring additionalProperties: type: string Yes, Map<String, String> :heavy_check_mark:
java Not specified Yes, Map<String, Object> :heavy_check_mark:
java additionalProperties: false No :heavy_check_mark:
java additionalProperties: true Yes, Map<String, Object> additionalProperties :heavy_check_mark:
java additionalProperties: type: string Yes, Map<String, Object> additionalProperties :x: Should be Map<String, String> additionalProperties, as with the spring generator

ncelerier avatar Jan 22 '25 15:01 ncelerier

Hey, @wing328, sorry for taking so long, but I think I see the cause of the issue: https://github.com/OpenAPITools/openapi-generator/blob/de310f6ee16a8d6a44c27e0dca744fee1828b30f/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java#L2868 isDisallowAdditionalPropertiesIfNotPresent is not taken into account at this point, so addAdditionPropertiesToCodeGenModel is never called, which means CodegenModel.additionalPropertiesType is never set here: https://github.com/OpenAPITools/openapi-generator/blob/de310f6ee16a8d6a44c27e0dca744fee1828b30f/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java#L2333 My idea is to add another if-else branch to this block with isDisallowAdditionalPropertiesIfNotPresent, but since DefaultGenerator is such a fundamental class, I want to know if simply adding a test case like givenAdditionalPropertiesNotDefinedAndIsDisallowAdditionalPropertiesIfNotPresentIsFalseWhenGenerateModelThenAdditionalPropertiesTypeIsObject is enough, or if there's more involved. Also, for the next part of this issue:

❌ Should be Map<String, String> additionalProperties, as with the spring generator

I see that this is because Object is hardcoded for okhttp-gson which is the default, and it seems to be that way because of the custom adapter: https://github.com/OpenAPITools/openapi-generator/blob/de310f6ee16a8d6a44c27e0dca744fee1828b30f/modules/openapi-generator/src/main/resources/Java/libraries/okhttp-gson/pojo.mustache#L538 Does this mean that not much can be done without a complete rewrite? I'm not sure if it's worth changing if that's the case

DavidGrath avatar Apr 26 '25 17:04 DavidGrath

@wing328 @martin-mfg , please what are your thoughts?

DavidGrath avatar May 02 '25 18:05 DavidGrath

Hi @ncelerier

to make sure we're all on the same page: all rows of the table in https://github.com/OpenAPITools/openapi-generator/issues/20139#issuecomment-2607479080 were tested with disallowAdditionalPropertiesIfNotPresent=false, correct?


Hi @DavidGrath,

isDisallowAdditionalPropertiesIfNotPresent is not taken into account at this point, so addAdditionPropertiesToCodeGenModel is never called

This is true for both the Spring generator and the Java generator, right? Yet they behave differently if additionalProperties is not specified. So I think the problem is located elsewhere. The reason could be that java okhttp-gson uses this check: https://github.com/OpenAPITools/openapi-generator/blob/f409fb61848f71cb464f2caa7d15364703d311b7/modules/openapi-generator/src/main/resources/Java/libraries/okhttp-gson/additional_properties.mustache#L1 while spring uses a slightly different check: https://github.com/OpenAPITools/openapi-generator/blob/d27fa00cf55d3d7aae46bb9f8affbf33f057432e/modules/openapi-generator/src/main/resources/JavaSpring/additionalProperties.mustache#L1 I didn't look further into this though.

Does this mean that not much can be done without a complete rewrite? I'm not sure if it's worth changing if that's the case

We could at least improve the situation a bit by adding some code which verifies that incoming data complies with the expected data type.

Alternatively we could completely change the implementation for additionalProperties in okhttp-gson - not by rewriting it but instead by copying over the additionalProperties implementation from a different library or Java-based generator - e.g. spring.

Either way this seems to be independent of the originally discussed problem, so it can be addressed in a separate PR.

martin-mfg avatar May 07 '25 12:05 martin-mfg

https://github.com/OpenAPITools/openapi-generator/pull/21229 @martin-mfg, I was able to confirm. I found out that these libraries all use the same additional properties mustache file: jersey3, jersey2, feign, resttemplate, webclient, restclient, native After making the if-else change, I ran a DataProvider test with all the Java libraries, including the ones not listed above, and all the other ones failed the test, except feign-hc5 for some reason, but I reverted back to a single hard-coded value to avoid redundancy I also tested spring and it worked

DavidGrath avatar May 09 '25 05:05 DavidGrath

@DavidGrath Thanks for mentioning the PR again. I had overlooked it before. And thanks for creating a fix!

martin-mfg avatar May 11 '25 06:05 martin-mfg

@martin-mfg thank you, and you're welcome!

DavidGrath avatar May 11 '25 09:05 DavidGrath

Hello,

@martin-mfg, disallowAdditionalPropertiesIfNotPresent=false was indeed being used.

@DavidGrath, I have tested your branch and I think it is working properly now.

Here is a recap of the current behaviour using your branch :

generatorName additionalProperties value additionalProperties field/methods present Result
spring Not specified Yes, Map<String, Object> :heavy_check_mark: (OK thanks to your fix David)
spring additionalProperties: false No :heavy_check_mark:
spring additionalProperties: true Yes, Map<String, Object> :heavy_check_mark:
spring additionalProperties: type: string Yes, Map<String, String> :heavy_check_mark:
java Not specified Yes, Map<String, Object> :heavy_check_mark:
java additionalProperties: false No :heavy_check_mark:
java additionalProperties: true Yes, Map<String, Object> additionalProperties :heavy_check_mark:
java additionalProperties: type: string Yes, Map<String, Object> additionalProperties :x: Should be Map<String, String> additionalProperties, as with the spring generator

There is still a difference between spring/java and the additionalProperties: type: string, but this one is less important and is not really related to this ticket.

Thank you for your help and fixes !!

ncelerier avatar Jun 10 '25 07:06 ncelerier