Skip to content
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

Libs(all): bump openapi generator from 5.2.0 to 7.9.0 #1480

Merged
merged 53 commits into from
Oct 24, 2024

Conversation

svix-onelson
Copy link
Contributor

@svix-onelson svix-onelson commented Oct 8, 2024

With the existing generator versions, we've had issues during codegen for certain cases in the svix spec including:

  • serialization of nullable fields
  • enum (oneOf, anyOf) representations

Bumping the generators all the way up to the latest version (at time of writing, 7.9.0) seems to resolve both of these, at least partially.

Some refactoring of extant template overrides has been required to "re-seat" our local patches on top of the newer templates.

There have also been cases of the generated symbol names using slightly different schemes than in earlier generator versions. The high-level API had to be updated to use the new names.

Approach

Customization to the codegen comes in 2 flavors:

  • edits to templates.
  • manual edits where the codegen is checked-in to git and the paths are recorded in an .openapi-generator-ignore file.

Templates are better because they don't hide the customization as readily, but we've got a mix of these approaches throughout.

At a high-level, this change followed the following steps:

  • replace templates entirely with the version tagged for 7.9.0.
  • recreate our local patches on top as a single rev.
  • leave behind any customization made moot by upstream refactors.
  • try to remove as many manual edits/ignores by migrating into templates where needed or just omitting the change entirely.
  • add a light/gentle test to check specific behaviors things we had hacks in place for:
    • nullable containers (ie. Endpoint channels and filter types).
    • responses with empty bodies.

In addition to doing the "upstream merge" on template sources, I also had to sometimes look at the dependencies a lib depends on, upgrading or changing the package manifest, or build tooling.

When refering/comparing to the upstream templates:

Status

Each lib came with its own challenges. Some I have higher or lower confidence in.
I'll call out the ones that are weaker so reviewers can give more scrutiny.

Ruby: I wasn't able to get my local ruby setup in a healthy enough state to follow along with the readme instructions for building/testing.
This one needs a so-called "kitchen sink" test like I added to the other libs, and may need some dependencies bumped. Normally the dep updates required become known during testing.

Edit: setting BUNDLE_PATH=$HOME/.gems unblocked me. Apparently bundle install tries to put stuff on the "system" level by default (i.e. requiring sudo, and not really what I wanted anyway). With this, I was able to learn enough Ruby to get a test written, and also verify that the deps are lined up as needed. Ruby seems to be in good shape!

Python: This was the last lib I worked on, and I ran out of time. The "kitchen sink" test looks good, and we also have a decent set of tests that give me okay confidence but here's where I cut corners. I left the templates, manual edits, dependencies and the rest all as-is in the hopes that the boon of the tests will surface problems that stem from all the surrounding code being updated around this stuff that stays where it is.

The tests are passing, so maybe we're good, but we should loop back and try to do proper updates to this one.

C#: manual edits for nullability were left in place for now. A "kitchen sink" test was written, but has also been reverted because however I wrote it ended up using language features not supported by dotnet 5.0 (which our CI process is targeting).

The tests pass locally when I update the solution to target 8.0, so I feel good about the lib generally. I was not able to figure out how to install dotnet 5.0 on my system since it's beyond End of Life. We should look to update our target to a currently supported dotnet like 8.0, at which point we can un-revert the test!

Kotlin: had to update the version of kotlin we build with, but other than that I think things look good. There's a fixme in the ApiClient template about updating the retry loop to work for 2 flavors of request sending; I'd like to look at that in a follow-up, later.

The rest all seemed okay and not very concerning to me.

Verification

The so-called "kitchen sink" tests added to each lib follow a pattern of auto-skipping by default and running only when the env vars SVIX_TOKEN and SVIX_SERVER_URL are set. The test will then step through creating an app and an endpoint, patching the endpoint, and finally deleting the endpoint.

Currently these tests are not setup to run in CI, but they could be if we are able to automate raising the docker-compose stack, generating a valid token, and setting the env vars such that they are available to each "lint" or "test" workflow.

For now, these tests have been seen to work locally on my machine, and hopefully yours as well.

Notes for Reviewers

I can offer 2 bits of advice when reviewing this chungus of a diff.

  1. I split this into many commits and it may be helpful to review one by one.
  2. Many files such as the templates themselves may be easier to read if you "ignore whitespace" (at your discretion).

It would be good if someone knowledgeable in Ruby could actually build the package and add a similar "kitchen sink" test before this merges, but I don't know if @svix-gabriel is willing to dive into this rabbit hole with me. Please and thank you, Gabriel!

Good luck!

@svix-onelson svix-onelson force-pushed the onelson/generator-bump branch 14 times, most recently from 6f4f88c to 085b3f3 Compare October 17, 2024 04:45
@svix-onelson svix-onelson marked this pull request as ready for review October 17, 2024 06:06
@svix-onelson svix-onelson requested a review from a team as a code owner October 17, 2024 06:06
Copy link
Member

@svix-jplatte svix-jplatte left a comment

Choose a reason for hiding this comment

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

Nice work!

java/lib/src/main/java/com/svix/SvixOptions.java Outdated Show resolved Hide resolved
java/lib/src/test/java/com/svix/KitcheSinkTest.java Outdated Show resolved Hide resolved
javascript/templates/api/api.mustache Outdated Show resolved Hide resolved
javascript/templates/http/http.mustache Outdated Show resolved Hide resolved
rust/.openapi-generator-ignore Outdated Show resolved Hide resolved
This brings us up to the latest generator version without updating the
templates at all.

Quite possibly, codegen will be broken in this state.
Removes the sed we ran to remove some unused imports in the sink models.
The hope is we no longer need this with the updated generators.
The new codegen litters symbol names with "period" where a dot appears
in the operation id.

There also seems to be some slight differences in how symbols refer to
each other, so the imports have been tweaked in the template.
We had a local patch on an older version of this template to add support
for enums with integer reprs.

Seems like this is provided by default today, however the imports in the
default version of the template will cause a bunch of unused import
warnings.

This diff adds the v7.9.0 template as-is, and small mods will be made in
a future rev to fix the warnings.
This might not strictly be the best option, but it'll get the build to
be green. Currently these warnings are the only thing standing between
us and a clean build.
I guess this is included in the module as a documentation resource or
something.

No sense in tracking it in version control.
Adding:
- retries
- `svix-req-id` header
The description field for event type is required.

Seems like the "default zero value" for the string fields was
incorrectly accepted by the server.

Realized there was an issue when the server responded 422 on description-free
event types when I ported this test to Java. The Java models must not have
sent empty strings like Go might.
All the work in this "bump generator" branch so far has been using a
pre-release version of the spec to figure out how it handles some newer
features that previous generator versions stuggled with.

Before merging back to main, let's reset the spec to make the merge a
little cleaner.

The newer stuff will need high-level API added before it's usable
anyway.
In `wrapError`, the 7.9.0 generator code was not wrapping internal
errors in the same way. Seems like we have to dereference the error now
where we didn't before.
Superlinter looks like it must have started linting with v3.3.3 of
prettier so it's flagging things in CI that didn't get flagged locally.
Looks like we need this for tests.
This test is like the others added across our other libs. It walks
through some light API interactions just to check how the serialization
is working WRT nullable containers and empty response bodies.

Setting `SVIX_TOKEN` and `SVIX_SERVER_URL` env vars will optionally
enable the test which is skipped otherwise.
@svix-onelson
Copy link
Contributor Author

Superlinter seems like it sprung some new lints on me today, which had to be addressed by updating our prettier version to match.

Re-requested review now that the whitespace stuff has been resolved and also I got the ruby lib verified. I think this is good to go, unless more whitespace/formatting things pop out.

@svix-onelson svix-onelson requested a review from a team October 24, 2024 18:13
@svix-onelson svix-onelson merged commit 141d3cd into main Oct 24, 2024
15 checks passed
@svix-onelson svix-onelson deleted the onelson/generator-bump branch October 24, 2024 23:23
svix-onelson added a commit that referenced this pull request Oct 29, 2024
Work was done previously [1] to make nullable containers (arrays, maps)
generate as refs. This avoided a problem where the JSON serialization
code incrrectly sent an empty array for cases where a field was left
unset. This would result in requests incorrectly failing with a 422
response when it should have been OK.

During the openapi generator update [2], the resulting codegen no longer
had the 422 issue since it correctly omitted empty arrays from the body
for PATCH requests. However, this now means we have no way to explicitly
set a field to `null`. This specific issue was fixed in the some other langs
with the work done to update the generator, but the Go generator seems to
have fallen behind.

This rev adds tests to demonstrate the shortfall. We may need to rework
the template patch made for the old 5.x generator to solve this in the
meantime.

- 1: #1450
- 2: #1480
svix-onelson added a commit that referenced this pull request Nov 7, 2024
Fixes #1505

Big thanks to @ZONGMENG-kaito for the report.

During the work on #1480
various dependencies were updated to match what is being produced by the
upstream code generator. Unfortunately, the version spec change for
`@stablelib/base64` up to 2.x means we inherit their ESM requirement.

The local tests (i.e. `SVIX_SERVER_URL=... SVIX_TOKEN=... yarn test`)
continue to pass with the downgrade, so it seems like pinning to 1.x for
now will free consumers of _our lib_ of this new requirement for now.

Hopefully in the future we'll find a way to track 2.x without
disruption.
svix-onelson added a commit that referenced this pull request Nov 7, 2024
…1506)

Fixes #1505

Big thanks to @ZONGMENG-kaito for the report.

During the work on #1480
various dependencies were updated to match what is being produced by the
upstream code generator. Unfortunately, the version spec change for
`@stablelib/base64` up to 2.x means we inherit their ESM requirement.

The local tests (i.e. `SVIX_SERVER_URL=... SVIX_TOKEN=... yarn test`)
continue to pass with the downgrade, so it seems like pinning to 1.x
will free consumers of _our lib_ of this new requirement for now.

Hopefully in the future we'll find a way to track 2.x without
disruption.
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.

4 participants