Bug 17371 - Add routes for biblios [OMNIBUS]
Summary: Add routes for biblios [OMNIBUS]
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement with 1 vote (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL: https://wiki.koha-community.org/wiki/...
Keywords:
: 17007 (view as bug list)
Depends on: 28201 30790 30799 21334 23677 24909
Blocks:
  Show dependency treegraph
 
Reported: 2016-09-28 21:42 UTC by Benjamin Rokseth
Modified: 2022-10-14 12:08 UTC (History)
14 users (show)

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


Attachments
Bug 17371 - REST API: add CRUD for biblios (17.73 KB, patch)
2016-09-29 00:20 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17371 - REST API: add CRUD for biblios (17.89 KB, patch)
2017-01-15 16:07 UTC, Jiri Kozlovsky
Details | Diff | Splinter Review
Added filtering OPAChiddenItems (2.74 KB, patch)
2017-01-15 19:44 UTC, Jiri Kozlovsky
Details | Diff | Splinter Review
Bug 17371 - REST API: add CRUD for biblios (QA followup) (5.47 KB, patch)
2017-07-06 09:56 UTC, Jiri Kozlovsky
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Rokseth 2016-09-28 21:42:40 UTC
This bug proposes to add full CRUD for biblios to the REST API, meaning a basic endpoint for managing records outside of Koha staff. It will make use of Koha Objects and largely replicate the /svc/biblios functionality, but from a RESTful perspective.

Proposed routes :

GET    /api/v1/biblios/{biblionumber}                 Get a simple record
GET    /api/v1/biblios/{biblionumber}/items           Get a record with items
GET    /api/v1/biblios/{biblionumber}/expanded        Get a record with items and item statuses
POST   /api/v1/biblios/{biblionumber}/{frameworkcode} Create a record with items (and optional framework)
PUT    /api/v1/biblios/{biblionumber}/{frameworkcode} Update a record without touching items (and optional framework)
DELETE /api/v1/biblios/{biblionumber}                 Delete a record with items

POST and PUT will take marcxml as input
Comment 1 Benjamin Rokseth 2016-09-29 00:20:14 UTC
Created attachment 55898 [details] [review]
Bug 17371 - REST API: add CRUD for biblios

Patch to add CRUD to biblios.

Test plan:
1) add dependent bug 17004
2) create some staff user with editcatalogue permission
3) play with API with routes explained in bug description
4) run test t/db_dependent/api/v1/biblios.t
Comment 2 Josef Moravec 2016-10-25 11:21:22 UTC
Shouldn't this be "Needs signoff"?
Comment 3 Magnus Enger 2016-11-02 13:57:49 UTC
Setting this to "needs signoff", please correct me if I'm wrong!
Comment 4 Jiri Kozlovsky 2016-11-09 20:17:47 UTC
What if you visit items endpoint in a record with really huge amount of items? I think it would actually timeout, depending on the server or proxy configuration serving this endpoint. Wouldn't it be better to return only list of itemnumbers so that the user can fetch let's say only first 10 items and after he want to see next, he fetches another 10 items?
Comment 5 Benjamin Rokseth 2016-11-15 10:12:21 UTC
(In reply to Jiri Kozlovsky from comment #4)
> What if you visit items endpoint in a record with really huge amount of
> items? I think it would actually timeout, depending on the server or proxy
> configuration serving this endpoint. Wouldn't it be better to return only
> list of itemnumbers so that the user can fetch let's say only first 10 items
> and after he want to see next, he fetches another 10 items?

Depends on what you mean by huge amount? thousands? We have no issue on hundreds, although it takes some seconds. The API strives to be agnostic of the DB and merely uses Koha Objects  ($biblio->items). Getting only the IDs would actually be more code and logic in the API than we want.

However, if this is an issue, I would rather suggest adding a method to Koha::Biblios to get the IDs of the attached items only, and then the API could reflect that with another route.
Comment 6 Jiri Kozlovsky 2016-11-20 13:16:17 UTC
(In reply to Benjamin Rokseth from comment #5)
> (In reply to Jiri Kozlovsky from comment #4)
> > What if you visit items endpoint in a record with really huge amount of
> > items? I think it would actually timeout, depending on the server or proxy
> > configuration serving this endpoint. Wouldn't it be better to return only
> > list of itemnumbers so that the user can fetch let's say only first 10 items
> > and after he want to see next, he fetches another 10 items?
> 
> Depends on what you mean by huge amount? thousands? We have no issue on
> hundreds, although it takes some seconds. The API strives to be agnostic of
> the DB and merely uses Koha Objects  ($biblio->items). Getting only the IDs
> would actually be more code and logic in the API than we want.
> 
> However, if this is an issue, I would rather suggest adding a method to
> Koha::Biblios to get the IDs of the attached items only, and then the API
> could reflect that with another route.

I agree with that, it seems like much cleaner approach. I also think the problem I've came up with is not critical and it will not occur much.
Comment 7 Jiri Kozlovsky 2016-12-11 23:03:22 UTC
Comment on attachment 55898 [details] [review]
Bug 17371 - REST API: add CRUD for biblios

Review of attachment 55898 [details] [review]:
-----------------------------------------------------------------

Nice work by the way!

::: Koha/REST/V1/Biblio.pm
@@ +45,5 @@
> +    my $biblio = Koha::Biblios->find($args->{biblionumber});
> +    unless ($biblio) {
> +        return $c->$cb({error => "Biblio not found"}, 404);
> +    }
> +    return $c->$cb({ biblio => $biblio->unblessed, items => $biblio->items->unblessed }, 200);

Does this respect OpacHiddenItems preference?

@@ +79,5 @@
> +
> +        # mark as onhold if item marked as hold
> +        if (scalar(@{$holds}) > 0) {
> +            $item->{status} = "onhold";
> +        }

I'm missing the next statement after one of the conditions succeeds. I can see no reason to check for another condition, which can result in not necessary DB querying.
Comment 8 Jiri Kozlovsky 2017-01-15 16:07:36 UTC
Created attachment 58995 [details] [review]
[SIGNED-OFF] Bug 17371 - REST API: add CRUD for biblios

Patch to add CRUD to biblios.

Test plan:
1) add dependent bug 17004
2) create some staff user with editcatalogue permission
3) play with API with routes explained in bug description
4) run test t/db_dependent/api/v1/biblios.t

Signed-off-by: Jiri Kozlovsky <mail@jkozlovsky.cz>

Note that I've rebased this patch on top of master
Comment 9 Jiri Kozlovsky 2017-01-15 16:14:26 UTC
(In reply to Jiri Kozlovsky from comment #8)
> Created attachment 58995 [details] [review] [review]
> Bug 17371 - REST API: add CRUD for biblios
> 
> Patch to add CRUD to biblios.
> 
> Test plan:
> 1) add dependent bug 17004
> 2) create some staff user with editcatalogue permission
> 3) play with API with routes explained in bug description
> 4) run test t/db_dependent/api/v1/biblios.t
> 
> Signed-off-by: Jiri Kozlovsky <mail@jkozlovsky.cz>
> 
> Note that I've rebased this patch on top of master

Test plan was changed actually - I've cut off the dependency on bug 17004, which is not really needed for this API to work. Also there is a chance, the 17004 is going to be implemented somehow differently .. see comments till new year.
Comment 10 Jiri Kozlovsky 2017-01-15 16:20:01 UTC
(In reply to Jiri Kozlovsky from comment #7)
> Comment on attachment 55898 [details] [review] [review]
> Bug 17371 - REST API: add CRUD for biblios
> 
> Review of attachment 55898 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Nice work by the way!
> 
> ::: Koha/REST/V1/Biblio.pm
> @@ +45,5 @@
> > +    my $biblio = Koha::Biblios->find($args->{biblionumber});
> > +    unless ($biblio) {
> > +        return $c->$cb({error => "Biblio not found"}, 404);
> > +    }
> > +    return $c->$cb({ biblio => $biblio->unblessed, items => $biblio->items->unblessed }, 200);
> 
> Does this respect OpacHiddenItems preference?

I'm going to implement this in next patch - basically just migrate completed work from bug 17007 so that it can be closed as duplicate. This bug has much more to offer.

> 
> @@ +79,5 @@
> > +
> > +        # mark as onhold if item marked as hold
> > +        if (scalar(@{$holds}) > 0) {
> > +            $item->{status} = "onhold";
> > +        }
> 
> I'm missing the next statement after one of the conditions succeeds. I can
> see no reason to check for another condition, which can result in not
> necessary DB querying.

I've probably misunderstood something - now I can't see anything being wrong about this chunk of code.
Comment 11 Jiri Kozlovsky 2017-01-15 19:44:32 UTC
Created attachment 58996 [details] [review]
Added filtering OPAChiddenItems

Test plan:
    1) Create OpacHiddenItems rules to hide chosen items
    2) Check that regular user doesn't see those items through the API
    3) Check that librarian can see them through the API
Comment 12 Jiri Kozlovsky 2017-01-15 19:47:07 UTC
Comment on attachment 58995 [details] [review]
[SIGNED-OFF] Bug 17371 - REST API: add CRUD for biblios

Changed original patch name to include [SIGNED-OFF] to be clear what is signed off and what needs to be signed off
Comment 13 Jiri Kozlovsky 2017-01-15 19:54:35 UTC
*** Bug 17007 has been marked as a duplicate of this bug. ***
Comment 14 Josef Moravec 2017-01-17 20:31:10 UTC
QA tool complains:

 FAIL	t/db_dependent/api/v1/biblios.t
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 74)
		forbidden pattern: tab char (line 70)
		forbidden pattern: tab char (line 84)
		forbidden pattern: tab char (line 71)
		forbidden pattern: tab char (line 85)
		forbidden pattern: tab char (line 82)
		forbidden pattern: tab char (line 80)
		forbidden pattern: tab char (line 83)
		forbidden pattern: tab char (line 68)
		forbidden pattern: tab char (line 69)
		forbidden pattern: tab char (line 66)
		forbidden pattern: tab char (line 76)
		forbidden pattern: tab char (line 72)
		forbidden pattern: tab char (line 86)
		forbidden pattern: tab char (line 64)
		forbidden pattern: tab char (line 75)
		forbidden pattern: tab char (line 67)

tests failing:

t/db_dependent/api/v1/biblios.t .............. 1/3 substr outside of string at /usr/share/perl5/MARC/File/XML.pm line 561.
Use of uninitialized value $enc in string eq at /usr/share/perl5/MARC/File/XML.pm line 563.
Use of uninitialized value $enc in string eq at /usr/share/perl5/MARC/File/XML.pm line 563.
Use of uninitialized value $enc in string eq at /usr/share/perl5/MARC/File/XML.pm line 565.
Use of uninitialized value $enc in concatenation (.) or string at /usr/share/perl5/MARC/File/XML.pm line 568.
    # No tests run!
t/db_dependent/api/v1/biblios.t .............. 2/3 
#   Failed test 'No tests run for subtest "Create biblio"'
#   at t/db_dependent/api/v1/biblios.t line 77.
Unsupported UNIMARC character encoding [] for XML output for UNIMARC; 100$a -> Christie, Agatha, at /usr/share/perl5/MARC/File/XML.pm line 568.

t/db_dependent/api/v1/swagger/definitions.t .. 
    #   Failed test 'Columns is nullable in DB, not in swagger file for biblio: datecreated, timestamp'
Comment 15 Jiri Kozlovsky 2017-07-06 09:56:38 UTC
Created attachment 64842 [details] [review]
Bug 17371 - REST API: add CRUD for biblios (QA followup)

QA followup

Note: Rebased on master
Comment 16 Jonathan Druart 2017-09-26 18:35:58 UTC
CONFLICT (content): Merge conflict in api/v1/swagger/definitions.json

I tried to rebase but the tests then fail.
Comment 17 Tomás Cohen Arazi 2018-02-27 13:57:44 UTC
I've added the ''needs_rfc'' keyword to this bug. The community process requires a vote on the RFC, so the dev and interested parties should take care of adding the RFC to the agenda on a dev meeting, and gather support for it.

Take a look here: https://wiki.koha-community.org/wiki/REST_api_RFCs
Comment 18 Tomás Cohen Arazi 2018-03-29 14:55:36 UTC
I've added an RFC for everyone to contribute. It is ni the lines of this patches with a broader target use cases in mind. Still WIP but the implementation should be pretty simple.
Comment 19 Tomás Cohen Arazi 2019-09-25 19:57:54 UTC
I'm taking over this bug, somehow. Will file separate bugs for separate use cases, and ask the original authors to add their comments there.
Comment 20 Johanna Räisä 2019-10-11 07:44:50 UTC
I have a suggestion to POST and PUT endpoints. At the moment the code doesn't validate or compare coming MARC record. On POST endpoint there should be some kind of validation at least checking that the control numbers exist. On PUT we should check matching rules so libraries can choose what they want to do with the fields. Matcher_id check could be added as an optional parameter.
Comment 21 Rickard Lindfors 2021-02-16 08:39:08 UTC
Hi! We use the API for our discovery to get item information, but that's one api call per item. It would be nice to just use one call from the biblio.

How about to add the get functions of this bug as starter, and add the post/put function later? They should be easier to implement.
Comment 22 Jonathan Druart 2021-02-16 09:06:16 UTC
(In reply to Rickard Lindfors from comment #21)
> Hi! We use the API for our discovery to get item information, but that's one
> api call per item. It would be nice to just use one call from the biblio.
> 
> How about to add the get functions of this bug as starter, and add the
> post/put function later? They should be easier to implement.

See bug 27358.