Skip to content

Fix optional query param parsing #48

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
Sep 7, 2023

Conversation

czechboy0
Copy link
Contributor

@czechboy0 czechboy0 commented Sep 6, 2023

Motivation

Fixes apple/swift-openapi-generator#249.

Modifications

Adds a decodeIfPresent method on URIDecoder now used by the optional query param getter.

Result

This makes the query param getter return nil instead of throwing an error when it encounters a missing optional query param.

Test Plan

Added new unit tests, hopefully these cover it now.

@czechboy0
Copy link
Contributor Author

@swift-server-bot test this please

Copy link
Contributor

@tib tib left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you for the fix.

@czechboy0
Copy link
Contributor Author

@swift-server-bot test this please

@czechboy0
Copy link
Contributor Author

@tib thanks for the review. Have you also had a chance to try this branch to verify the issue is fixed for the cases you tested?

@czechboy0
Copy link
Contributor Author

@swift-server-bot test this please

@tib
Copy link
Contributor

tib commented Sep 7, 2023

@tib thanks for the review. Have you also had a chance to try this branch to verify the issue is fixed for the cases you tested?

Yes, I was able to verify it and the fix works just fine. If you could release this as a patch (0.2.1?) that'd be so nice. Thank you very much for the help. 🙏

@czechboy0
Copy link
Contributor Author

@tib thanks for the review. Have you also had a chance to try this branch to verify the issue is fixed for the cases you tested?

Yes, I was able to verify it and the fix works just fine. If you could release this as a patch (0.2.1?) that'd be so nice. Thank you very much for the help. 🙏

Will do, just wanted to make sure we really addressed the issue first. Thanks for the confirmation 🙏

@czechboy0 czechboy0 merged commit f0589f6 into apple:main Sep 7, 2023
@czechboy0 czechboy0 deleted the hd-fix-optional-query-params branch September 7, 2023 12:42
@czechboy0 czechboy0 added the 🔨 semver/patch No public API change. label Sep 7, 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.

Non-required query parameters throw errors during parsing instead of returning nil
4 participants