Bug 25870 - Add a q_ccl query parameter to /biblios
Summary: Add a q_ccl query parameter to /biblios
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Hammat wele
QA Contact:
URL:
Keywords:
Depends on: 32734
Blocks: 27113
  Show dependency treegraph
 
Reported: 2020-06-24 18:25 UTC by Tomás Cohen Arazi
Modified: 2023-09-20 13:34 UTC (History)
9 users (show)

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


Attachments
Bug 25870 - Add a q_ccl query parameter to /biblios (9.75 KB, patch)
2022-07-26 09:38 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 25870 - (follow-up) Add a q_ccl query parameter to /biblios (12.39 KB, patch)
2022-08-19 09:28 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 25870 - (follow-up) Removed Data::Dumper + call (1.74 KB, patch)
2022-08-19 09:51 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 25870 - (follow-up) Raw MARC-XML records from Zebra now get appropriate treatment (2.15 KB, patch)
2022-08-19 13:14 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 25870: Add a q_ccl query parameter to /biblios (8.96 KB, patch)
2023-03-13 13:37 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 25870: (follow-up) Adding pagination information on the headers, returns an empty result when the query is empty (3.00 KB, patch)
2023-03-15 21:24 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 25870: (follow-up) Adding pagination information on the headers, returns an empty result when the query is empty (4.79 KB, patch)
2023-03-21 13:00 UTC, Hammat wele
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 2020-06-24 18:25:48 UTC
It would be great if we could just pass a CCL query to the /biblios route
Comment 1 Paul Derscheid 2022-07-21 11:12:21 UTC
Should this be a public route?
Comment 2 Paul Derscheid 2022-07-22 16:29:28 UTC
There's another thing: I think that the only Content-Type that makes sense for the return value is 'application/marc-in-json' because we return 1..n records instead of a single one. 

Additionally I can't imagine a use-case in which you wouldn't want JSON as a return value for that route.

Is there any sane way to discern marc records from one another in an api response?
Comment 3 Tomás Cohen Arazi 2022-07-25 11:12:41 UTC
(In reply to Paul Derscheid from comment #2)
> There's another thing: I think that the only Content-Type that makes sense
> for the return value is 'application/marc-in-json' because we return 1..n
> records instead of a single one. 
> 
> Additionally I can't imagine a use-case in which you wouldn't want JSON as a
> return value for that route.
> 
> Is there any sane way to discern marc records from one another in an api
> response?

All of the accepted formats in GET /biblio/{biblio_id} have their plural representations [1]

BUT I think we could start with 'application/marc-in-json'. and add things 'as-needed'. So if you want to work on this, it'd be ok to just implement marc-in-json for now.

[1] "application/json" => this is the biblio + biblioitem table, plural is an arrary,
    "application/marcxml+xml" => there's '<collection>' for multiple records,
    "application/marc-in-json" => as you said, an array,
    "application/marc" => concatenated usmarc records would work,
    "text/plain" => double new line separator would work.
Comment 4 Paul Derscheid 2022-07-26 09:38:04 UTC
Created attachment 138123 [details] [review]
Bug 25870 - Add a q_ccl query parameter to /biblios

This patch adds new functionality to the existing /public/biblios route. Instead of specifying a biblio by a biblionumber as a path parameter, you can use the new q_ccl query parameter to run a query on the endpoint.

To test:
1) Apply the patch
2) Pick an endpoint tester of your choice, e.g. Insomnia or the ThunderClient if you use VSCode or derivatives.
3) Run a query while using Zebra.
4) Observe the marc-in-json response and check for validity.
5) Run a query while using Elasticsearch.
6) Again, observe the marc-in-json response and check for validity.
7) I'd love to say: 'Sign-Off' but the unit tests need work and I need help with that, so.. an attachment or guidance would be great.
Comment 5 Paul Derscheid 2022-07-26 09:43:34 UTC
The other issue is that this route can be abused for DOS-attacks which must be handled somehow.
Comment 6 Kyle M Hall 2022-07-26 10:46:26 UTC
(In reply to Paul Derscheid from comment #5)
> The other issue is that this route can be abused for DOS-attacks which must
> be handled somehow.

Wouldn't the simplest solution be to make it *not* public, and just require the catalogue permission?

Another option would be to bake in a rate limiting middleware such as https://metacpan.org/pod/Plack::Middleware::Throttle or https://metacpan.org/pod/Plack::Middleware::Throttle::Lite
Comment 7 Paul Derscheid 2022-07-26 10:49:20 UTC
That's right but at LMSCloud we need this as a public route and tcohen also wants this to be public, so the simple approach doesn't fly. 

Thanks for the pointers, I'll have a look in a bit.
Comment 8 Kyle M Hall 2022-07-26 10:50:05 UTC
Also https://metacpan.org/pod/Plack::Middleware::Cerberus

We need something that throttles by the minute or even second if possible.
Comment 9 Paul Derscheid 2022-07-26 11:14:05 UTC
Another question is what a sane 'number of requests/interval' would look like.
Comment 10 Kyle M Hall 2022-07-26 11:27:59 UTC
(In reply to Paul Derscheid from comment #9)
> Another question is what a sane 'number of requests/interval' would look
> like.

I think that depends on the power of the server, and your use case. What will you be using this API for?
Comment 11 Tomás Cohen Arazi 2022-07-26 11:31:39 UTC
(In reply to Paul Derscheid from comment #5)
> The other issue is that this route can be abused for DOS-attacks which must
> be handled somehow.

Isn't that already the case for bots hitting opac-search.pl?

I think we need to introduce some tool for throttling, but not necessarily as part of this dev, as it is really tricky (e.g. it will be common for campus sites to be queried many times from a transparent proxy on the LAN so we will also need to be able to configure exceptions...).
Comment 12 Tomás Cohen Arazi 2022-07-26 11:33:13 UTC
(In reply to Tomás Cohen Arazi from comment #11)
> (In reply to Paul Derscheid from comment #5)
> > The other issue is that this route can be abused for DOS-attacks which must
> > be handled somehow.
> 
> Isn't that already the case for bots hitting opac-search.pl?

What I meant to say is that introducing this route just adds another place we need to take care of. But the solution is needed in general. Maybe at Apache level.
Comment 13 Kyle M Hall 2022-07-26 11:34:19 UTC
(In reply to Tomás Cohen Arazi from comment #11)
> (In reply to Paul Derscheid from comment #5)
> > The other issue is that this route can be abused for DOS-attacks which must
> > be handled somehow.
> 
> Isn't that already the case for bots hitting opac-search.pl?
> 
> I think we need to introduce some tool for throttling, but not necessarily
> as part of this dev, as it is really tricky (e.g. it will be common for
> campus sites to be queried many times from a transparent proxy on the LAN so
> we will also need to be able to configure exceptions...).

That is a good point. Now, we just need to add throttling to this api, then use that api in opac-search.... ;)

Tomas, at this time I think you have the ultimate authority to declare throttling "out of scope" and move it to a new bug report.
Comment 14 Tomás Cohen Arazi 2022-07-26 11:35:31 UTC
(In reply to Paul Derscheid from comment #7)
> That's right but at LMSCloud we need this as a public route and tcohen also
> wants this to be public, so the simple approach doesn't fly. 

We can have both, really. And when we say 'public' we mean 'unprivileged access' which could or not imply authentication.
Comment 15 Tomás Cohen Arazi 2022-07-26 11:36:36 UTC
(In reply to Kyle M Hall from comment #13)
> That is a good point. Now, we just need to add throttling to this api, then
> use that api in opac-search.... ;)
>
> Tomas, at this time I think you have the ultimate authority to declare
> throttling "out of scope" and move it to a new bug report.

I think we should file a separate bug, yes. And I wouldn't make this one dependent on the new one.
Comment 16 Kyle M Hall 2022-07-26 11:38:17 UTC
I'd really like to see us get away from Apache based solutions, but at the same time it's a very practical way to handle it that works for 99.9% of Koha users. If there exists middleware that would work for Koha, that would be a better solution IMO, especially because we can bake the throttling settings directly into Koha.

Now, I think we've done enough bike-shedding. We should move this discussion to a throttling specific bug report!
Comment 17 Tomás Cohen Arazi 2022-07-26 11:45:38 UTC
Comment on attachment 138123 [details] [review]
Bug 25870 - Add a q_ccl query parameter to /biblios

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

::: Koha/REST/V1/Biblios.pm
@@ +160,5 @@
> +    my $record_processor = Koha::RecordProcessor->new(
> +        {
> +            filters => 'ViewPolicy',
> +            options => {
> +                interface => 'opac',

I think you could implement only a list() method, and rely on $c->stash('is_public') to choose staff vs. public. And later check the OpacHiddenItems stuff accordingly.

@@ +177,5 @@
> +            }
> +        );
> +    }
> +
> +    sub format_record_by_content_type {

This inline subs look really untidy.

@@ +243,5 @@
> +        }
> +
> +        my $response =
> +          format_record_by_content_type(
> +            { content_type => $requested_content_type, records => \@records } );

It feels like this method call should be placed in the data => portion of the respond_to, with the right parameters for each case instead of 'detecting it' inside format_record_... But as I said, I'm still not convinced by those subs.
Comment 18 David Cook 2022-07-27 05:22:22 UTC
(In reply to Kyle M Hall from comment #16)
> I'd really like to see us get away from Apache based solutions, but at the
> same time it's a very practical way to handle it that works for 99.9% of
> Koha users. If there exists middleware that would work for Koha, that would
> be a better solution IMO, especially because we can bake the throttling
> settings directly into Koha.
 
I've just raised bug 31242
Comment 19 David Cook 2022-07-27 05:38:20 UTC
(In reply to Tomás Cohen Arazi from comment #14)
> (In reply to Paul Derscheid from comment #7)
> > That's right but at LMSCloud we need this as a public route and tcohen also
> > wants this to be public, so the simple approach doesn't fly. 
> 
> We can have both, really. And when we say 'public' we mean 'unprivileged
> access' which could or not imply authentication.

I know I struggle to wrap my head around how the API is organised as well... 

If I recall correctly, I think the majority of API endpoints are for admin/staff users and the "public" APIs are for public/OPAC users. 

Public users don't have authorizations (hence "unprivileged access"). 

Some public APIs require authentication and some don't.
Comment 20 David Cook 2022-07-27 05:39:43 UTC
(In reply to Paul Derscheid from comment #7)
> That's right but at LMSCloud we need this as a public route and tcohen also
> wants this to be public, so the simple approach doesn't fly. 
> 
> Thanks for the pointers, I'll have a look in a bit.

I'd like this as a public route as well, as I'd like to point a third-party at it, so that they can search Koha in an event-driven way.
Comment 21 David Cook 2022-07-27 05:42:15 UTC
(In reply to David Cook from comment #20)
> (In reply to Paul Derscheid from comment #7)
> > That's right but at LMSCloud we need this as a public route and tcohen also
> > wants this to be public, so the simple approach doesn't fly. 
> > 
> > Thanks for the pointers, I'll have a look in a bit.
> 
> I'd like this as a public route as well, as I'd like to point a third-party
> at it, so that they can search Koha in an event-driven way.

Although for my use case I could go with a non-public route and just give them a "catalogue" authorized user like Kyle suggested. 

But that wouldn't work if we wanted opac-search.pl to use it.

But... we could start with non-public and then add public later if we're concerned. They could probably share the controller even.
Comment 22 Paul Derscheid 2022-08-19 09:28:59 UTC
Created attachment 139452 [details] [review]
Bug 25870 - (follow-up) Add a q_ccl query parameter to /biblios

I refactored the controller code but unfortunately still haven't figured out why Koha::Biblios->search doesn't work properly. For the time being I still use Koha::Biblios->find. That aside I think it has become much cleaner now.

To test:
1) Apply the patch
2) Pick an endpoint tester of your choice, e.g. Insomnia or the ThunderClient if you use VSCode or derivatives.
3) Run a query while using Zebra.
4) Observe the marc-in-json response and check for validity.
5) Run a query while using Elasticsearch.
6) Again, observe the marc-in-json response and check for validity.
7) Not ready for sign-off but please leave a comment or help me on the Koha::Biblios->search thing.
Comment 23 Paul Derscheid 2022-08-19 09:51:11 UTC
Created attachment 139469 [details] [review]
Bug 25870 - (follow-up) Removed Data::Dumper + call

I refactored the controller code but unfortunately still haven't figured out why Koha::Biblios->search doesn't work properly. For the time being I still use Koha::Biblios->find. That aside I think it has become much cleaner now.

To test:
1) Apply the patch
2) Pick an endpoint tester of your choice, e.g. Insomnia or the ThunderClient if you use VSCode or derivatives.
3) Run a query while using Zebra.
4) Observe the marc-in-json response and check for validity.
5) Run a query while using Elasticsearch.
6) Again, observe the marc-in-json response and check for validity.
7) Not ready for sign-off but please leave a comment or help me on the Koha::Biblios->search thing.
Comment 24 Paul Derscheid 2022-08-19 13:14:09 UTC
Created attachment 139491 [details] [review]
Bug 25870 - (follow-up) Raw MARC-XML records from Zebra now get appropriate treatment

To test:
1) Apply the patch
2) Pick an endpoint tester of your choice, e.g. Insomnia or the ThunderClient if you use VSCode or derivatives.
3) Run a query while using Zebra.
4) Observe the marc-in-json response and check for validity.
5) Run a query while using Elasticsearch.
6) Again, observe the marc-in-json response and check for validity.
7) Not ready for sign-off but please leave a comment or help me on the Koha::Biblios->search thing.
Comment 25 Tomás Cohen Arazi 2023-02-21 13:33:19 UTC
Paul, please redo on top of bug 32734. Let me know if you cannot work on it, so we trace a path. This needs tests as well.
Comment 26 Paul Derscheid 2023-02-21 15:23:10 UTC
I'll estimate the workload and get back to you, Tomas.
Comment 27 Paul Derscheid 2023-02-22 16:10:55 UTC
Tomas, I'll continue working on this next week if that's alright with you. I'd love to do it immediately but I'm really swamped this week.
Comment 28 Hammat wele 2023-03-13 13:37:13 UTC
Created attachment 148117 [details] [review]
Bug 25870: Add a q_ccl query parameter to /biblios

To test:
1) Apply the patch
2) Pick an endpoint tester of your choice, e.g. Insomnia or the ThunderClient if you use VSCode or derivatives.
3) Run a query while using Zebra.
4) Observe the marc-in-json response and check for validity.
5) Run a query while using Elasticsearch.
6) Again, observe the marc-in-json response and check for validity.
7) Not ready for sign-off but please leave a comment or help me on the Koha::Biblios->search thing.
Comment 29 Hammat wele 2023-03-13 13:47:32 UTC
Redoing this bug on top of the bug 32734
Comment 30 Tomás Cohen Arazi 2023-03-14 11:32:34 UTC
A few comments on the preliminary code:

* An empty query should return an empty resultset, not a 404
* Koha::SearchEngine::Search->extract_biblionumber should be used instead of reinventing it
* Pagination information is missing on the headers. It should be extracted from the query and added. Please look at $c->add_pagination_headers
Comment 31 Hammat wele 2023-03-15 21:24:59 UTC
Created attachment 148247 [details] [review]
Bug 25870: (follow-up) Adding pagination information on the headers, returns an empty result when the query is empty
Comment 32 Tomás Cohen Arazi 2023-03-15 21:38:44 UTC
(In reply to Hammat wele from comment #31)
> Created attachment 148247 [details] [review] [review]
> Bug 25870: (follow-up) Adding pagination information on the headers, returns
> an empty result when the query is empty

Well :-D

You are extracting pagination parameters from the (HTTP) query, good. But they are not being passed to the backend (in the form of offset + limit I think, as they should.

Also, you need to know that the add_pagination_headers helper needs two totals:

- the page total if the query is paginated (it would always be the case)
- the total records from which the page was taken (base_total).
Comment 33 Hammat wele 2023-03-21 13:00:26 UTC
Created attachment 148446 [details] [review]
Bug 25870: (follow-up) Adding pagination information on the headers, returns an empty result when the query is empty
Comment 34 Hammat wele 2023-03-21 13:01:06 UTC
I still need help with Koha::Biblios->search thing. how to use it to search biblio hidden_in_opac. With the Koha::Biblios->search adding the pagination would be more easier.
Comment 35 Jonathan Druart 2023-04-21 06:34:43 UTC
(In reply to Hammat wele from comment #34)
> I still need help with Koha::Biblios->search thing. how to use it to search
> biblio hidden_in_opac. With the Koha::Biblios->search adding the pagination
> would be more easier.

How that?
You need to rewrite ->hidden_in_opac, to build a parameter hashref from the syspref, then pass it to ->search

I haven't had a deep look but I think it is possible and should work.
Comment 36 Tomás Cohen Arazi 2023-04-21 12:25:07 UTC
You really need to perform the search against the search engine, with any implicit filtering like OpacHiddenItems added to the CCL query. Otherwise you would kill the interesting bit of searching on the backend.
Comment 37 David Cook 2023-04-23 23:54:30 UTC
(In reply to Jonathan Druart from comment #35)
> (In reply to Hammat wele from comment #34)
> > I still need help with Koha::Biblios->search thing. how to use it to search
> > biblio hidden_in_opac. With the Koha::Biblios->search adding the pagination
> > would be more easier.
> 
> How that?
> You need to rewrite ->hidden_in_opac, to build a parameter hashref from the
> syspref, then pass it to ->search
> 
> I haven't had a deep look but I think it is possible and should work.

I don't know if all item DB fields have matching index names/aliases, so that would be something that needs reviewing.

(In reply to Tomás Cohen Arazi from comment #36)
> You really need to perform the search against the search engine, with any
> implicit filtering like OpacHiddenItems added to the CCL query. Otherwise
> you would kill the interesting bit of searching on the backend.

Yeah, it'll need both OpacSuppression, OpacSuppressionByIPRange, and OpacHiddenItems checking.
Comment 38 Katrin Fischer 2023-07-01 14:19:15 UTC
As this is blocking the much desired 27113 - what can we do to help move this along?
Comment 39 Marcel de Rooy 2023-09-20 13:34:14 UTC
Any plans to get this moving again ?