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.

Sign up for GitHub

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
Open

Remove blank space on rewrited operationId #622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/open_api_spex/paths.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

end)
end)
end
Expand Down
2 changes: 1 addition & 1 deletion test/paths_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ defmodule OpenApiSpex.PathsTest do
operation_ids = [pets_path_item.put.operationId, pets_path_item.patch.operationId]

assert "OpenApiSpexTest.PetController.update" in operation_ids
assert "OpenApiSpexTest.PetController.update (2)" in operation_ids
assert "OpenApiSpexTest.PetController.update_2" in operation_ids
end
end
end
Loading