Update OpenAPIKit to v6#875
Conversation
…emaWarningsForwardedToGeneratorDiagnostics`
mattpolzin
force-pushed
the
update-openapi-kit-to-v5
branch
3 times, most recently
from
b61dbf0 to
4e5ff88
Compare
mattpolzin
force-pushed
the
update-openapi-kit-to-v5
branch
from
4e5ff88 to
7b35cbd
Compare
Co-authored-by: Mathew Polzin <matt.polzin@gmail.com>
…ted natively by OpenAPIKit.
…g OAS documents prior to 3.2.0 in the same manner for now
… that no Components Objects entries are references
This could be squashed into 'fix code that inspects UnresolvedSchemas for references' except for the fact that two commits tells the story better. The first commit fixes by looking into JSONSchema.references when they are found in UnresolvedSchemas in places where that is needed. This second commit cleans up by flattening the UnresolvedSchema so that we only need the inner of the two switch statements.
mattpolzin
force-pushed
the
update-openapi-kit-to-v5
branch
from
7b35cbd to
2113489
Compare
| // Also handle nil values in nullable schemas. | ||
| let isNullValue = anyValue is Void || (anyValue as? String) == nil | ||
| if isNullable && isNullValue { |
There was a problem hiding this comment.
Is this new check correct? Won't (anyValue as? String) be nil for non-nullable non-string values?
There was a problem hiding this comment.
I'm taking a closer look at this now. I inherited this change from the v4 bump.
non-nullable
From what I can tell, this possibility is ruled out because isNullable must be true on line 105.
non-string
We are in a case where the "backing type" has been determined to be a string, so I imagine that was the thinking with assuming the allowed values would be strings. I can say for sure that if the JSON Schema is of string type, then OpenAPIKit will special-case its parsing of allowedValues to only parse String (or String? if nullable) values. Tracing the calls in the generator project for translateRawEnum() I see that it does only pass .string as the backing type when OpenAPIKit has determined the schema type to be string, so this change actually should be safe and correct, albeit a little involved to sanity check!
|
Thanks for the review. I'll fix the CI failures in follow-up commits (formatting fixed just now). |
Specifically, Sources/_OpenAPIGeneratorCore/Translator/TypesTranslator/translateComponents.swift:38:54: error: generic parameter 'T' could not be inferred
|
@mattpolzin I've merged #887 now and updated this PR branch so 6.0 is no longer an issue. The fact the unit tests are not completing appears to be actually relevant to this PR as all the tests passed fine in #887. |
|
Troubling. This represents a massive performance regression. The examples CI jobs that do succeed before timing out still take close to twice as long on this branch as they did on the branch you linked to. Since it all compiles and works, I think I'll need to just make educated guesses at what might be slowing the compiler down and tweak those things. |
|
There's something really subtle going on here that I have not figured out yet. In the most recent CI run, Linux-based "Release Build" for Swift 6.2 finished in What's worse, I can't reproduce any of this on my Linux laptop inside a docker container (yet). Using a Swift 6.1 Docker Image I saw a |
|
I 'd like to come up with a troubleshooting process that doesn't involve me pushing a hopeful change and seeing if it works after CI runs in GitHub. |
|
Well, I've found something to go on I think. As a proxy for what's going on in CI (though the situation is not nearly as severe on my personal laptop) I can observe a meaningful difference in compile time between the Interestingly, over half the difference in build time can be observed going as far back as the bump to OpenAPIKit v4, the other half (or less) comes from bumping to OpenAPKit v5. I'm worried this may have less to do with function overloading slowing the compilation down and more to do with the introduction of concurrency (mainly |
|
@mattpolzin yeah, there's a bunch of abstractions in the CI that come from our use of a shared set of GitHub Workflows configs from another repo and the use of matrix jobs. But they really should boil down to just Worth noting that the compatibility tests require an environment variable to enable: Hopefully you have enough to work through locally. I wonder if it's worth having a look at some of the compiler flags, like Would be good if we can understand the impact here and then we can assess the blast radius. I don't think CI should be the issue, but if we can identify a pathological setup in CI that isn't representative of users then maybe we can change CI. |
|
This is not very promising news I'm afraid. I have bisected the performance degradation and the vast majority of it occurs between OpenAPIKit versions The numbers (on my personal laptop) of
On the plus side, compilation does not take very long given I'm not confident any good solution exists other than waiting for Swift to compile faster or removing concurrency (which I added to the external dereferencing code by request of Honza; and, it is definitely a good use-case for concurrency). Any thoughts based on the above? |
|
My first thought was add more type annotations. Annotating the async let lines with their types does not make any difference it doesn't look like. My next thought is conditional compilation -- at least until or unless a project wants to use OpenAPIKit to perform external dereferencing, the massive hit to compilation time may be avoidable. I'll experiment with that at least enough to know if it makes much of a difference like I think it might. |
|
Good news/Bad news. I added a trait that conditionally compiles the external dereferencing code and that was entirely the slowdown.
The bad news is that package traits are only available for Swift 6.1+ so to disable external loading we will need to bump The other bad news, naturally, is that Swift compilation of concurrency features is so slow that it might be considered prohibitive to enable an OpenAPIKit feature that would allow |
|
Oof. Thanks for the update, Matt! IIUC that means we have an incremental path forward. Swift OpenAPI Generator already is 6.1+ so we could take on a trait when bumping the OpenAPIKit dependency. I agree it's a shame it won't be as straightforward to investigate support for external references. We could explore using traits in Swift OpenAPI Generator too if the cost is prohibitively high to enable by default? |
|
It'll be on by default in OpenAPIKit because I don't want to assume that any given project will be driven by a desire to keep compilation time low. But I'll disable it when bringing v6 of OpenAPIKit into the generator project because that'll make it much easier to justify the update (CI won't take substantially longer or time out for you). I figure then it's up to y'all when you are ready to sink time into external loading features whether the compilation time cost is appropriate or not. |
|
Alright, I've bumped to OpenAPIKit v6 now. Major version 6 does not involve any breaking changes. It bumps the minimum Swift version to 6.1 (already this project's minimum requirement) and it encloses external loading features in an |
|
@simonjbeaumont looks like there's some sort of caching issue in CI maybe? The "Examples" job for Swift 6.1 succeeds (resolving OpenAPIKit to version 6.0.0) but the Examples job for Swift 6.2 fails (resolving OpenAPIKit to version 3.9.0). |
|
Looking at the examples CI, this looks to be because the examples share a common harness and, on each example, do a Let me see if I can get a PR up to fix that. |
…ed (#902) ### Motivation This project has CI for example projects and an integration test. These use a standalone package with a dependency on `swift-openapi-generator`. The scripts driving this CI used `swift package edit` to override this dependency to the checkout of the PR under test. Unfortunately this does not work in the presence of package traits. In open PR #875 the OpenAPIKit dependency is being bumped and is switched to disabling default traits. However, the CI is failing to perform the `swift package edit` command. The root cause is that `swift package edit` performs dependency resolution in order to edit the dependency. This resolves OpenAPIKit to a version satisfying the constraints on main -- a version of OpenAPIKit that has no traits. After the override, the build fails because SwiftPM forbids disabling traits on a package that has no declared traits. This is because OpenAPIKit is not re-resolved to the version declared in the local PR branch, even though it has been bumped. After quite some investigation, it appears there's no way to influence this behaviour or to defer resolution until after the dependency edit. This thesis aligns with the open SwiftPM issue tracking support for traits for `swift package edit`[^1] ### Modifications Workaround the lack of trait-aware `swift package edit` by updating the examples and integration test CI scripts to instead inject an override block into the package manifest. ### Result The scripts should now work as intended, even when the minimum version of a transitive dependency is bumped and/or when transitive dependencies make use of package traits. ### Test Plan Reproduced the original error in #875: ```console % git rev-parse --symbolic-full-name HEAD refs/heads/update-openapi-kit-to-v5 % SINGLE_EXAMPLE_PACKAGE=hello-world-urlsession-client-example ./scripts/test-examples.sh ... error: Disabled default traits by package 'swift-openapi-generator' (swift-openapi-generator) on package 'openapikit' (OpenAPIKit) that declares no traits. This is prohibited to allow packages to adopt traits initially without causing an API break. ``` With this patch applied we can run the script fine on the PR branch: ```console % git cherry-pick sb/fix-package-override-in-ci-scripts [update-openapi-kit-to-v5 2544c13] Update CI scripts to support dependency overrides where traits are used Date: Tue May 5 14:55:35 2026 +0100 2 files changed, 29 insertions(+), 18 deletions(-) % SINGLE_EXAMPLE_PACKAGE=hello-world-urlsession-client-example ./scripts/test-examples.sh ... ** Overriding swift-openapi-generator dependency in hello-world-urlsession-client-example to use /Users/Si/work/code/swift-openapi-generator ** Building example package: hello-world-urlsession-client-example ... Computing version for https://github.com/mattpolzin/OpenAPIKit Computed https://github.com/mattpolzin/OpenAPIKit at 6.0.0 (14.10s) ... Creating working copy for https://github.com/mattpolzin/OpenAPIKit Working copy of https://github.com/mattpolzin/OpenAPIKit resolved at 6.0.0 ... Build complete! (44.22s) ** ✅ Successfully built the example package hello-world-urlsession-client-example. ``` [^1]: swiftlang/swift-package-manager#8354
|
Thank you so much for driving this effort @mattpolzin and for your patience as we navigated through the changes. I think this is good to land now. ❤️ 🙏 |
|
Also—thank you to @iT0ny! |
…903) ### Motivation After updating to OpenAPIKit 6.0 (#875), the generator fails on OpenAPI documents that use `style: simple` on query parameters. While this is technically invalid per the OpenAPI specification, it was previously tolerated -- the generator would skip the unsupported parameter with a warning diagnostic and continue generation. The new OpenAPIKit 6.0 brings a new default built-in validation (`.parameterStyleAndLocationAreCompatible`) that rejects documents with this combination at validation time. Because this fires as a validation error (not a warning), it is thrown even with `strict: false`, before the translator gets a chance to handle the parameter gracefully. This is a regression for users with documents that have this common spec violation, which we do have examples of in our extended compatibility test suite). ### Modifications Replace the use of `Validator()` (which includes all OpenAPIKit defaults) with a custom validator built from `Validator.blank`. This explicitly lists all default validations from OpenAPIKit 6.0.0 except `.parameterStyleAndLocationAreCompatible`, restoring the previous behaviour where the generator tolerates these documents. We might want to revisit how lenient we are for this, but this patch is the most expeditious way to getting a release out with the OpenAPIKit update. ### Result Documents with `style: simple` on query parameters are no longer rejected at validation time. The translator continues to emit an "unsupported" diagnostic and skip the parameter, as it did before the OpenAPIKit upgrade: ``` 'Feature "Query params of style simple, explode: false" is not supported, skipping' ```
3 participants
This PR builds upon #839 to update from OpenAPIKit 4.x to OpenAPIKit
5.06.0.Motivation
Updating to OpenAPIKit v4.x enables efforts to add external loading support to the generator (as a separate future project). This means being able to read in OpenAPI Documents spread across multiple files.
Updating to OpenAPIKit v5.x allows the generator project to support OAS 3.2.0 features and take advantage of various other improvements to OpenAPIKit that required breaking changes to be made.
Updating to OpenAPIKit v6.0 allows this project to turn the
ExternalLoadingtrait off (on by default). Turning this trait off saves 50% of the compile time of OpenAPKit for as long asswift-openapi-generatoris not using the external loading features of OpenAPKit anyway.Modifications
assumeLookupOnce()to temporarily maintain the existing invariant that no entries in the Components Object are themselves references; replace alllookup()calls withassumeLookupOnce(). See the Note on this below.OpenAPI.Reference. This assumption does not hold in OpenAPIKit v5.NOTE on
assumeLookupOnce()The new
assumeLookupOnce()function is intended to allow for gradual migration. The generator had previously usedlookup()which used to look at the Components Object and either return an entry or throw. Going forward,lookup()does recursive lookup until a non-reference is found andlookupOnce()acts more likelookup()used to act -- however,lookupOnce()has a different method signature owing to the fact that it may return a reference instead of the ultimately desired object. The need for these changes arise from the fact that OpenAPIKit v5 now allows entries in the Components Object to themselves be references. This has been supported by the OpenAPI Standard for some time but OpenAPIKit only introduced support for it with v5.Therefore,
assumeLookupOnce()was added as a way to throw an error if a reference is found as an entry in the Components Object (behavior already exhibited by the generator project) but the goal should be to remove uses ofassumeLookupOnce()over time and replace them with eitherlookupOnce()orlookup()depending on the needs of each call site. In this way, the generator project will move closer to supporting Component Object reference entries without the overhead of changing every call site all at once.Result
Theoretically, nothing functionally changes.
Test Plan
Tests updated,
swift testpassing, "Compatibility test" suite passing.