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

Retain operation level parameters over path level ones #183

Merged
merged 8 commits into from
Aug 11, 2023

Conversation

arthurcro
Copy link
Contributor

@arthurcro arthurcro commented Aug 9, 2023

Motivation

Fixes #168.

Modifications

Previously, an operation's parameters were returned as a concatenation of the parameters from the path item level with those at operation level.

However, this is incorrect according to https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#fixed-fields-7.

A list of parameters that are applicable for all the operations described under this path. These parameters can be overridden at the operation level, but cannot be removed there. The list MUST NOT include duplicated parameters.

With this change, we are merging the two arrays of parameters using a unique identifier of the location + name of parameters. If duplicate parameters exist, only the parameters from the operation level are preserved.

Result

After this change, an operation's parameters won't contain any duplicate.

Test Plan

Add tests for OperationDescription.allParameters at Tests/OpenAPIGeneratorCoreTests/Translator/Operations/Test_OperationDescription.swift.

@czechboy0
Copy link
Contributor

The change looks good, just a few minor style suggestions.

For testing, you can create a new unit test file Tests/OpenAPIGeneratorCoreTests/Translator/Operations/Test_OperationDescription.swift, follow the style seen e.g. in Tests/OpenAPIGeneratorCoreTests/Translator/Content/Test_ContentType.swift, and create an OperationDescription with duplicated params in the path and operation in the unit test and verify that allParameters returns just the unique ones.

Co-authored-by: Honza Dvorsky <[email protected]>
@arthurcro
Copy link
Contributor Author

Thank you for the nice review, I applied all suggestions! 🙂
I'll add a test shortly and mark this as ready for review.

@arthurcro arthurcro marked this pull request as ready for review August 10, 2023 19:17
@czechboy0
Copy link
Contributor

@swift-server-bot add to allowlist

@arthurcro arthurcro requested a review from czechboy0 August 11, 2023 07:10
@arthurcro arthurcro force-pushed the improve-operation-all-parameters branch from 2b36abd to 3a5b182 Compare August 11, 2023 07:23
Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

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

Just two more comments about the tests, otherwise looks great!

@arthurcro arthurcro force-pushed the improve-operation-all-parameters branch from 87c823d to a0d04b2 Compare August 11, 2023 11:51
Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@czechboy0 czechboy0 enabled auto-merge (squash) August 11, 2023 12:51
@czechboy0 czechboy0 merged commit 15423b4 into apple:main Aug 11, 2023
@czechboy0 czechboy0 added the 🔨 semver/patch No public API change. label Aug 16, 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.

Overriding path item parameters with operation parameters doesn't work
2 participants