Skip to content

Fix OpenAPIValueContainer serialization of nested values #25

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 7 commits into from
Jul 17, 2023

Conversation

czechboy0
Copy link
Contributor

Motivation

Credit goes to @rboyce for discovering this bug!

When I saw the PR #24, I realized we didn't have any unit tests for the OpenAPI*Container types, which was an oversight.

This PR adds those missing unit tests, and fixes the bug in casting that motivated #24.

Modifications

Adds unit tests and fixes the casting bug that expected wrapped, instead of raw stdlib values.

Result

Now all the new unit tests pass and serializing nested values in OpenAPIValueContainer and friends works correctly.

Test Plan

Most of this PR is new unit tests, verified they pass locally.

@czechboy0 czechboy0 requested a review from simonjbeaumont July 14, 2023 09:45
@czechboy0
Copy link
Contributor Author

@rboyce once this PR lands, please rebase your PR and add the great unit tests you had there to the basic ones I added in this PR. 🙏

@czechboy0
Copy link
Contributor Author

@simonjbeaumont let's get #22 landed first, then I'll update this PR, and then a subset of #24 with just the additional unit tests.

@czechboy0
Copy link
Contributor Author

Investigating the integration test failure, we're seeing behavior that I don't know whether it's desired or a bug in SwiftPM.

When we enable the existential any feature in the runtime library, it also forces that feature on in the integration test package, which depends on the runtime library. That'd mean that enabling an upcoming feature in an upstream package forces it on all downstream packages?

Will try to create a reproduction case to see if we're just doing something wrong. But tagging @neonichu in case this is known.

@neonichu
Copy link
Member

@DougGregor may know more here

@DougGregor
Copy link
Member

@DougGregor may know more here

That's not supposed to be the case. Upcoming features should only affect the target, not downstream targets.

@czechboy0
Copy link
Contributor Author

czechboy0 commented Jul 14, 2023

@neonichu @DougGregor I spent a few hours bisecting this using toolchains from Swift.org, and it seems to have been fixed in 5.9 between 04-26 and 04-27. Before it, including 5.8.1, this bug is occurring. After, it's not. Unfortunately, 5.8.1 will need to be supported for a few more months in this project, so I wonder if there's a chance of a cherry pick + 5.8.2?

swift-package-manager and swift-driver don't have any changes that look related to me, but the main Swift repo has stuff related to existential any, which is the feature we're seeing "leaked" from an upstream to downstream dependees.

swiftlang/swift@swift-5.9-DEVELOPMENT-SNAPSHOT-2023-04-26-a...swift-5.9-DEVELOPMENT-SNAPSHOT-2023-04-27-a

@czechboy0
Copy link
Contributor Author

To recap - what we're seeing on these older Swift versions is that when we enable the ExistentialAny feature in the upstream Runtime package, it affects a downstream integration test package, which consumes the Runtime library.

When we disable it in Runtime, it goes back to working.

@czechboy0
Copy link
Contributor Author

API breakage is the known sendable stuff

2 breaking changes detected in OpenAPIRuntime:
  💔 API breakage: constructor OpenAPIObjectContainer.init(unvalidatedValue:) has parameter 0 type change from [Swift.String : Any?] to [Swift.String : (Swift.Sendable)?]
  💔 API breakage: constructor OpenAPIArrayContainer.init(unvalidatedValue:) has parameter 0 type change from [Any?] to [(Swift.Sendable)?]
** ERROR: ❌ Breaking API changes detected.

@czechboy0 czechboy0 requested a review from simonjbeaumont July 17, 2023 10:48
@czechboy0 czechboy0 merged commit c5ab4e8 into apple:main Jul 17, 2023
@czechboy0 czechboy0 deleted the hd-fix-value-container branch July 17, 2023 11:28
@czechboy0 czechboy0 added 🆕 semver/minor Adds new public API. 🔨 semver/patch No public API change. and removed 🆕 semver/minor Adds new public API. labels Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants