Bug 28272 - Definition files are missing additionalProperties: false
Summary: Definition files are missing additionalProperties: false
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks: 28318 28369 28370
  Show dependency treegraph
 
Reported: 2021-04-30 13:26 UTC by Jonathan Druart
Modified: 2021-05-20 13:34 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
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


Attachments
Bug 28272: Prevent api spec to accept extra parameters (9.44 KB, patch)
2021-05-06 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28272: Prevent api spec to accept extra parameters (9.46 KB, patch)
2021-05-07 11:01 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 28272: Prevent api spec to accept extra parameters (9.55 KB, patch)
2021-05-07 13:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28272: Fix note_seen, desk_id, cancellation_reason (2.53 KB, patch)
2021-05-10 15:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28272: Fix many things... (6.22 KB, patch)
2021-05-10 20:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28272: Add unseen_renewals for checkouts (1.77 KB, patch)
2021-05-11 07:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28272: issues.notseen needs to be marked as a boolean (768 bytes, patch)
2021-05-11 11:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28272: (QA follow-up) Add 'manager' to cashups definition (1.46 KB, patch)
2021-05-19 15:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28272: (QA follow-up) Add 'manager' to cashups definition (1.52 KB, patch)
2021-05-19 15:58 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 Jonathan Druart 2021-04-30 13:26:49 UTC
To force all the attributes to be documented we should add additionalProperties: false to all our definition files (api/v1/swagger/definitions/*.json).
Comment 1 Jonathan Druart 2021-05-06 13:14:15 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
Comment 2 Owen Leonard 2021-05-07 11:01:07 UTC
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>
Comment 3 Tomás Cohen Arazi 2021-05-07 13:26:56 UTC
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>
Comment 4 Jonathan Druart 2021-05-10 14:19:20 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 5 Jonathan Druart 2021-05-10 15:55:49 UTC
Created attachment 120801 [details] [review]
Bug 28272: Fix note_seen, desk_id, cancellation_reason
Comment 6 Jonathan Druart 2021-05-10 15:58:02 UTC
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.
Comment 7 Tomás Cohen Arazi 2021-05-10 20:54:15 UTC
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>
Comment 8 Tomás Cohen Arazi 2021-05-10 20:56:04 UTC
Sending to the RM queue.

credit_sequence_number
Comment 9 Jonathan Druart 2021-05-11 07:27:40 UTC
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.
Comment 10 Jonathan Druart 2021-05-11 08:47:40 UTC
Last patches pushed to master.

Keeping the "additional_work_needed" kw for now as we need to open new bug reports.
Comment 11 Tomás Cohen Arazi 2021-05-11 11:31:19 UTC
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>
Comment 12 Jonathan Druart 2021-05-11 13:40:08 UTC
Last patch pushed to master.
Comment 13 Martin Renvoize 2021-05-19 15:46:41 UTC
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.
Comment 14 Tomás Cohen Arazi 2021-05-19 15:58:02 UTC
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>
Comment 15 Jonathan Druart 2021-05-20 06:52:20 UTC
Follow-up pushed to master.
Comment 16 Fridolin Somers 2021-05-20 12:59:59 UTC
Can this be backported to stable branches ?
Comment 17 Jonathan Druart 2021-05-20 13:34:03 UTC
(In reply to Fridolin Somers from comment #16)
> Can this be backported to stable branches ?

No, this is a major change.