-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Upgrade OpenAPI Generator to 6.1.0-SNAPSHOT #16026
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! great working, getting this done with the openapi generator people. I also appreciate the comments explaining why we've taken the approach we have.
airbyte-server/src/main/java/io/airbyte/server/handlers/WebBackendConnectionsHandler.java
Outdated
Show resolved
Hide resolved
@@ -22,7 +24,7 @@ task generateApiServerLegacy(type: GenerateTask) { | |||
invokerPackage = "io.airbyte.api.invoker.generated" | |||
modelPackage = "io.airbyte.api.model.generated" | |||
|
|||
importMappings = [ | |||
schemaMappings = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a change in the plugin syntax?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pmossman ! Great stuff getting this in!
Just to make sure I understand, the non-api related generation code here is adjusting things to conform to our new behaviour of a null field = not set, and an empty field = set to empty. Is that right?
Yes @davinchia that's right! |
What
Upgrade our version of openapi-generator to the latest, so that we can take advantage of the fix we sponsored here: OpenAPITools/openapi-generator#13025
The core change in this version of openapi-generator is that schemas with optional array fields are now instantiated in Java as
null
instead ofnew ArrayList<>()
. We need this new behavior to implement PATCH update requests, so that clients can leave array fields asnull
to indicate that they should be left unchanged.This unblocks our issue to make the Connection Update endpoint use PATCH semantics: https://github.com/airbytehq/airbyte-cloud/issues/2064
How
Note to reviewers:
config.yaml
untouched, but there are some cases where I could see an argument for marking an array field asrequired
, which would force it to be generated as an emptyArrayList
like before. For example, ourConnectionRead
currently specifiesoperationIds
as an optional array field. Does this make sense? Or would it be clearer to actually specifyoperationIds
asrequired
, since it isn't really clear whatnull
means when reading aConnection
. Perhaps it would always be clearer to have an explicit empty array unless otherwise specified.