Bug 28370 - Routes still missing additionalProperties in spec
Summary: Routes still missing additionalProperties in spec
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on: 28272 28424
Blocks: 28369
  Show dependency treegraph
 
Reported: 2021-05-18 12:33 UTC by Tomás Cohen Arazi
Modified: 2021-12-13 21:13 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00


Attachments
Bug 28370: Add missing additionalProperties in spec (5.61 KB, patch)
2021-05-18 13:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28370: Add missing additionalProperties in spec (5.64 KB, patch)
2021-05-18 21:08 UTC, David Nind
Details | Diff | Splinter Review
Bug 28370: Add missing additionalProperties in spec (5.71 KB, patch)
2021-05-20 08:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28370: Add tests (3.20 KB, patch)
2021-05-21 08:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28370: Add missing additionalProperties in spec (5.77 KB, patch)
2021-05-21 20:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28370: Add tests (3.25 KB, patch)
2021-05-21 20:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28370: Rewrite tests (5.30 KB, patch)
2021-05-21 20:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28370: (follow-up) More cases (2.73 KB, patch)
2021-05-21 20:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28370: Fix invalid responses in /patrons (1.05 KB, patch)
2021-05-21 20:21 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 Tomás Cohen Arazi 2021-05-18 12:33:28 UTC
There are still some routes that have inline object definitions, missing additionalProperties.
Comment 1 Tomás Cohen Arazi 2021-05-18 13:09:59 UTC
Created attachment 121110 [details] [review]
Bug 28370: Add missing additionalProperties in spec

This patch adds missing attribute to the object definitions in the spec.
We wrongly supposed it was the default, but it proved not.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1
=> SUCCESS: Tests pass, nothing breaks!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 David Nind 2021-05-18 21:08:37 UTC
Created attachment 121134 [details] [review]
Bug 28370: Add missing additionalProperties in spec

This patch adds missing attribute to the object definitions in the spec.
We wrongly supposed it was the default, but it proved not.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1
=> SUCCESS: Tests pass, nothing breaks!
3. Sign off :-D

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Martin Renvoize 2021-05-20 08:26:28 UTC
Created attachment 121232 [details] [review]
Bug 28370: Add missing additionalProperties in spec

This patch adds missing attribute to the object definitions in the spec.
We wrongly supposed it was the default, but it proved not.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1
=> SUCCESS: Tests pass, nothing breaks!
3. 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 4 Martin Renvoize 2021-05-20 08:27:49 UTC
Good Call, 

Should we also add an explicit coding guideline for this perhaps.. and a QA script catch to spot removals of the line in api/swagger/*.json files or lack of it in new files in that directory?

Passing QA
Comment 5 Jonathan Druart 2021-05-21 08:49:16 UTC
Created attachment 121270 [details] [review]
Bug 28370: Add tests
Comment 6 Jonathan Druart 2021-05-21 08:51:08 UTC
Those tests are failing, can you have a look please?

1. They caught 3 missing additionalProperties in paths/*

2. It seems that we should ignore the error.json and open a separate bug report
Comment 7 Tomás Cohen Arazi 2021-05-21 20:21:39 UTC
Created attachment 121284 [details] [review]
Bug 28370: Add missing additionalProperties in spec

This patch adds missing attribute to the object definitions in the spec.
We wrongly supposed it was the default, but it proved not.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1
=> SUCCESS: Tests pass, nothing breaks!
3. 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: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2021-05-21 20:21:43 UTC
Created attachment 121285 [details] [review]
Bug 28370: Add tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2021-05-21 20:21:47 UTC
Created attachment 121286 [details] [review]
Bug 28370: Rewrite tests

This patch rewrites the tests, and also doesn't enforce
additionalProperties to be false. We could need it, and there are routes
that need to be reviewed that would break otherwise now: error.json, for
example, is used everywhere, and some routes add some payload to it.

The main change this patch introduces is using the resolved spec instead
of finding each .json file, which wasn't even correct as we have .yaml
files already. Parameters and responses are tested.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove xt/api.t
=> SUCCESS: Tests fail! A nice report on the failures is printed

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2021-05-21 20:21:51 UTC
Created attachment 121287 [details] [review]
Bug 28370: (follow-up) More cases

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2021-05-21 20:21:56 UTC
Created attachment 121289 [details] [review]
Bug 28370: Fix invalid responses in /patrons

This response codes are not actually used.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2021-05-21 21:35:35 UTC
Bug 28424 fixes the remaining issue.
Comment 13 Martin Renvoize 2021-05-24 07:22:52 UTC
Great teamwork, the test looks a solid improvement.
Comment 14 Jonathan Druart 2021-05-24 08:11:00 UTC
t/db_dependent/api/v1/holds.t tests are failing.

Didn't we decide to fix error.json separately, because of the "conflict" param that was not in the spec? bug 28020
Comment 15 Tomás Cohen Arazi 2021-05-24 12:15:32 UTC
(In reply to Jonathan Druart from comment #14)
> t/db_dependent/api/v1/holds.t tests are failing.
> 
> Didn't we decide to fix error.json separately, because of the "conflict"
> param that was not in the spec? bug 28020

You are missing bug 28369 on your tree.
Comment 16 Tomás Cohen Arazi 2021-05-24 12:21:47 UTC
(In reply to Jonathan Druart from comment #14)
> Didn't we decide to fix error.json separately, because of the "conflict"
> param that was not in the spec? bug 28020

Do you think we should rush about it? It felt like some coordinated teamwork is needed to think about it and propose something sound. Specially when it comes to a generic payload schema. I allowed the extra attributes in error.json as a transitional step as we are a few days away from the release.

Can happily work on a better solution on that bug.
Comment 17 Jonathan Druart 2021-05-24 12:41:52 UTC
(In reply to Tomás Cohen Arazi from comment #16)
> (In reply to Jonathan Druart from comment #14)
> > Didn't we decide to fix error.json separately, because of the "conflict"
> > param that was not in the spec? bug 28020
> 
> Do you think we should rush about it?

No, definitely not. I saw you changed it and so that the tests were failing because of that. Wrong assumption!
Comment 18 Jonathan Druart 2021-05-25 07:46:20 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 19 Fridolin Somers 2021-05-25 10:20:16 UTC
Depends on Bug 28272 not in 20.11.x