Bug 38929 - POST endpoints not returning the Location header
Summary: POST endpoints not returning the Location header
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Matt Blenkinsop
URL:
Keywords:
Depends on: 38926
Blocks:
  Show dependency treegraph
 
Reported: 2025-01-20 13:33 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-01-24 15:15 UTC (History)
6 users (show)

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


Attachments
Bug 38929: Regression tests (19.15 KB, patch)
2025-01-20 17:03 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38929: Make POST endpoints return the `Location` header (4.43 KB, patch)
2025-01-20 17:03 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38929: (follow-up) Replace SWAGGER -> REST in the tests (57.90 KB, patch)
2025-01-20 17:03 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38929: Add xt test to enforce REST3.4.1 POST (1.56 KB, patch)
2025-01-22 10:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38929: Add 2 exceptions (1.26 KB, patch)
2025-01-22 12:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38929: Regression tests (19.18 KB, patch)
2025-01-23 19:38 UTC, David Nind
Details | Diff | Splinter Review
Bug 38929: Make POST endpoints return the `Location` header (4.47 KB, patch)
2025-01-23 19:38 UTC, David Nind
Details | Diff | Splinter Review
Bug 38929: (follow-up) Replace SWAGGER -> REST in the tests (57.88 KB, patch)
2025-01-23 19:39 UTC, David Nind
Details | Diff | Splinter Review
Bug 38929: Add xt test to enforce REST3.4.1 POST (1.61 KB, patch)
2025-01-23 19:39 UTC, David Nind
Details | Diff | Splinter Review
Bug 38929: Add 2 exceptions (1.31 KB, patch)
2025-01-23 19:39 UTC, David Nind
Details | Diff | Splinter Review
Bug 38929: Regression tests (19.26 KB, patch)
2025-01-24 12:14 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 38929: Make POST endpoints return the `Location` header (4.54 KB, patch)
2025-01-24 12:14 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 38929: (follow-up) Replace SWAGGER -> REST in the tests (58.01 KB, patch)
2025-01-24 12:14 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 38929: Add xt test to enforce REST3.4.1 POST (1.67 KB, patch)
2025-01-24 12:14 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 38929: Add 2 exceptions (1.37 KB, patch)
2025-01-24 12:14 UTC, Matt Blenkinsop
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 (tcohen) 2025-01-20 13:33:35 UTC
I found `POST /biblios` is not returning it. Opening this bug to track and fix such problems.
Comment 1 Tomás Cohen Arazi (tcohen) 2025-01-20 17:03:35 UTC
Created attachment 176832 [details] [review]
Bug 38929: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi (tcohen) 2025-01-20 17:03:37 UTC
Created attachment 176833 [details] [review]
Bug 38929: Make POST endpoints return the `Location` header

This patch makes the POST endpoints missing the Location header return
it.

Some endpoints I skipped:

* /patrons/account/credits (doesn't have a GET route to point to)
* /patrons/account/debits (doesn't have a GET route to point to)

To test:
1. Apply the regression tests
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/authorities.t \
           t/db_dependent/api/v1/biblios.t \
           t/db_dependent/api/v1/clubs_holds.t \
           t/db_dependent/api/v1/holds.t \
           t/db_dependent/api/v1/ill_batchstatuses.t \
           t/db_dependent/api/v1/import_batch_profiles.t \
           t/db_dependent/api/v1/transfer_limits.t
=> FAIL: Tests fail! The endpoints don't return the header!
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 Tomás Cohen Arazi (tcohen) 2025-01-20 17:03:40 UTC
Created attachment 176834 [details] [review]
Bug 38929: (follow-up) Replace SWAGGER -> REST in the tests

The wiki now uses `REST*` for the rule names instead of `SWAGGER*` some
tests have their descriptions correct, but most don't.

This is a follow-up to an editorial decision made on the Wiki (probably
at a dev meeting too, I don't remember).

QA tests will probably not pass, but I refrain from tidying it because
there's a major tidy on its way to main.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 David Cook 2025-01-20 23:31:47 UTC
I hadn't thought about this use case but it makes sense to return the Location header
Comment 5 Jonathan Druart 2025-01-21 07:53:43 UTC
Do we want a test to enforce this?

Should we ALWAYS find $c->res->headers->location before the render call?
Comment 6 Tomás Cohen Arazi (tcohen) 2025-01-21 11:37:10 UTC
(In reply to Jonathan Druart from comment #5)
> Do we want a test to enforce this?
> 
> Should we ALWAYS find $c->res->headers->location before the render call?

Well, we have a long time voted coding guideline

https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#REST3.4.1_POST
Comment 7 Jonathan Druart 2025-01-22 10:55:09 UTC
Created attachment 176901 [details] [review]
Bug 38929: Add xt test to enforce REST3.4.1 POST
Comment 8 Jonathan Druart 2025-01-22 10:55:19 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #6)
> (In reply to Jonathan Druart from comment #5)
> > Do we want a test to enforce this?
> > 
> > Should we ALWAYS find $c->res->headers->location before the render call?
> 
> Well, we have a long time voted coding guideline
> 
> https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#REST3.4.1_POST

Yes, and it's not enough. Here is a test to enforce it, we have 3 failures.

    #   Failed test 'Koha/REST/V1/Auth/Password.pm does not contain the location header'
    #   at xt/api.t line 186.

    #   Failed test 'Koha/REST/V1/Patrons/Account.pm does not contain the location header'
    #   at xt/api.t line 186.

    #   Failed test 'Koha/REST/V1/Preservation/WaitingList.pm does not contain the location header'
    #   at xt/api.t line 186.
    # Looks like you failed 3 tests of 55.
Comment 9 Tomás Cohen Arazi (tcohen) 2025-01-22 11:46:25 UTC
(In reply to Jonathan Druart from comment #8)
> (In reply to Tomás Cohen Arazi (tcohen) from comment #6)
> > 
> > Well, we have a long time voted coding guideline
> > 
> > https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#REST3.4.1_POST
> 
> Yes, and it's not enough. Here is a test to enforce it, we have 3 failures.

I know about them. Answering inline:

>     #   Failed test 'Koha/REST/V1/Auth/Password.pm does not contain the
> location header'
>     #   at xt/api.t line 186.

I'm not sure what location it should return. Maybe the same was used for the request.

>     #   Failed test 'Koha/REST/V1/Patrons/Account.pm does not contain the
> location header'
>     #   at xt/api.t line 186.

This is certainly handled by bug 38932.

>     #   Failed test 'Koha/REST/V1/Preservation/WaitingList.pm does not
> contain the location header'
>     #   at xt/api.t line 186.
>     # Looks like you failed 3 tests of 55.

A complex one. The POST receives a list of items. I'm also not sure on this one. Ideas?
Comment 10 Jonathan Druart 2025-01-22 12:05:43 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #9)
> (In reply to Jonathan Druart from comment #8)
> > (In reply to Tomás Cohen Arazi (tcohen) from comment #6)
> >     #   Failed test 'Koha/REST/V1/Auth/Password.pm does not contain the
> > location header'
> >     #   at xt/api.t line 186.
> 
> I'm not sure what location it should return. Maybe the same was used for the
> request.

This one could be skip I guess.

> >     #   Failed test 'Koha/REST/V1/Preservation/WaitingList.pm does not
> > contain the location header'
> >     #   at xt/api.t line 186.
> >     # Looks like you failed 3 tests of 55.
> 
> A complex one. The POST receives a list of items. I'm also not sure on this
> one. Ideas?

Point to the first one? Of we skip this specific file from the test.
Exceptions are not bad, if they prevent us from new missing occurrences.
Comment 11 Jonathan Druart 2025-01-22 12:08:30 UTC
Created attachment 176906 [details] [review]
Bug 38929: Add 2 exceptions
Comment 12 David Nind 2025-01-22 19:08:52 UTC
Testing notes (using KTD):
1. xt tests now fail: prove xt/api.t
2. Other tests pass after pacthes applied:
   - Tests in step 2 of the test plan
   - All the API tests: prove t/db_dependent/api/v1/*

prove xt/api.t
xt/api.t .. 3/6 
    #   Failed test 'Koha/REST/V1/Patrons/Account.pm does not contain the location header'
    #   at xt/api.t line 193.
    # Looks like you failed 1 test of 55.

#   Failed test 'POST (201) have location header'
#   at xt/api.t line 196.
# Looks like you failed 1 test of 6.
xt/api.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/6 subtests 

Test Summary Report
-------------------
xt/api.t (Wstat: 256 (exited 1) Tests: 6 Failed: 1)
  Failed test:  6
  Non-zero exit status: 1
Files=1, Tests=6,  4 wallclock secs ( 0.01 usr  0.01 sys +  4.39 cusr  0.32 csys =  4.73 CPU)
Result: FAIL
Comment 13 Jonathan Druart 2025-01-23 10:03:34 UTC
David, this failure should be fixed with the patches from bug 38932 applied.
Comment 14 Jonathan Druart 2025-01-23 10:04:06 UTC
@RM - Push along with bug 38932.
Comment 15 David Nind 2025-01-23 19:38:54 UTC
Created attachment 177068 [details] [review]
Bug 38929: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Comment 16 David Nind 2025-01-23 19:38:57 UTC
Created attachment 177069 [details] [review]
Bug 38929: Make POST endpoints return the `Location` header

This patch makes the POST endpoints missing the Location header return
it.

Some endpoints I skipped:

* /patrons/account/credits (doesn't have a GET route to point to)
* /patrons/account/debits (doesn't have a GET route to point to)

To test:
1. Apply the regression tests
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/authorities.t \
           t/db_dependent/api/v1/biblios.t \
           t/db_dependent/api/v1/clubs_holds.t \
           t/db_dependent/api/v1/holds.t \
           t/db_dependent/api/v1/ill_batchstatuses.t \
           t/db_dependent/api/v1/import_batch_profiles.t \
           t/db_dependent/api/v1/transfer_limits.t
=> FAIL: Tests fail! The endpoints don't return the header!
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 17 David Nind 2025-01-23 19:39:00 UTC
Created attachment 177070 [details] [review]
Bug 38929: (follow-up) Replace SWAGGER -> REST in the tests

The wiki now uses `REST*` for the rule names instead of `SWAGGER*` some
tests have their descriptions correct, but most don't.

This is a follow-up to an editorial decision made on the Wiki (probably
at a dev meeting too, I don't remember).

QA tests will probably not pass, but I refrain from tidying it because
there's a major tidy on its way to main.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Comment 18 David Nind 2025-01-23 19:39:03 UTC
Created attachment 177071 [details] [review]
Bug 38929: Add xt test to enforce REST3.4.1 POST

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 David Nind 2025-01-23 19:39:06 UTC
Created attachment 177072 [details] [review]
Bug 38929: Add 2 exceptions

Signed-off-by: David Nind <david@davidnind.com>
Comment 20 David Nind 2025-01-23 19:43:39 UTC
(In reply to Jonathan Druart from comment #13)
> David, this failure should be fixed with the patches from bug 38932 applied.

Thanks Jonathan!

Testing notes (using KTD):

1. Apply 38926.
2. Apply 38932.
3. Run: yarn api:bundle
4. Apply 38929.
5. Restart everything: restart_all
6. Check that these tests pass (step 2 of the test plan):
    prove t/db_dependent/api/v1/authorities.t \
      t/db_dependent/api/v1/biblios.t \
      t/db_dependent/api/v1/clubs_holds.t \
      t/db_dependent/api/v1/holds.t \
      t/db_dependent/api/v1/ill_batchstatuses.t \
      t/db_dependent/api/v1/import_batch_profiles.t \
      t/db_dependent/api/v1/transfer_limits.t
7. Check that these tests pass: prove xt/api.t
8. Check that all the API tests pass: prove t/db_dependent/api/v1/*
Comment 21 Matt Blenkinsop 2025-01-24 12:14:10 UTC
Created attachment 177106 [details] [review]
Bug 38929: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 22 Matt Blenkinsop 2025-01-24 12:14:13 UTC
Created attachment 177107 [details] [review]
Bug 38929: Make POST endpoints return the `Location` header

This patch makes the POST endpoints missing the Location header return
it.

Some endpoints I skipped:

* /patrons/account/credits (doesn't have a GET route to point to)
* /patrons/account/debits (doesn't have a GET route to point to)

To test:
1. Apply the regression tests
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/authorities.t \
           t/db_dependent/api/v1/biblios.t \
           t/db_dependent/api/v1/clubs_holds.t \
           t/db_dependent/api/v1/holds.t \
           t/db_dependent/api/v1/ill_batchstatuses.t \
           t/db_dependent/api/v1/import_batch_profiles.t \
           t/db_dependent/api/v1/transfer_limits.t
=> FAIL: Tests fail! The endpoints don't return the header!
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: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 23 Matt Blenkinsop 2025-01-24 12:14:16 UTC
Created attachment 177108 [details] [review]
Bug 38929: (follow-up) Replace SWAGGER -> REST in the tests

The wiki now uses `REST*` for the rule names instead of `SWAGGER*` some
tests have their descriptions correct, but most don't.

This is a follow-up to an editorial decision made on the Wiki (probably
at a dev meeting too, I don't remember).

QA tests will probably not pass, but I refrain from tidying it because
there's a major tidy on its way to main.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 24 Matt Blenkinsop 2025-01-24 12:14:20 UTC
Created attachment 177109 [details] [review]
Bug 38929: Add xt test to enforce REST3.4.1 POST

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 25 Matt Blenkinsop 2025-01-24 12:14:23 UTC
Created attachment 177110 [details] [review]
Bug 38929: Add 2 exceptions

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 26 Katrin Fischer 2025-01-24 14:40:05 UTC
Please note: xt/api.t will fail without Bug 38932.
Comment 27 Katrin Fischer 2025-01-24 15:15:24 UTC
Pushed for 25.05!

Well done everyone, thank you!