Skip to content

[Fix] Accept additional properties in API JSON responses #95

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

jhamon
Copy link
Contributor

@jhamon jhamon commented Apr 5, 2024

Problem

When new properties are added to API responses, this causes the Java client to error. This prevents us from making additive API changes that would otherwise be non-breaking to the end user.

Solution

  • Add tests that exercise the JSON parsing behavior in src/test/java/org/openapitools/client/JsonParsingTest.java and ensure the presence of additional properties will not cause the SDK to error. These tests use JSON fixtures added in src/test/resources.
  • Regenerate the control plane rest client using the OpenAPI java client generator. This time, configuring the generator with disallowAdditionalPropertiesIfNotPresent=false should activate the correct JSON parsing behavior in the generated code.

How the code was generated

The code generation step was performed with this command:

mkdir codegen
mkdir codegen/gen
cd codegen

docker run --rm -v $(pwd):/workspace openapitools/openapi-generator-cli:v7.0.1 generate \
    --input-spec $SPEC_FILE \
    --additional-properties=dateLibrary='java8',disallowAdditionalPropertiesIfNotPresent=false \
    --generator-name java \
    --output /workspace/gen/java

## We want to replace all generated files in src, so remove the old ones
rm -rf ../src/main/java/org/openapitools/client

# We don't want an entire generated project structure, just the java src files.
cp -r gen/java/src/main/java/org/openapitools/client ../src/main/java/org/openapitools/client

It took some trial and error to discover an openapi generator version and spec version that produced similar results to the ones Rohan created previously using steps and commands that were not captured anywhere. These steps will be scripted and automated in a future PR to remove this element of guesswork.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Test Plan

Since the new tests exercise all the generated code up to the boundary where a response is returned from the okhttp library used for making requests, that gives a high confidence this will work if unexpected fields appear in future API responses.

@jhamon jhamon force-pushed the jhamon/regen-openapi branch from 37ccd55 to d9222ac Compare April 6, 2024 02:53
Copy link
Contributor

@austin-denoble austin-denoble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for getting this resolved and adding tests! The logic around the flag you needed to pass is really gnarly.

@@ -0,0 +1,116 @@
package org.openapitools.client;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this issue basically forces it, but good idea for adding a testing section focused on some of the generated bits. 👍

"status": {
"ready": true,
"state": "Ready",
"reticulating_splines": true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol 😭

@jhamon jhamon merged commit 12318d7 into main Apr 9, 2024
@jhamon jhamon deleted the jhamon/regen-openapi branch April 9, 2024 15:26
jhamon added a commit that referenced this pull request Apr 9, 2024
When new properties are added to API responses, this causes the Java
client to error. This prevents us from making additive API changes that
would otherwise be non-breaking to the end user.

- Add tests that exercise the JSON parsing behavior in
`src/test/java/org/openapitools/client/JsonParsingTest.java` and ensure
the presence of additional properties will not cause the SDK to error.
These tests use JSON fixtures added in `src/test/resources`.
- Regenerate the control plane rest client using the OpenAPI [java
client generator](https://openapi-generator.tech/docs/generators/java/).
This time, configuring the generator with
`disallowAdditionalPropertiesIfNotPresent=false` should activate the
correct JSON parsing behavior in the generated code.

The code generation step was performed with this command:

```bash
mkdir codegen
mkdir codegen/gen
cd codegen

docker run --rm -v $(pwd):/workspace openapitools/openapi-generator-cli:v7.0.1 generate \
    --input-spec $SPEC_FILE \
    --additional-properties=dateLibrary='java8',disallowAdditionalPropertiesIfNotPresent=false \
    --generator-name java \
    --output /workspace/gen/java

rm -rf ../src/main/java/org/openapitools/client

cp -r gen/java/src/main/java/org/openapitools/client ../src/main/java/org/openapitools/client
```

It took some trial and error to discover an openapi generator version
and spec version that produced similar results to the ones Rohan created
previously using steps and commands that were not captured anywhere.
These steps will be scripted and automated in a future PR to remove this
element of guesswork.

- [x] Bug fix (non-breaking change which fixes an issue)

Since the new tests exercise all the generated code up to the boundary
where a response is returned from the okhttp library used for making
requests, that gives a high confidence this will work if unexpected
fields appear in future API responses.
jhamon added a commit that referenced this pull request Apr 9, 2024
When new properties are added to API responses, this causes the Java
client to error. This prevents us from making additive API changes that
would otherwise be non-breaking to the end user.

- Add tests that exercise the JSON parsing behavior in
`src/test/java/org/openapitools/client/JsonParsingTest.java` and ensure
the presence of additional properties will not cause the SDK to error.
These tests use JSON fixtures added in `src/test/resources`.
- Regenerate the control plane rest client using the OpenAPI [java
client generator](https://openapi-generator.tech/docs/generators/java/).
This time, configuring the generator with
`disallowAdditionalPropertiesIfNotPresent=false` should activate the
correct JSON parsing behavior in the generated code.

The code generation step was performed with this command:

```bash
mkdir codegen
mkdir codegen/gen
cd codegen

docker run --rm -v $(pwd):/workspace openapitools/openapi-generator-cli:v7.0.1 generate \
    --input-spec $SPEC_FILE \
    --additional-properties=dateLibrary='java8',disallowAdditionalPropertiesIfNotPresent=false \
    --generator-name java \
    --output /workspace/gen/java

rm -rf ../src/main/java/org/openapitools/client

cp -r gen/java/src/main/java/org/openapitools/client ../src/main/java/org/openapitools/client
```

It took some trial and error to discover an openapi generator version
and spec version that produced similar results to the ones Rohan created
previously using steps and commands that were not captured anywhere.
These steps will be scripted and automated in a future PR to remove this
element of guesswork.

- [x] Bug fix (non-breaking change which fixes an issue)

Since the new tests exercise all the generated code up to the boundary
where a response is returned from the okhttp library used for making
requests, that gives a high confidence this will work if unexpected
fields appear in future API responses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants