Bug 18282 - OpenAPI operationId must be unique
Summary: OpenAPI operationId must be unique
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST api (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Lari Taskula
QA Contact: Tomás Cohen Arazi
URL:
Whiteboard:
Keywords:
Depends on: 18137
Blocks:
  Show dependency treegraph
 
Reported: 2017-03-16 12:34 UTC by Lari Taskula
Modified: 2017-10-27 11:53 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18282: operationId must be unique (1.90 KB, patch)
2017-03-16 12:37 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18282: operationId must be unique (1.97 KB, patch)
2017-08-22 19:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2017-03-16 12:34:52 UTC
operationId has the following documentation:
 "Unique string used to identify the operation. The id MUST be unique among all
  operations described in the API."
Comment 1 Lari Taskula 2017-03-16 12:37:54 UTC
Created attachment 61169 [details] [review]
Bug 18282: operationId must be unique

operationId has the following documentation:
 "Unique string used to identify the operation. The id MUST be unique among all
  operations described in the API."

This patch modifies operationIds to be unique accross our API operations.
Comment 2 Tomás Cohen Arazi 2017-03-16 12:52:09 UTC
Lari, shouldn't this patch also change the controller class function names?

- sub list {
+ sub list_cities {
- sub add {
+ sub add_city {
- sub update {
+ sub update_city {
- sub delete {
+ sub delete_city {
Comment 3 Lari Taskula 2017-03-16 12:58:35 UTC
(In reply to Tomás Cohen Arazi from comment #2)
> Lari, shouldn't this patch also change the controller class function names?
> 
> - sub list {
> + sub list_cities {
> - sub add {
> + sub add_city {
> - sub update {
> + sub update_city {
> - sub delete {
> + sub delete_city {

With Swagger2 plugin yes, but I set this as dependent on Mojolicious::Plugin::OpenAPI, which is using "x-mojo-to" for pointing to the controller class operation.
Comment 4 Lari Taskula 2017-03-16 13:02:01 UTC
(In reply to Lari Taskula from comment #3)
> (In reply to Tomás Cohen Arazi from comment #2)
> > Lari, shouldn't this patch also change the controller class function names?
> > 
> > - sub list {
> > + sub list_cities {
> > - sub add {
> > + sub add_city {
> > - sub update {
> > + sub update_city {
> > - sub delete {
> > + sub delete_city {
> 
> With Swagger2 plugin yes, but I set this as dependent on
> Mojolicious::Plugin::OpenAPI, which is using "x-mojo-to" for pointing to the
> controller class operation.

In fact not either with Swagger2. As you can see from patrons controller.
Comment 5 Tomás Cohen Arazi 2017-03-16 13:04:24 UTC
(In reply to Lari Taskula from comment #4)
> (In reply to Lari Taskula from comment #3)
> > (In reply to Tomás Cohen Arazi from comment #2)
> > > Lari, shouldn't this patch also change the controller class function names?
> > > 
> > > - sub list {
> > > + sub list_cities {
> > > - sub add {
> > > + sub add_city {
> > > - sub update {
> > > + sub update_city {
> > > - sub delete {
> > > + sub delete_city {
> > 
> > With Swagger2 plugin yes, but I set this as dependent on
> > Mojolicious::Plugin::OpenAPI, which is using "x-mojo-to" for pointing to the
> > controller class operation.
> 
> In fact not either with Swagger2. As you can see from patrons controller.

I tend to prefer avoiding black magic as much as possible to avoid unexpected behaviours. That's why I prefer to explicitly put the most similar to the function name. But keep moving and see how bad/good it looks.
Comment 6 Tomás Cohen Arazi 2017-08-22 19:08:42 UTC
Created attachment 66369 [details] [review]
Bug 18282: operationId must be unique

operationId has the following documentation:
 "Unique string used to identify the operation. The id MUST be unique among all
  operations described in the API."

This patch modifies operationIds to be unique accross our API operations.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2017-08-22 19:09:38 UTC
SKipping QA step on a trivial string change.
Comment 8 Jonathan Druart 2017-09-21 15:49:48 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 9 Fridolin SOMERS 2017-10-27 11:53:10 UTC
Enhancement not pushed to 17.05.x