[Bug 28369] New: additionalProperties missing in holds routes
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 Bug ID: 28369 Summary: additionalProperties missing in holds routes Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: REST API Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Depends on| |28272 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28272 [Bug 28272] Definition files are missing additionalProperties: false -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff CC| |arthur.suzuki@biblibre.com, | |jonathan.druart+koha@gmail. | |com, | |joonas.kylmala@helsinki.fi, | |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 121105 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121105&action=edit Bug 28369: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 121106 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121106&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |major Keywords| |rel_21_05_candidate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |ere.maijala@helsinki.fi, | |nugged@gmail.com, | |stalkernoid@gmail.com --- Comment #3 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- 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/...) 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 --- Comment #5 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 --- Comment #6 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 --- Comment #7 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 121119 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121119&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121105|0 |1 is obsolete| | --- Comment #8 from David Nind <david@davidnind.com> --- Created attachment 121131 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121131&action=edit Bug 28369: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121106|0 |1 is obsolete| | --- Comment #9 from David Nind <david@davidnind.com> --- Created attachment 121132 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121132&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121119|0 |1 is obsolete| | --- Comment #10 from David Nind <david@davidnind.com> --- Created attachment 121133 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121133&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 --- Comment #11 from Ere Maijala <ere.maijala@helsinki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121131|0 |1 is obsolete| | --- Comment #14 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 121233 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121233&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121132|0 |1 is obsolete| | --- Comment #15 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 121234 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121234&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121133|0 |1 is obsolete| | --- Comment #16 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 121235 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121235&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact| |joonas.kylmala@helsinki.fi -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121233|0 |1 is obsolete| | Attachment #121234|0 |1 is obsolete| | Attachment #121235|0 |1 is obsolete| | --- Comment #17 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 121236 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121236&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 --- Comment #18 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 121237 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121237&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 --- Comment #19 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 121238 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121238&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28370 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28370 [Bug 28370] Routes still missing additionalProperties in spec -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|28370 | Depends on| |28370 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28370 [Bug 28370] Routes still missing additionalProperties in spec -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |21.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 --- Comment #20 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #21 from Fridolin Somers <fridolin.somers@biblibre.com> --- Depends on Bug 28272 not in 20.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28369 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_21_05_candidate | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org