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

fix: Block duplicate status code definitions #92

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

provokateurin
Copy link
Member

Duplicate status code definitions lead to invalid specs (https://github.com/julien-nc/cospend-nc/blob/76964a296fd1a22aabf147e3fec5dec0b48fd4eb/openapi.json#L7939) because they can not be merged automatically. Therefore we need to prevent people from defining a status code twice. IMO this also lines up with HTTP semantics were one status code always means a specific body and set of headers is returned. It is up to the developer to specify the body and header types correctly (e.g. nullability).

@provokateurin provokateurin added the bug Something isn't working label Feb 2, 2024
@nickvergessen
Copy link
Member

PHP Fatal error: Uncaught Error: OCM#discovery: Each status code can only be defined once, but 2 was defined multiple times.
thrown in /home/runner/work/openapi-extractor/openapi-extractor/src/Logger.php on line 53

@nickvergessen
Copy link
Member

Ah, that's "implicit", one 500 is documented as it's returned, one is automatic because it's from an exception

@provokateurin
Copy link
Member Author

I see, also the error message is wrong because it should have shown the status code and not the number of times it appears.

@provokateurin
Copy link
Member Author

For the failure seen in the CI the cause wasn't the 500s, but this PR can't actually work for scenarios where multiple different response classes are used like in https://github.com/nextcloud/server/blob/489608a2238a62409eb2305a2aee621d6bd774b6/apps/theming/lib/Controller/IconController.php#L117.
The proper fix for that would be to allow different content types to define the same status code, but that also doesn't work for the case mentioned above because of the different response classes.
I don't see another way than printing a warning about this case (every time) and panic in case an invalid spec would be generated.
@nickvergessen what are your thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants