Bug 18282

Summary: OpenAPI operationId must be unique
Product: Koha Reporter: Lari Taskula <lari.taskula>
Component: REST APIAssignee: Lari Taskula <lari.taskula>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 18137    
Bug Blocks:    
Attachments: Bug 18282: operationId must be unique
Bug 18282: operationId must be unique

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