Bug 24502 - Add a query language and param (q=) to the API
Summary: Add a query language and param (q=) to the API
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Agustín Moyano
QA Contact:
URL:
Keywords:
Depends on: 24356 24487
Blocks: 24615 25514
  Show dependency treegraph
 
Reported: 2020-01-23 23:08 UTC by Agustín Moyano
Modified: 2020-11-30 21:48 UTC (History)
10 users (show)

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


Attachments
Bug 24502: object.search also filter by prefetched columns (9.18 KB, patch)
2020-01-25 03:08 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24502: object.search also filter by prefetched columns (9.31 KB, patch)
2020-01-30 19:27 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24502: (follow-up) Add default q parameter in api definition (983 bytes, patch)
2020-01-30 19:27 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24502: object.search also filter by prefetched columns (15.30 KB, patch)
2020-02-07 22:10 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24502: (follow-up) Add all 3 query parameters in api definition (1.57 KB, patch)
2020-02-07 22:11 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24502: object.search also filter by prefetched columns (15.38 KB, patch)
2020-02-08 03:05 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24502: (follow-up) Add all 3 query parameters in api definition (1.57 KB, patch)
2020-02-08 03:05 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24502: (follow-up) Add all 3 query parameters in api definition (1.58 KB, patch)
2020-02-08 14:36 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24502: object.search also filter by prefetched columns (15.41 KB, patch)
2020-02-19 14:09 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24502: (follow-up) Add all 3 query parameters in api definition (1.58 KB, patch)
2020-02-19 14:09 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24502: object.search also filter by prefetched columns (15.48 KB, patch)
2020-02-21 07:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24502: (follow-up) Add all 3 query parameters in api definition (1.64 KB, patch)
2020-02-21 07:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24502: object.search also filter by prefetched columns (15.55 KB, patch)
2020-03-02 16:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24502: (follow-up) Add all 3 query parameters in api definition (1.71 KB, patch)
2020-03-02 16:30 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Agustín Moyano 2020-01-23 23:08:19 UTC
Make object.search helper filter by prefetched columns
Comment 1 Agustín Moyano 2020-01-25 03:08:43 UTC
Created attachment 97934 [details] [review]
Bug 24502: object.search also filter by prefetched columns

This patch adds the possibility to object.search helper, to also filter by prefetched columns.

In order to dynamically add filter parameters, they must be coded as json string, and sent within the q parameter.

The coded json, is in fact dbix syntax.

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t t/db_dependent/Koha/REST/Plugin/Objects.t
3. Sign off
Comment 2 Martin Renvoize 2020-01-27 12:17:03 UTC
How about taking ES and an example for DSL and Querying structures..

They allow for a simple syntax in the query string, but if you want to 'level up' your queries then they use JSON in a request body.. 

I like this work so far, but I'm not a big fan of escaped JSON directly in the query string, it feels a bit messy and feels like it could end up leading to escaping problems down the line.  Moving the JSON based query DSL into a GET request body seems like a good move to me.
Comment 4 Martin Renvoize 2020-01-27 12:21:28 UTC
Tomas also asked me to mention how I did similar when coding another project.. just for possible inspiration...


I used 'Search::Query::Dialect' and adopted the DBIx::Class dialect with some customised additions to get it working.. it leads to a reasonably nice functional query string syntax for mostly simple queries.. I don't remember getting as far as doing full-on JSON based body queries though.. so I'm not sure how helpful it would be here?
Comment 5 Agustín Moyano 2020-01-30 16:46:21 UTC
(In reply to Martin Renvoize from comment #2)
> How about taking ES and an example for DSL and Querying structures..
> 
> They allow for a simple syntax in the query string, but if you want to
> 'level up' your queries then they use JSON in a request body.. 
> 
> I like this work so far, but I'm not a big fan of escaped JSON directly in
> the query string, it feels a bit messy and feels like it could end up
> leading to escaping problems down the line.  Moving the JSON based query DSL
> into a GET request body seems like a good move to me.

Hi Martin, agreed, I'll put the q parameter in the body.
Comment 6 Agustín Moyano 2020-01-30 16:54:59 UTC
(In reply to Martin Renvoize from comment #4)
> Tomas also asked me to mention how I did similar when coding another
> project.. just for possible inspiration...
> 
> 
> I used 'Search::Query::Dialect' and adopted the DBIx::Class dialect with
> some customised additions to get it working.. it leads to a reasonably nice
> functional query string syntax for mostly simple queries.. I don't remember
> getting as far as doing full-on JSON based body queries though.. so I'm not
> sure how helpful it would be here?

Regarding DSL, I didn't find it was illogical the one dbic proposes.. it certainly has ugly names like "-or" or "-and", but the rest it's quite similar.

Maybe we can discuss it later on a following bug?
Comment 7 Martin Renvoize 2020-01-30 17:56:50 UTC
All sounds good to me. 😀
Comment 8 Agustín Moyano 2020-01-30 19:27:18 UTC
Created attachment 98185 [details] [review]
Bug 24502: object.search also filter by prefetched columns

This patch adds the possibility to object.search helper, to also filter by prefetched columns.

In order to dynamically add filter parameters, they must be coded as json and placed in the body of the request, within the q parameter.

The coded json, is in fact dbix syntax.

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t t/db_dependent/Koha/REST/Plugin/Objects.t
3. Sign off
Comment 9 Agustín Moyano 2020-01-30 19:27:23 UTC
Created attachment 98186 [details] [review]
Bug 24502: (follow-up) Add default q parameter in api definition
Comment 10 David Nind 2020-01-31 07:25:38 UTC
I get this message when trying to apply this patch (git bz apply 24502):

 Failed to retrieve bug information: NotPermitted

Other bugs apply fine.
Comment 11 David Nind 2020-02-02 01:48:04 UTC
(In reply to David Nind from comment #10)
> I get this message when trying to apply this patch (git bz apply 24502):
> 
>  Failed to retrieve bug information: NotPermitted

Figured it out - a dependency is security related.

Will let someone else test who has the right access!
Comment 12 Agustín Moyano 2020-02-05 16:09:21 UTC
(In reply to David Nind from comment #11)
> (In reply to David Nind from comment #10)
> > I get this message when trying to apply this patch (git bz apply 24502):
> > 
> >  Failed to retrieve bug information: NotPermitted
> 
> Figured it out - a dependency is security related.
> 
> Will let someone else test who has the right access!

Thaks for trying anyway!
Comment 13 Agustín Moyano 2020-02-05 16:19:35 UTC
Hi Martin, I tryed to use this version of q parameter to be sent into the body of a GET request, but unfortunately browsers don't allow that kind of messages.

I stripped the code to de bare bones of JS creating a XMLHTTPRequest and it's replacement, the "Fetch API", but nither of both sends body content with a GET request. The first one ignores the content if the method is GET, and the second one throws an exception.

I would suggest to keep q parameter in body, but also to have a q parameter in the query, and maybe also in a header (something like 'x-koha-query')

So if you are using any server side program to fetch from de API, you can send body content in the GET request, but if you are using the browser you can choose to use q parameter in the query, or if you want to keep it clean, use the x-koha-query header.

Regards
Comment 14 Agustín Moyano 2020-02-07 22:10:57 UTC
Created attachment 98596 [details] [review]
Bug 24502: object.search also filter by prefetched columns

This patch adds the possibility to object.search helper, to also filter by prefetched columns.

In order to dynamically add filter parameters, they must be coded as json and placed in the body of the request, coded as string in 'q' query parameter or as string in 'x-koha-query' header.

The coded json, is in fact dbix syntax.

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t t/db_dependent/Koha/REST/Plugin/Objects.t
3. Sign off
Comment 15 Agustín Moyano 2020-02-07 22:11:01 UTC
Created attachment 98597 [details] [review]
Bug 24502: (follow-up) Add all 3 query parameters in api definition

This patch adds api definition for body, query or header parameters.

To use them include:

* "$ref": "../parameters.json#/q_body" for body parameter.
* "$ref": "../parameters.json#/q_param" for query parameter.
* "$ref": "../parameters.json#/q_header" for header parameter.
Comment 16 Agustín Moyano 2020-02-08 03:05:51 UTC
Created attachment 98605 [details] [review]
Bug 24502: object.search also filter by prefetched columns

This patch adds the possibility to object.search helper, to also filter by prefetched columns.

In order to dynamically add filter parameters, they must be coded as json and placed in the body of the request, coded as string in 'q' query parameter or as string in 'x-koha-query' header.

The coded json, is in fact dbix syntax.

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t t/db_dependent/Koha/REST/Plugin/Objects.t
3. Sign off
Comment 17 Agustín Moyano 2020-02-08 03:05:56 UTC
Created attachment 98606 [details] [review]
Bug 24502: (follow-up) Add all 3 query parameters in api definition

This patch adds api definition for body, query or header parameters.

To use them include:

* "$ref": "../parameters.json#/q_body" for body parameter.
* "$ref": "../parameters.json#/q_param" for query parameter.
* "$ref": "../parameters.json#/q_header" for header parameter.
Comment 18 Agustín Moyano 2020-02-08 14:36:16 UTC
Created attachment 98611 [details] [review]
Bug 24502: (follow-up) Add all 3 query parameters in api definition

This patch adds api definition for body, query or header parameters.

To use them include:

* "$ref": "../parameters.json#/q_body" for body parameter.
* "$ref": "../parameters.json#/q_param" for query parameter.
* "$ref": "../parameters.json#/q_header" for header parameter.
Comment 19 Agustín Moyano 2020-02-09 12:37:38 UTC
(In reply to David Nind from comment #11)
> (In reply to David Nind from comment #10)
> > I get this message when trying to apply this patch (git bz apply 24502):
> > 
> >  Failed to retrieve bug information: NotPermitted
> 
> Figured it out - a dependency is security related.
> 
> Will let someone else test who has the right access!

Hi David, bug 24487 is now in master. Would you please try again?

Thanks
Comment 20 David Nind 2020-02-09 18:44:55 UTC
> Hi David, bug 24487 is now in master. Would you please try again?

The bug is still restricted, so not able to yet.

Also, is there anyway to test the query parameters in the follow up patch (without writing a program, as I'm not a programmer), or are these sufficiently covered by the tests?
Comment 21 Tomás Cohen Arazi 2020-02-09 19:25:57 UTC
(In reply to David Nind from comment #20)
> > Hi David, bug 24487 is now in master. Would you please try again?
> 
> The bug is still restricted, so not able to yet.
> 
> Also, is there anyway to test the query parameters in the follow up patch
> (without writing a program, as I'm not a programmer), or are these
> sufficiently covered by the tests?

David, you don't seem to need to apply that bug:

(base) ✔ ~/git/koha [qa_24502|⚑ 13]
16:24 $ git bz apply 24502

Bug 24502 Depends on bug 24356
Follow? [(y)es, (n)o] y
Bug 24356 - objects.search prefetch

98602 - Bug 24356: prefetch_whitelist tests
98603 - Bug 24356: Make objects.search prefetch embedded relations

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 24356: prefetch_whitelist tests
Applying: Bug 24356: Make objects.search prefetch embedded relations
Bug 24502 - Add a query language and param (q=) to the API

98605 - Bug 24502: object.search also filter by prefetched columns
98611 - Bug 24502: (follow-up) Add all 3 query parameters in api definition

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 24502: object.search also filter by prefetched columns
Applying: Bug 24502: (follow-up) Add all 3 query parameters in api definition
(base) ✔ ~/git/koha [qa_24502 ↑·4|⚑ 13]
16:25 $
Comment 22 Katrin Fischer 2020-02-09 19:50:35 UTC
24356 is not in security, which is the bug that gives you trouble? I can give you access to security bugs if needed (just needs someone with access to add you to the CC list)
Comment 23 David Nind 2020-02-09 19:58:21 UTC
This is what I get when I try to apply:

root@b7a19a96d4cf:koha(bz24502)$ git bz apply 24502

Bug 24502 Depends on bug 24356
Follow? [(y)es, (n)o] y
Bug 24356 - objects.search prefetch

98602 - Bug 24356: prefetch_whitelist tests
98603 - Bug 24356: Make objects.search prefetch embedded relations

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 24356: prefetch_whitelist tests
Applying: Bug 24356: Make objects.search prefetch embedded relations
Failed to retrieve bug information: NotPermitted

When I go to bug 24487 in Bugzilla I get "You are not authorized to access bug #24487." (with a red background)
Comment 24 Katrin Fischer 2020-02-09 20:03:13 UTC
(In reply to David Nind from comment #23)
> This is what I get when I try to apply:
> 
> root@b7a19a96d4cf:koha(bz24502)$ git bz apply 24502
> 
> Bug 24502 Depends on bug 24356
> Follow? [(y)es, (n)o] y
> Bug 24356 - objects.search prefetch
> 
> 98602 - Bug 24356: prefetch_whitelist tests
> 98603 - Bug 24356: Make objects.search prefetch embedded relations
> 
> Apply? [(y)es, (n)o, (i)nteractive] y
> Applying: Bug 24356: prefetch_whitelist tests
> Applying: Bug 24356: Make objects.search prefetch embedded relations
> Failed to retrieve bug information: NotPermitted
> 
> When I go to bug 24487 in Bugzilla I get "You are not authorized to access
> bug #24487." (with a red background)

Hi David, I've added you to the CC list, that should give you access. As it's already pushed, maybe it's a problem in git bz not being able to determine the status. Applying the bugs manually without following the dependencies could also work in this case.
Comment 25 David Nind 2020-02-10 00:46:56 UTC
Thanks Katrin. Can now apply the patches!
Comment 26 David Nind 2020-02-10 00:49:57 UTC
After all that, 1 test failed for me!

root@5bbe8b290c9a:koha(bz24502)$ prove t/Koha/REST/Plugin/Query.t t/db_dependent/Koha/REST/Plugin/Objects.t
t/Koha/REST/Plugin/Query.t ................. 3/6 
    #   Failed test '200 OK'
    #   at t/Koha/REST/Plugin/Query.t line 314.
    #          got: '500'
    #     expected: '200'

    #   Failed test 'exact match for JSON Pointer "/-and"'
    #   at t/Koha/REST/Plugin/Query.t line 314.
    #     Structures begin differing at:
    #          $got = undef
    #     $expected = ARRAY(0x5564a4153a48)
    # Looks like you failed 2 tests of 3.

#   Failed test '/merge_q_params'
#   at t/Koha/REST/Plugin/Query.t line 357.
# Looks like you failed 1 test of 6.
t/Koha/REST/Plugin/Query.t ................. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/6 subtests 
t/db_dependent/Koha/REST/Plugin/Objects.t .. ok   

Test Summary Report
-------------------
t/Koha/REST/Plugin/Query.t               (Wstat: 256 Tests: 6 Failed: 1)
  Failed test:  4
  Non-zero exit status: 1
Files=2, Tests=14, 25 wallclock secs ( 0.06 usr  0.01 sys + 21.66 cusr  3.74 csys = 25.47 CPU)
Result: FAIL
Comment 27 Agustín Moyano 2020-02-19 14:09:21 UTC
Created attachment 99252 [details] [review]
Bug 24502: object.search also filter by prefetched columns

This patch adds the possibility to object.search helper, to also filter by prefetched columns.

In order to dynamically add filter parameters, they must be coded as json and placed in the body of the request, coded as string in 'q' query parameter or as string in 'x-koha-query' header.

The coded json, is in fact dbix syntax.

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t t/db_dependent/Koha/REST/Plugin/Objects.t
3. Sign off
Comment 28 Agustín Moyano 2020-02-19 14:09:26 UTC
Created attachment 99253 [details] [review]
Bug 24502: (follow-up) Add all 3 query parameters in api definition

This patch adds api definition for body, query or header parameters.

To use them include:

* "$ref": "../parameters.json#/q_body" for body parameter.
* "$ref": "../parameters.json#/q_param" for query parameter.
* "$ref": "../parameters.json#/q_header" for header parameter.
Comment 29 Agustín Moyano 2020-02-19 14:11:17 UTC
(In reply to David Nind from comment #26)
> After all that, 1 test failed for me!
> 
> root@5bbe8b290c9a:koha(bz24502)$ prove t/Koha/REST/Plugin/Query.t
> t/db_dependent/Koha/REST/Plugin/Objects.t
> t/Koha/REST/Plugin/Query.t ................. 3/6 
>     #   Failed test '200 OK'
>     #   at t/Koha/REST/Plugin/Query.t line 314.
>     #          got: '500'
>     #     expected: '200'
> 
>     #   Failed test 'exact match for JSON Pointer "/-and"'
>     #   at t/Koha/REST/Plugin/Query.t line 314.
>     #     Structures begin differing at:
>     #          $got = undef
>     #     $expected = ARRAY(0x5564a4153a48)
>     # Looks like you failed 2 tests of 3.
> 
> #   Failed test '/merge_q_params'
> #   at t/Koha/REST/Plugin/Query.t line 357.
> # Looks like you failed 1 test of 6.
> t/Koha/REST/Plugin/Query.t ................. Dubious, test returned 1 (wstat
> 256, 0x100)
> Failed 1/6 subtests 
> t/db_dependent/Koha/REST/Plugin/Objects.t .. ok   
> 
> Test Summary Report
> -------------------
> t/Koha/REST/Plugin/Query.t               (Wstat: 256 Tests: 6 Failed: 1)
>   Failed test:  4
>   Non-zero exit status: 1
> Files=2, Tests=14, 25 wallclock secs ( 0.06 usr  0.01 sys + 21.66 cusr  3.74
> csys = 25.47 CPU)
> Result: FAIL

Hi David, sorry for the delay. Now it should work ok.

Thanks for testing
Comment 30 Martin Renvoize 2020-02-21 07:21:56 UTC
Created attachment 99351 [details] [review]
Bug 24502: object.search also filter by prefetched columns

This patch adds the possibility to object.search helper, to also filter by prefetched columns.

In order to dynamically add filter parameters, they must be coded as json and placed in the body of the request, coded as string in 'q' query parameter or as string in 'x-koha-query' header.

The coded json, is in fact dbix syntax.

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t t/db_dependent/Koha/REST/Plugin/Objects.t
3. Sign off

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize 2020-02-21 07:22:00 UTC
Created attachment 99352 [details] [review]
Bug 24502: (follow-up) Add all 3 query parameters in api definition

This patch adds api definition for body, query or header parameters.

To use them include:

* "$ref": "../parameters.json#/q_body" for body parameter.
* "$ref": "../parameters.json#/q_param" for query parameter.
* "$ref": "../parameters.json#/q_header" for header parameter.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Martin Renvoize 2020-02-21 07:23:11 UTC
All works as expected and passes qa tests..

Well done Agustin, this is a great feature to have added..

Signing off
Comment 33 Kyle M Hall 2020-03-02 16:30:14 UTC
Created attachment 99946 [details] [review]
Bug 24502: object.search also filter by prefetched columns

This patch adds the possibility to object.search helper, to also filter by prefetched columns.

In order to dynamically add filter parameters, they must be coded as json and placed in the body of the request, coded as string in 'q' query parameter or as string in 'x-koha-query' header.

The coded json, is in fact dbix syntax.

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t t/db_dependent/Koha/REST/Plugin/Objects.t
3. Sign off

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Kyle M Hall 2020-03-02 16:30:30 UTC
Created attachment 99947 [details] [review]
Bug 24502: (follow-up) Add all 3 query parameters in api definition

This patch adds api definition for body, query or header parameters.

To use them include:

* "$ref": "../parameters.json#/q_body" for body parameter.
* "$ref": "../parameters.json#/q_param" for query parameter.
* "$ref": "../parameters.json#/q_header" for header parameter.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 35 Martin Renvoize 2020-03-03 09:23:07 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 36 Joy Nelson 2020-04-01 21:11:46 UTC
enhancement not backported to 19.11.x