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

test(testing): add test to detect duplicate endpoint names #3908

Merged
merged 9 commits into from
Jul 19, 2024

Conversation

ThijsBroersen
Copy link
Contributor

@ThijsBroersen ThijsBroersen commented Jul 6, 2024

From my interpretation on the OpenAPI spec, operationId's (endpoint names), should be unique. This test shows it is currently not detected in the EndpointVerifier and when generating the OpenAPI spec it does not warn or fail on duplicate operationId's, it just generates it.

Shall I continue implement the test in the EndpointVerifier?

Should the OpenAPIDocsInterpreter throw an exception on duplicate names? Or perhaps add a toVerifiedOpenAPI: Either[OpenAPIError, OpenAPI] method to the interpreter and not allow e.g. duplicate operationId's?

Refs: https://softwaremill.community/t/duplicate-operationids/416/7

@adamw
Copy link
Member

adamw commented Jul 8, 2024

It's a good question on the error modes, there's one place current during the openapi interpreter where we throw exceptions. Not perfect, but introducing Eithers at this point might be confusing. So maybe we should indeed just perform the check in the interpreter itself.

@ThijsBroersen ThijsBroersen marked this pull request as ready for review July 17, 2024 23:29
@ThijsBroersen
Copy link
Contributor Author

I implemented a duplicate name check in the EndpointVerifier.
I implemented an optional duplicate operationId check in the OpenAPI converter. I added a failOnDuplicateOperationId option to the OpenAPIDocsOptions (with a default to false).

@ThijsBroersen
Copy link
Contributor Author

The failing test is a flaky one? Does not look related to my code changes.

@kciesielski
Copy link
Member

@ThijsBroersen yes, we are dealing with some flaky tests right now, sorry for inconvenience.

@@ -213,6 +213,8 @@ Options can be customised by providing an instance of `OpenAPIDocsOptions` to th
* `schemaName`: specifies how schema names are created from the full type name. By default, this takes the last
component of a dot-separated type name. Suffixes might be added at a later stage to disambiguate between different
schemas with same names.
* `failOnDuplicateOperationId`: if set to `true`, the interpreter will throw an exception if it encounters two endpoints
with the same operation id. An OpenAPI document with duplicate operation ids is not valid. Code generators can silently drop duplicates.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe mention & link the endpoint verifier?

Copy link
Member

Choose a reason for hiding this comment

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

Though I'm not 100% sure that such verification should be part of the interpreter. There are other errors that are checked by the EndpointVerifier, which would also cause invalid specs to be generated (like shadowing, duplicate methods, invalid body). So shouldn't we rather rely on the verifier to contain all the verifications?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The feature (interpreter option) is opt-in, so not enabled by default. This is backwards compatible.
The EndpointVerifier is not per-se focused on OpenAPI compatibility. I think it is fair to say the OpenAPI converter should have a role in this and preferably provide a safe conversion to an Either/Option/Try.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's leave it then

Copy link
Member

Choose a reason for hiding this comment

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

I'll add some more docs

@adamw adamw merged commit 4be8ce7 into softwaremill:master Jul 19, 2024
22 checks passed
@adamw
Copy link
Member

adamw commented Jul 19, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants