Make object.search helper filter by prefetched columns
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
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.
https://www.elastic.co/guide/en/elasticsearch/reference/current/search-search.html for inspiration.
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?
(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.
(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?
All sounds good to me. [U+1F600]
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
Created attachment 98186 [details] [review] Bug 24502: (follow-up) Add default q parameter in api definition
I get this message when trying to apply this patch (git bz apply 24502): Failed to retrieve bug information: NotPermitted Other bugs apply fine.
(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!
(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!
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
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
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.
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
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.
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.
(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
> 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?
(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 $
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)
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)
(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.
Thanks Katrin. Can now apply the patches!
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
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
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.
(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
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>
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>
All works as expected and passes qa tests.. Well done Agustin, this is a great feature to have added.. Signing off
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>
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>
Nice work everyone! Pushed to master for 20.05
enhancement not backported to 19.11.x