OpenAPITools / openapi-generator

OpenAPI Generator allows generation of API client libraries (SDK generation), server stubs, documentation and configuration automatically given an OpenAPI Spec (v2, v3)
https://openapi-generator.tech
Apache License 2.0
21.29k stars 6.44k forks source link

[BUG][JAVA] IllegalArgumentException while parsing responses with unknown fields ❗ #12550

Open jaaufauvre opened 2 years ago

jaaufauvre commented 2 years ago

Bug Report Checklist

Description

API client libraries generated with OpenAPI Generator should ignore response fields that were not existing at the time the code was generated, instead of raising the following error:

java.lang.IllegalArgumentException: The field `updated` in the JSON string is not defined in the `Dog` properties. JSON: {"name":"Luna","created":"2021-04-23T17:32:28Z","updated":"2021-04-23T17:32:28Z"}
openapi-generator version
Version Status
OpenAPI Generator 5.4.0 ✔️
OpenAPI Generator 6.0.0
OpenAPI Generator 6.0.1-SNAPSHOT
OpenAPI declaration file content or url

Download ⤓

Generation Details
{ 
    "groupId" : "com.acme.app",
    "artifactId" : "sample-api-client",
    "artifactVersion" : "1.0.0",
    "invokerPackage" : "com.acme.app",
    "apiPackage" : "com.acme.app.api",
    "modelPackage" : "com.acme.app.model"
}
Steps to reproduce
  1. Generate a client from sample-v1.yaml:
    java -jar openapi-generator-cli-6.0.0.jar generate -g java -i sample-v1.yaml -c config.json -o api_client_6.0
  2. Update DogsApiTest:

    
    public class DogsApiTest {
    
    @Test
    public void addDogTest() throws ApiException {
        // GIVEN
        ApiClient client = new ApiClient();
        client.setDebugging(true);
        client.setBasePath("http://localhost:4010");
        NewDog newDog = new NewDog()
                .name("Bella");
    
        // WHEN
        Dog dog = new DogsApi(client).addDog(newDog);
    
        // THEN
        assertNotNull(dog);
    }

}

3. Run `prism.exe mock sample-v1.yaml`
4. Run `addDogTest` => ✔️
5. Run `prism.exe mock sample-v2.yaml`
4. Run `addDogTest` => ❌

java.lang.IllegalArgumentException: The field updated in the JSON string is not defined in the Dog properties. JSON: {"name":"Luna","created":"2021-04-23T17:32:28Z","updated":"2021-04-23T17:32:28Z"}

at com.acme.app.model.Dog.validateJsonObject(Dog.java:217)
at com.acme.app.model.Dog$CustomTypeAdapterFactory$1.read(Dog.java:253)
at com.acme.app.model.Dog$CustomTypeAdapterFactory$1.read(Dog.java:243)
at com.google.gson.TypeAdapter$1.read(TypeAdapter.java:199)
at com.google.gson.Gson.fromJson(Gson.java:991)
at com.google.gson.Gson.fromJson(Gson.java:956)
at com.google.gson.Gson.fromJson(Gson.java:905)
at com.acme.app.JSON.deserialize(JSON.java:151)
at com.acme.app.ApiClient.deserialize(ApiClient.java:845)
at com.acme.app.ApiClient.handleResponse(ApiClient.java:1053)

...

carmenquan commented 2 years ago

@jaaufauvre Configuring disallowAdditionalPropertiesIfNotPresent to false fixes this issue for me. Still testing to see what other fallouts come from setting this, and not sure why the change surfaced in the latest version.

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

jaaufauvre commented 2 years ago

This is a concerning one, because a client generated with OpenAPI Generator 6.0.x is likely to break as soon as new fields are added to responses. In addition to that, it makes it impossible to remove deprecated fields from an API specification.

sergiofigueras commented 2 years ago

Any news on this, folks?

Rugal commented 2 years ago

Following this issue, generating kotlin client with gradle is having the same issue. The code is enforcing all properties to be define in contract, that we do not need in this client implementation at all.
Need a method to ignore properties not defined in contract while actually send back to client.

aaron-613 commented 2 years ago

Very related to this (I ran into the exact same issues as the OP). Including the additional config file of {"disallowAdditionalPropertiesIfNotPresent":false} worked to not include the block of validation code throwing IllegalArgumentExceptions. So that's good! Thanks for documenting this issue and workaround..! 🙏🏼

My problem came right after that, because my unknown/additional field is an array... and the code at the bottom of the generated class (the TypeAdapter<T> read() method, that tries to store additional fields expects it to be either a String, Number, Boolean, or Object. Not arrays:

// store additional fields in the deserialized instance
Consumer instance = thisAdapter.fromJsonTree(jsonObj);
for (Map.Entry<String, JsonElement> entry : jsonObj.entrySet()) {
  if (!openapiFields.contains(entry.getKey())) {
    if (entry.getValue().isJsonPrimitive()) { // primitive type
      if (entry.getValue().getAsJsonPrimitive().isString())
        instance.putAdditionalProperty(entry.getKey(), entry.getValue().getAsString());
      else if (entry.getValue().getAsJsonPrimitive().isNumber())
        instance.putAdditionalProperty(entry.getKey(), entry.getValue().getAsNumber());
      else if (entry.getValue().getAsJsonPrimitive().isBoolean())
        instance.putAdditionalProperty(entry.getKey(), entry.getValue().getAsBoolean());
      else
        throw new IllegalArgumentException(String.format("The field `%s` has unknown primitive type. Value: %s", entry.getKey(), entry.getValue().toString()));
    } else { // non-primitive type
      instance.putAdditionalProperty(entry.getKey(), gson.fromJson(entry.getValue(), HashMap.class));  <-- Assumes JSON Object?
    }
  }
}

Exception:

java.lang.IllegalStateException: Expected BEGIN_ARRAY but was BEGIN_OBJECT at path $[0]
com.google.gson.JsonSyntaxException: java.lang.IllegalStateException: Expected BEGIN_ARRAY but was BEGIN_OBJECT at path $[0]
    at com.google.gson.Gson.fromJson(Gson.java:944)
    at com.google.gson.Gson.fromJson(Gson.java:1003)
    at com.google.gson.Gson.fromJson(Gson.java:975)
    at community.solace.ep.client.model.Consumer$CustomTypeAdapterFactory$1.read(Consumer.java:518)    <-- that's the line above with the HashMap.class

Dang! I guess I'll be raising an Issue for this!

EDIT: if I manually add these two lines into that method, everything works. So I guess I know what needs to be updated in the code generation template:

} else if (entry.getValue().isJsonArray()) {
    instance.putAdditionalProperty(entry.getKey(), gson.fromJson(entry.getValue(), ArrayList.class));
janisz commented 1 year ago

Looks like it's solved in latest release.

mas-chen commented 1 year ago

I also encountered this nasty bug and looks like it is fixed via this commit: https://github.com/OpenAPITools/openapi-generator/pull/13630

The 6.4.0 upgrade and configOption mentioned above are what fixed it for me.

IMO, the configOption needs to be set to false as default in the next major release. Thoughts?

FloCoop commented 1 year ago

With 6.6.0 we still have to set configOptions disallowAdditionalPropertiesIfNotPresent to false. Will this issue ever get fixed for compatibility with the old Version 5.4.0?

ajrice6713 commented 1 year ago

Running into this issue on 6.4.0 without setting the disallowAdditionalPropertiesIfNotPresent config option to false - wanted to chime in and say that this config option should be false as the default, its perfectly acceptable for a REST API to add properties to a response body, this is non breaking and clients should ignore anything they don't recognize. AFAIK the other languages don't behave in this way.

anm-cb commented 1 year ago

With 6.6.0 disallowAdditionalPropertiesIfNotPresent doesn't change anything at all for me with the default library. (setting in gradle)

thejeff77 commented 1 year ago

We were able to get this to work on 6.6.0 with disallowAdditionalPropertiesIfNotPresent set. However I do agree with @ajrice6713 that this should be false by default, the claim in the documentation that this should be true by default and this is the new OAS compliant standard seems mis-guided.

A client shouldn't break if there is additional data in the response that it doesn't understand.

nicolas-chamand commented 3 weeks ago

I kinda feel what you are saying but I don't agree. An API is meant to be like a contract.

I see it like a type defined method. If you add additional parameters to the method call, would you expect it to run exactly as if there were the correct number of parameters ?

"A client shouldn't break if there is additional data in the response that it doesn't understand." If your API has changed, then the contract is broken. Even though it may work with given parameter on client call, you can't consider that the call was successful as it didn't respect the contract. That's the idea I think behind the disallowAdditionalPropertiesIfNotPresent being true by default.

I saw someone saying "it makes it impossible to remove deprecated fields from an API specification". As soon as you edit your API, it's another version. Now, up to you to keep the old version available for client in which case they would still be able to use the old specification as a contract. Otherwise, they would need to switch to the new API specification and that's expected. To be honest, I don't get the problem here, server and client should respect one thing, the contract made by the OpenAPI specification.

thejeff77 commented 3 weeks ago

The default behavior of every other client I've encountered in every language will read available fields and ignore the rest.

Removing a field is a different story, that isn't backwards compatible.

This is admittedly a bandwagon argument, but in my opinion an effective one.

cb-suryak commented 1 day ago

with 6.6.0 too, I am still getting this error.

I see still it checks for unknown params and throws error.

public static void validateJsonObject(JsonObject jsonObj) throws IOException {
        if (jsonObj == null && !openapiRequiredFields.isEmpty()) {
            throw new IllegalArgumentException(String.format("The required field(s) %s in XXX is not found in the empty JSON string", openapiRequiredFields.toString()));
        } else {
           ....
           ....

                if (!openapiFields.contains(entry.getKey())) {
                    throw new IllegalArgumentException(String.format("The field `%s` in the JSON string is not defined in the `XXX` properties. JSON: %s", entry.getKey(), jsonObj.toString()));
                }
            }

I have set the config options as below What am I missing here folks?

 configOptions = [
                    disallowAdditionalPropertiesIfNotPresent: 'false',
                    library: 'okhttp-gson'
            ]. //it is not making any difference.
thejeff77 commented 1 day ago

The reasoning in the documentation is incorrect and a mis-interpretation of the OAPI specification. Nowhere in the specification for 3.1.0 does it state that additional properties are not allowed, with the exception of defining a #ref... Which makes sense since your model is defined elsewhere - you shouldn't be able to put properties next to it..

This totally breaks the tolerant reader pattern in a way that defies all previous precedent as far as OAPI, API contracts, and every database ORM library I've ever seen - I.E. tolerate extra data and ignore it.

Screenshot 2024-09-12 072302