Bug 38926 - POST /biblios returns 200 even if AddBiblio fails
Summary: POST /biblios returns 200 even if AddBiblio fails
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Testopia
URL:
Keywords:
Depends on: 31800
Blocks: 38929
  Show dependency treegraph
 
Reported: 2025-01-20 11:46 UTC by Jonathan Druart
Modified: 2025-01-24 11:03 UTC (History)
2 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 38926: Regression tests (1.46 KB, patch)
2025-01-20 13:10 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38926: Make POST /biblios return 400 if AddBiblio fails (1.97 KB, patch)
2025-01-20 13:10 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38926: Make POST /biblios return 400 if AddBiblio fails (2.62 KB, patch)
2025-01-20 17:14 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38926: Regression tests (1.53 KB, patch)
2025-01-21 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38926: Make POST /biblios return 400 if AddBiblio fails (2.69 KB, patch)
2025-01-21 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38926: Regression tests (1.58 KB, patch)
2025-01-22 10:04 UTC, David Nind
Details | Diff | Splinter Review
Bug 38926: Make POST /biblios return 400 if AddBiblio fails (2.74 KB, patch)
2025-01-22 10:04 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2025-01-20 11:46:05 UTC
For some reasons I POSTed an invalid biblio to /api/v1/biblios
it returned {id: null}

The biblio record is not created, but the response is 200.

707         my ( $biblio_id ) = AddBiblio( $record, $frameworkcode, { record_source_id => $record_source_id } );
708 
709         $c->render(
710             status  => 200,
711             openapi => { id => $biblio_id }
712         );

Obviously we should test $biblio_id
Comment 1 Tomás Cohen Arazi (tcohen) 2025-01-20 13:10:44 UTC
Created attachment 176795 [details] [review]
Bug 38926: Regression tests
Comment 2 Tomás Cohen Arazi (tcohen) 2025-01-20 13:10:46 UTC
Created attachment 176796 [details] [review]
Bug 38926: Make POST /biblios return 400 if AddBiblio fails

We don't have proper exceptions in the `C4::Biblio::AddBiblio` method,
but we at least know `$biblio_id` will be `undef` in the even of an
error processing the call.

This patch makes the controller handle this situation so (at least) it
is obvious that something bad happened.

To test:
1. Apply the regression tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/biblios.t
=> FAIL: Tests fail! The endpoint returns 200 even on error!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! The endpoint returns a 400 with a reasonable
message!
5. Sign off :-D
Comment 3 Jonathan Druart 2025-01-20 15:30:54 UTC
should not we list the error_code in the spec?
Comment 4 Tomás Cohen Arazi (tcohen) 2025-01-20 17:14:04 UTC
Created attachment 176835 [details] [review]
Bug 38926: Make POST /biblios return 400 if AddBiblio fails

We don't have proper exceptions in the `C4::Biblio::AddBiblio` method,
but we at least know `$biblio_id` will be `undef` in the even of an
error processing the call.

This patch makes the controller handle this situation so (at least) it
is obvious that something bad happened.

To test:
1. Apply the regression tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/biblios.t
=> FAIL: Tests fail! The endpoint returns 200 even on error!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! The endpoint returns a 400 with a reasonable
message!
5. Sign off :-D
Comment 5 Tomás Cohen Arazi (tcohen) 2025-01-20 17:14:30 UTC
(In reply to Jonathan Druart from comment #3)
> should not we list the error_code in the spec?

Good catch! Fixed.

Thanks!
Comment 6 Jonathan Druart 2025-01-21 08:35:05 UTC
Created attachment 176847 [details] [review]
Bug 38926: Regression tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 7 Jonathan Druart 2025-01-21 08:35:07 UTC
Created attachment 176848 [details] [review]
Bug 38926: Make POST /biblios return 400 if AddBiblio fails

We don't have proper exceptions in the `C4::Biblio::AddBiblio` method,
but we at least know `$biblio_id` will be `undef` in the even of an
error processing the call.

This patch makes the controller handle this situation so (at least) it
is obvious that something bad happened.

To test:
1. Apply the regression tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/biblios.t
=> FAIL: Tests fail! The endpoint returns 200 even on error!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! The endpoint returns a 400 with a reasonable
message!
5. Sign off :-D

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 8 David Nind 2025-01-22 10:04:29 UTC
Created attachment 176890 [details] [review]
Bug 38926: Regression tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2025-01-22 10:04:32 UTC
Created attachment 176891 [details] [review]
Bug 38926: Make POST /biblios return 400 if AddBiblio fails

We don't have proper exceptions in the `C4::Biblio::AddBiblio` method,
but we at least know `$biblio_id` will be `undef` in the even of an
error processing the call.

This patch makes the controller handle this situation so (at least) it
is obvious that something bad happened.

To test:
1. Apply the regression tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/biblios.t
=> FAIL: Tests fail! The endpoint returns 200 even on error!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! The endpoint returns a 400 with a reasonable
message!
5. Sign off :-D

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2025-01-22 10:05:17 UTC
Have added my signoff, so maybe Jonathan's can count as QA.
Comment 11 Katrin Fischer 2025-01-24 11:03:51 UTC
Pushed for 25.05!

Well done everyone, thank you!