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

Remove blank space on rewrited operationId #622

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

Conversation

VitorVRS
Copy link

The operationId is usually used in the URL (e.g: Swagger UI), so URLs should contain only safe chars.

The inspiration for this PR is the spectral linter validation: https://github.com/stoplightio/spectral/blob/develop/packages/rulesets/src/oas/index.ts#L296

@@ -84,7 +84,7 @@ defmodule OpenApiSpex.Paths do
rest
|> Enum.with_index(2)
|> Enum.map(fn {{path, verb, operation}, occurrence} ->
{path, verb, %{operation | operationId: "#{operation_id} (#{occurrence})"}}
{path, verb, %{operation | operationId: "#{operation_id}_#{occurrence}"}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO this should raise an error rather than re-writing the operation ID.
There's a chance that the "#{operation_id}_#{occurrence}" convention will conflict with an operation ID the library user has explicitly put on one of their operations.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are some additional issues with make_operation_ids_unique/1.
For update actions defined using the resource macro, like:

resources "/pets", OpenApiSpexTest.PetController, only: [:update]

both patch and put are handled by the same function which leads to duplicate operation ids. If there's nothing the user can do to prevent this, then raising an error might cause frustration.

Another issue with make_operation_ids_unique/1 is that it doesn't respect operation_id: nil.

operationId is an optional unique string used to identify an operation. If provided, these IDs must be unique
among all operations described in your API.

operationId is optional, but if one tries to skip it, the duplicate operation for the put verb will have a " (2)" operation_id, which will certainly be unexpected.

  1) test Paths from_router (OpenApiSpex.PathsTest)
     test/paths_test.exs:7
     Assertion with in failed
     code:  assert "OpenApiSpexTest.PetController.update" in operation_ids
     left:  "OpenApiSpexTest.PetController.update"
     right: [nil, " (2)"]
     stacktrace:
       test/paths_test.exs:20: (test)

My suggestion is to make it an open_api_spex configuration. Keep "#{operation_id} (#{occurrence})" as the default for backwards compatibility and add the following options:

  • raise - raises an error
  • verb_suffix - Suffixes the operation ID of each duplicate with the verb, "#{operation_id}_#{verb}". This makes generated operation_id more deterministic since it does not depend on the order that paths are loaded.

In any case, when the operation_id is nil, it should be preserved.

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