Bug 28369

Summary: additionalProperties missing in holds routes
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Joonas Kylmälä <joonas.kylmala>
Severity: major    
Priority: P5 - low CC: arthur.suzuki, ere.maijala, fridolin.somers, jonathan.druart, joonas.kylmala, martin.renvoize, nugged, stalkernoid
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00
Bug Depends on: 28272, 28370    
Bug Blocks:    
Attachments: Bug 28369: Regression tests
Bug 28369: Set additionalProperties: false and fix things
Bug 28369: (QA follow-up) Restore PUT with deprecation message
Bug 28369: Regression tests
Bug 28369: Set additionalProperties: false and fix things
Bug 28369: (QA follow-up) Restore PUT with deprecation message
Bug 28369: Regression tests
Bug 28369: Set additionalProperties: false and fix things
Bug 28369: (QA follow-up) Restore PUT with deprecation message
Bug 28369: Regression tests
Bug 28369: Set additionalProperties: false and fix things
Bug 28369: (QA follow-up) Restore PUT with deprecation message

Description Tomás Cohen Arazi 2021-05-18 11:50:38 UTC
There are some objects that are missing this. Enabling it highlights long-standing issues like: parameters that the code accepts, but are missing on the spec, and tests that pass some extra parameters and work only because they are not good tests.

Plus: there are design doubts about PUT /holds/:hold_id and we should make it a PATCH route.
Comment 1 Tomás Cohen Arazi 2021-05-18 12:15:48 UTC
Created attachment 121105 [details] [review]
Bug 28369: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2021-05-18 12:15:52 UTC
Created attachment 121106 [details] [review]
Bug 28369: Set additionalProperties: false and fix things

This patch performs a cleanup on the holds routes:
- It adds additionalProperties: false to all missing routes
- It syncs the spec with the guidelines
- additionalProperties: false highlighted some attributes read in the
  controlled weren't declared on the spec, adding them
- PUT /holds/:hold_id is moved to PATCH, as it makes more sense and
  gathered opinions lean towards that

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Lots of things are wrong with the API
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Joonas Kylmälä 2021-05-18 14:42:22 UTC
Could we please add first a deprecation notice and create the PATCH route in addition to the PUT instead of simply renaming the PUT? We have already code using this API (at least https://github.com/NatLibFi/NDL-VuFind2/blob/dev/module/Finna/src/Finna/ILS/Driver/KohaRest.php) and I'm thinking there might also other people using this API we are not aware of, so things might unexpectedly break on the next update if we simply remove the PUT method without any migration period.
Comment 4 Tomás Cohen Arazi 2021-05-18 14:59:13 UTC
(In reply to Joonas Kylmälä from comment #3)
> Could we please add first a deprecation notice and create the PATCH route in
> addition to the PUT instead of simply renaming the PUT? We have already code
> using this API (at least
> https://github.com/NatLibFi/NDL-VuFind2/blob/dev/module/Finna/src/Finna/ILS/
> Driver/KohaRest.php) and I'm thinking there might also other people using
> this API we are not aware of, so things might unexpectedly break on the next
> update if we simply remove the PUT method without any migration period.

Yes! Can certainly do!

But as I mentioned, the PUT route is using (in the code) different attribute names than the API spec, so you need to check if it isn't broken already (20.11). I couldn't find the references to the API routes on that PHP code.
Comment 5 Joonas Kylmälä 2021-05-18 15:04:51 UTC
(In reply to Tomás Cohen Arazi from comment #4)
> Yes! Can certainly do!
> 
> But as I mentioned, the PUT route is using (in the code) different attribute
> names than the API spec, so you need to check if it isn't broken already
> (20.11). I couldn't find the references to the API routes on that PHP code.

The code is in the function changePickupLocation. The code in changePickupLocation uses the attribute names that work (instead of broken ones).
Comment 6 Tomás Cohen Arazi 2021-05-18 15:21:34 UTC
(In reply to Joonas Kylmälä from comment #5)
> (In reply to Tomás Cohen Arazi from comment #4)
> > Yes! Can certainly do!
> > 
> > But as I mentioned, the PUT route is using (in the code) different attribute
> > names than the API spec, so you need to check if it isn't broken already
> > (20.11). I couldn't find the references to the API routes on that PHP code.
> 
> The code is in the function changePickupLocation. The code in
> changePickupLocation uses the attribute names that work (instead of broken
> ones).

Ok, I'm fine with fixing the spec and adding a deprecation warning, and adding PATCH as a duplicate for PUT.

Do you agree?
Comment 7 Tomás Cohen Arazi 2021-05-18 15:43:38 UTC
Created attachment 121119 [details] [review]
Bug 28369: (QA follow-up) Restore PUT with deprecation message

This patch restores the PUT route, but adding a deprecation message. The
controller method is the same, and relevant patch tests are duplicated
but calling PUT, so it still gets tested.

To test:
1. Apply this patch
2. Reload plack
3. Open https://<>/api/v1/.html and find the PUT /holds/{hold_id} route
=> SUCCESS: There's a deprecation wargning!
4. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 David Nind 2021-05-18 20:52:36 UTC
Created attachment 121131 [details] [review]
Bug 28369: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2021-05-18 20:52:40 UTC
Created attachment 121132 [details] [review]
Bug 28369: Set additionalProperties: false and fix things

This patch performs a cleanup on the holds routes:
- It adds additionalProperties: false to all missing routes
- It syncs the spec with the guidelines
- additionalProperties: false highlighted some attributes read in the
  controlled weren't declared on the spec, adding them
- PUT /holds/:hold_id is moved to PATCH, as it makes more sense and
  gathered opinions lean towards that

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Lots of things are wrong with the API
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2021-05-18 20:52:45 UTC
Created attachment 121133 [details] [review]
Bug 28369: (QA follow-up) Restore PUT with deprecation message

This patch restores the PUT route, but adding a deprecation message. The
controller method is the same, and relevant patch tests are duplicated
but calling PUT, so it still gets tested.

To test:
1. Apply this patch
2. Reload plack
3. Open https://<>/api/v1/.html and find the PUT /holds/{hold_id} route
=> SUCCESS: There's a deprecation wargning!
4. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 Ere Maijala 2021-05-19 05:57:22 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> (In reply to Joonas Kylmälä from comment #5)
> > (In reply to Tomás Cohen Arazi from comment #4)
> > > Yes! Can certainly do!
> > > 
> > > But as I mentioned, the PUT route is using (in the code) different attribute
> > > names than the API spec, so you need to check if it isn't broken already
> > > (20.11). I couldn't find the references to the API routes on that PHP code.
> > 
> > The code is in the function changePickupLocation. The code in
> > changePickupLocation uses the attribute names that work (instead of broken
> > ones).
> 
> Ok, I'm fine with fixing the spec and adding a deprecation warning, and
> adding PATCH as a duplicate for PUT.
> 
> Do you agree?

Works for me (I'm responsible for the KohaRest PHP module). As a side note, it would be really nice if PATCH was able to modify priority, suspension and suspension end date as well as pickup location. From an API user's standpoint modifying several attributes of a hold is typically a single operation, and having to make several API calls makes using it convoluted.
Comment 12 Tomás Cohen Arazi 2021-05-19 11:35:51 UTC
(In reply to Ere Maijala from comment #11)
> (In reply to Tomás Cohen Arazi from comment #6)
> > (In reply to Joonas Kylmälä from comment #5)
> > > (In reply to Tomás Cohen Arazi from comment #4)
> > > > Yes! Can certainly do!
> > > > 
> > > > But as I mentioned, the PUT route is using (in the code) different attribute
> > > > names than the API spec, so you need to check if it isn't broken already
> > > > (20.11). I couldn't find the references to the API routes on that PHP code.
> > > 
> > > The code is in the function changePickupLocation. The code in
> > > changePickupLocation uses the attribute names that work (instead of broken
> > > ones).
> > 
> > Ok, I'm fine with fixing the spec and adding a deprecation warning, and
> > adding PATCH as a duplicate for PUT.
> > 
> > Do you agree?
> 
> Works for me (I'm responsible for the KohaRest PHP module). As a side note,
> it would be really nice if PATCH was able to modify priority, suspension and
> suspension end date as well as pickup location. From an API user's
> standpoint modifying several attributes of a hold is typically a single
> operation, and having to make several API calls makes using it convoluted.

Luckily, it works like that!
Comment 13 Martin Renvoize 2021-05-20 08:39:43 UTC
This all looks good to me, but I'm not one of the existing consumers of this API endpoint, so I think it worth getting a final QA signoff from Joonas or Ere.. I'll add my SO lines to varify I've tested and it works as expected from my perspective however.
Comment 14 Martin Renvoize 2021-05-20 08:41:22 UTC
Created attachment 121233 [details] [review]
Bug 28369: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2021-05-20 08:41:27 UTC
Created attachment 121234 [details] [review]
Bug 28369: Set additionalProperties: false and fix things

This patch performs a cleanup on the holds routes:
- It adds additionalProperties: false to all missing routes
- It syncs the spec with the guidelines
- additionalProperties: false highlighted some attributes read in the
  controlled weren't declared on the spec, adding them
- PUT /holds/:hold_id is moved to PATCH, as it makes more sense and
  gathered opinions lean towards that

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Lots of things are wrong with the API
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2021-05-20 08:41:31 UTC
Created attachment 121235 [details] [review]
Bug 28369: (QA follow-up) Restore PUT with deprecation message

This patch restores the PUT route, but adding a deprecation message. The
controller method is the same, and relevant patch tests are duplicated
but calling PUT, so it still gets tested.

To test:
1. Apply this patch
2. Reload plack
3. Open https://<>/api/v1/.html and find the PUT /holds/{hold_id} route
=> SUCCESS: There's a deprecation wargning!
4. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Joonas Kylmälä 2021-05-20 12:21:02 UTC
Created attachment 121236 [details] [review]
Bug 28369: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 18 Joonas Kylmälä 2021-05-20 12:21:07 UTC
Created attachment 121237 [details] [review]
Bug 28369: Set additionalProperties: false and fix things

This patch performs a cleanup on the holds routes:
- It adds additionalProperties: false to all missing routes
- It syncs the spec with the guidelines
- additionalProperties: false highlighted some attributes read in the
  controlled weren't declared on the spec, adding them
- PUT /holds/:hold_id is moved to PATCH, as it makes more sense and
  gathered opinions lean towards that

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Lots of things are wrong with the API
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 19 Joonas Kylmälä 2021-05-20 12:21:11 UTC
Created attachment 121238 [details] [review]
Bug 28369: (QA follow-up) Restore PUT with deprecation message

This patch restores the PUT route, but adding a deprecation message. The
controller method is the same, and relevant patch tests are duplicated
but calling PUT, so it still gets tested.

To test:
1. Apply this patch
2. Reload plack
3. Open https://<>/api/v1/.html and find the PUT /holds/{hold_id} route
=> SUCCESS: There's a deprecation wargning!
4. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 20 Jonathan Druart 2021-05-25 07:46:13 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 21 Fridolin Somers 2021-05-25 10:20:50 UTC
Depends on Bug 28272 not in 20.11.x