Summary: | Definition files are missing additionalProperties: false | ||
---|---|---|---|
Product: | Koha | Reporter: | Jonathan Druart <jonathan.druart> |
Component: | REST API | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | CLOSED FIXED | QA Contact: | Tomás Cohen Arazi <tomascohen> |
Severity: | normal | ||
Priority: | P5 - low | CC: | fridolin.somers, martin.renvoize, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: |
When the REST API spec was first written we assumed additionalProperties: false was the default behavior. It probably was by then.
We recently found the need to explicitly add it to all the relevant places (i.e. those that require strictness).
This bug adds it, and fixes the tests that fail or the error conditions that were hidden due to this being absent in the spec.
|
Version(s) released in: |
21.05.00
|
Bug Depends on: | |||
Bug Blocks: | 28318, 28369, 28370 | ||
Attachments: |
Bug 28272: Prevent api spec to accept extra parameters
Bug 28272: Prevent api spec to accept extra parameters Bug 28272: Prevent api spec to accept extra parameters Bug 28272: Fix note_seen, desk_id, cancellation_reason Bug 28272: Fix many things... Bug 28272: Add unseen_renewals for checkouts Bug 28272: issues.notseen needs to be marked as a boolean Bug 28272: (QA follow-up) Add 'manager' to cashups definition Bug 28272: (QA follow-up) Add 'manager' to cashups definition |
Description
Jonathan Druart
2021-04-30 13:26:49 UTC
Created attachment 120630 [details] [review] Bug 28272: Prevent api spec to accept extra parameters Some of our definition files are missing { additionalProperties: false } We must have it to make sure all properties are defined and prevent the spec to accept extra parameters (being more strict but also allowing to catch errors more easily) grep -rL additionalProperties api/v1/swagger/definitions/*.json Created attachment 120673 [details] [review] Bug 28272: Prevent api spec to accept extra parameters Some of our definition files are missing { additionalProperties: false } We must have it to make sure all properties are defined and prevent the spec to accept extra parameters (being more strict but also allowing to catch errors more easily) grep -rL additionalProperties api/v1/swagger/definitions/*.json Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 120690 [details] [review] Bug 28272: Prevent api spec to accept extra parameters Some of our definition files are missing { additionalProperties: false } We must have it to make sure all properties are defined and prevent the spec to accept extra parameters (being more strict but also allowing to catch errors more easily) grep -rL additionalProperties api/v1/swagger/definitions/*.json Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to master for 21.05, thanks to everybody involved! Created attachment 120801 [details] [review] Bug 28272: Fix note_seen, desk_id, cancellation_reason Still failing: * t/db_dependent/api/v1/checkouts.t because of unseen_renewals but it's defined (?) * t/db_dependent/api/v1/acquisitions_orders.t about "conflict" that we are adding in the response. We do that in other places as well, what would be best to deal with that? And maybe others. I need a bit of help here. Created attachment 120812 [details] [review] Bug 28272: Fix many things... This patch could've been splitted into several. But, overall, adding additionalAttributes: false made the API fail on requests that send extra info (i.e. cases in which a dev added an attribute to the underlaying class/table and forgot to deal with it on the API (either adding it on the spec, or removing it from the response using Koha::Class::to_api_mapping). - Koha::Account::Line was missing: credit_type, interface, status, register_id and credit_number. I decided to call cash_register_id, and to remove credit_number from the response. FIXME: We need consensus on a name for the credit_number attribute, and add it to the response on the API. It deserves a separate bug. Too opinionated for a last-minute fix. - Koha::Club::Hold::add was returning bad auto-calculated values on field that (also) wasn't specified on the spec. Needs a test. - import_batch_profile had a typo: id_profile vs. profile_id. - error.json: In this case I reverted the change. This is because some routes are adding more 'info' with the error message, and I consider this should be done in a more generic approach. Time is required for us to think about this. So don't break the API in the meantime. FIXME: Implement a generic way to add a payload to error messages on the API. Maybe something to work on while on bug 28020. To test: 1. Run: $ kshell k$ prove t/db_dependent/api/v1/ => FAIL: Lots of tests fail 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Sending to the RM queue. credit_sequence_number Created attachment 120819 [details] [review] Bug 28272: Add unseen_renewals for checkouts t/db_dependent/api/v1/checkouts.t ...................... 7/93 # Failed test '200 OK' # at t/db_dependent/api/v1/checkouts.t line 198. # got: '500' # expected: '200' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/checkouts.t line 198. # Structures begin differing at: # $got->{unseen_renewals} = Does not exist # $expected->{unseen_renewals} = '0' t/db_dependent/api/v1/checkouts.t ...................... 88/93 # Failed test '200 OK' # at t/db_dependent/api/v1/checkouts.t line 218. # got: '500' # expected: '200' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/checkouts.t line 218. # Structures begin differing at: # $got->{max_renewals} = Does not exist # $expected->{max_renewals} = '1' # Looks like you failed 4 tests of 93. Last patches pushed to master. Keeping the "additional_work_needed" kw for now as we need to open new bug reports. Created attachment 120833 [details] [review] Bug 28272: issues.notseen needs to be marked as a boolean Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Last patch pushed to master. Created attachment 121182 [details] [review] Bug 28272: (QA follow-up) Add 'manager' to cashups definition The embeddable 'manager' relation field was missing from the specification file and as such calls using this embed would result in a 500 error since we introduced 'additionalProperties: false'. Test plan 1/ Enable 'UseCashRegisters' 2/ Navigate to the register details page 3/ Inspect the network traffic and note a 500 error on the cashups api call 4/ Apply the patch 5/ The 500 should have gone away and been replaced with an empty resultset 200 page. Created attachment 121184 [details] [review] Bug 28272: (QA follow-up) Add 'manager' to cashups definition The embeddable 'manager' relation field was missing from the specification file and as such calls using this embed would result in a 500 error since we introduced 'additionalProperties: false'. Test plan 1/ Enable 'UseCashRegisters' 2/ Navigate to the register details page 3/ Inspect the network traffic and note a 500 error on the cashups api call 4/ Apply the patch 5/ The 500 should have gone away and been replaced with an empty resultset 200 page. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Follow-up pushed to master. Can this be backported to stable branches ? (In reply to Fridolin Somers from comment #16) > Can this be backported to stable branches ? No, this is a major change. |