Bug 27680

Summary: API DataTables Wrapper fails for ordering on multi-data-field columns
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: agustinmoyano, andrewfh, fridolin.somers, jonathan.druart, kyle, tomascohen, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.04,20.05.10
Bug Depends on:    
Bug Blocks: 27806, 26273, 27807    
Attachments: :x
Bug 27680: [DO NOT PUSH] Example patch on cities
Bug 27680: Take first order column for ordering
Bug 27680: Add multi-field support
Bug 27680: Take first order column for ordering
Bug 27680: Add multi-field support
Bug 27680: Make collectionFormat open for order_by
Bug 27680: Add tests for various ways of passing multi-params
Bug 27680: Take first order column for ordering
Bug 27680: Add support for sorting fields with multiple data points
Bug 27680: Add tests for various ways of passing multi-params
Bug 27680: Allow traditional multi-params to work
Bug 27680: [DO NOT PUSH] Example patch on cities
Bug 27680: Take first order column for ordering
Bug 27680: Add support for sorting fields with multiple data points
Bug 27680: Add tests for various ways of passing multi-params
Bug 27680: Allow traditional multi-params to work
Bug 27680: (follow-up) Add Koha::Exception for bad queries
Bug 27680: (follow-up) Attempt to catch the exception
Bug 27680: (follow-up) Attempt to catch the exception
Bug 27680: [DO NOT PUSH] Example patch on cities
Bug 27680: Take first order column for ordering
Bug 27680: Add support for sorting fields with multiple data points
Bug 27680: Add tests for various ways of passing multi-params
Bug 27680: Allow traditional multi-params to work
Bug 27680: Add support for param[] syntax
Bug 27680: (QA follow-up) Minor perlcritic issue
Bug 27680: Take first order column for ordering
Bug 27680: Add support for sorting fields with multiple data points
Bug 27680: Add tests for various ways of passing multi-params
Bug 27680: Allow traditional multi-params to work
Bug 27680: Add support for param[] syntax
Bug 27680: (QA follow-up) Update test for dbic_merge_sorting

Description Martin Renvoize 2021-02-11 12:27:21 UTC
We can make a single column contain data from multiple 'data' points in the ajax response using a mixture of a render function and concatenating the data points with ':' in the data field.  This allows for a nice display and searching to work.. but the ordering call fails :(

The final patch on bug 26273 highlights this:

  {
     "data": "manager.firstname:manager.surname",
     "searchable": true,
     "orderable": true,
     "render": function(data, type, row, meta) {
         var fullname = row.manager.firstname + " " + row.manager.surname;
         return fullname;
     }
  },

Assuming manager.firstname = 'Bob' and manager.surname = 'Geldoff'.. we will display "Bob Geldoff". Searching for 'Bob' will add a suitable search header searching for 'Bob' in both 'manager.firstname' and 'manager.surname' fields.. but hitting order by on this column will result in a 500 error as we add '_order_by: +manager.firstname:manager.surname' to the query parameters.
Comment 1 Jonathan Druart 2021-02-12 09:53:47 UTC
Created attachment 116794 [details] [review]
:x

Bug 27680: Take first order column for ordering
Comment 2 Jonathan Druart 2021-02-12 09:54:33 UTC
Created attachment 116795 [details] [review]
Bug 27680: [DO NOT PUSH] Example patch on cities
Comment 3 Jonathan Druart 2021-02-12 09:54:55 UTC
Created attachment 116796 [details] [review]
Bug 27680: Take first order column for ordering
Comment 4 Jonathan Druart 2021-02-12 09:56:07 UTC
(In reply to Jonathan Druart from comment #3)
> Created attachment 116796 [details] [review] [review]
> Bug 27680: Take first order column for ordering

We can go with this patch for now.

Are the REST API routes support ordering on several attributes?
Comment 5 Martin Renvoize 2021-02-12 10:10:40 UTC
(In reply to Jonathan Druart from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > Created attachment 116796 [details] [review] [review] [review]
> > Bug 27680: Take first order column for ordering
> 
> We can go with this patch for now.
> 
> Are the REST API routes support ordering on several attributes?

It appears to: https://git.koha-community.org/Koha-community/Koha/src/branch/master/Koha/REST/Plugin/Query.pm#L87 :)
Comment 6 Jonathan Druart 2021-02-12 10:15:02 UTC
(In reply to Jonathan Druart from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > Created attachment 116796 [details] [review] [review] [review]
> > Bug 27680: Take first order column for ordering
> 
> We can go with this patch for now.
> 
> Are the REST API routes support ordering on several attributes?

Looks like we do

 95                 if ( reftype($order_by) and reftype($order_by) eq 'ARRAY' ) {

So either we go first with this quick fix, or we need to implement it for the DT API wrapper.

Currently it does not work if you select more than 1 column.
Comment 7 Martin Renvoize 2021-02-12 13:15:09 UTC
Created attachment 116829 [details] [review]
Bug 27680: Add multi-field support
Comment 8 Martin Renvoize 2021-02-12 13:29:38 UTC
Still working here.. I found a series of little bugs and am writing some extra unit tests
Comment 9 Martin Renvoize 2021-02-12 14:07:35 UTC
Created attachment 116831 [details] [review]
Bug 27680: Take first order column for ordering
Comment 10 Martin Renvoize 2021-02-12 14:07:38 UTC
Created attachment 116832 [details] [review]
Bug 27680: Add multi-field support
Comment 11 Martin Renvoize 2021-02-12 14:07:42 UTC
Created attachment 116833 [details] [review]
Bug 27680: Make collectionFormat open for order_by
Comment 12 Martin Renvoize 2021-02-12 14:07:45 UTC
Created attachment 116834 [details] [review]
Bug 27680: Add tests for various ways of passing multi-params
Comment 13 Martin Renvoize 2021-02-12 14:14:37 UTC
As the final tests expose.. this raises a question.

What form of 'multi-param' sending do we actually want to support?  Prior to my patches we were "in theory" limiting to pipe delimited strings.. but the tests weren't passing any pipe delimited parameters and were passing for 'traditional multiple params' (i.e ?param1=+thingy&param2=+thangy).

I've added tests to cover the following.. but clearly many of them currently fail.

1. Multi param traditional `?param=+this&param=+that` - Passing
2. Multi param PHP `?param[]=+this&param[]=+that` - Failing
3. CSV param `?param=+this,+that` - Failing

I like option 1, but feel we should fail in a more formal way for any/all other options.. right now we just explode horribly.
Comment 14 Jonathan Druart 2021-02-12 14:25:11 UTC
It does not work for me.

https://snipboard.io/tzlfnE.jpg
generates:
  /api/v1/cities?&_page=1&_per_page=20&q=%5B%5D&_match=contains&_order_by=-me.city_id
Comment 15 Jonathan Druart 2021-02-12 14:39:57 UTC
Actually I forgot to restart_all. Now the table does not display at all

OpenAPI <<< GET api/v1/cities [{"message":"Expected array - got string.","path":"\/_order_by"}]
Comment 16 Martin Renvoize 2021-02-12 19:29:31 UTC
Created attachment 116843 [details] [review]
Bug 27680: Take first order column for ordering
Comment 17 Martin Renvoize 2021-02-12 19:29:34 UTC
Created attachment 116844 [details] [review]
Bug 27680: Add support for sorting fields with multiple data points

This patch adds proper handling for sorting a single column that is
constructed of multiple data entities.. i.e `"data": "string1:string2"`

It does NOT add support for filtering on multiple columns yet.
Comment 18 Martin Renvoize 2021-02-12 19:29:37 UTC
Created attachment 116845 [details] [review]
Bug 27680: Add tests for various ways of passing multi-params

This patch adds unit test to increase the coverage of parameter that
accept multiple values.  There are a number of different ways end users
can send such parameters and we should test to ensure we are recieving
the correct option.

Options
`?param1=this&param1=that` - traditional multiple pass params
`?param1[]=this&param1[]=that` - php multiple pass params
`?param1=this,that` - comma delimited list param
`?param1=this|that` - pipe delimited list param
Comment 19 Martin Renvoize 2021-02-12 19:29:41 UTC
Created attachment 116846 [details] [review]
Bug 27680: Allow traditional multi-params to work

This patch allows the preceeding test to pass. Strictly, we want a comma
delimited string for our _order_by parameter, but we cannot easily block
a traditional multi-passed parameter.  As such the 'nice' thing to do is
handle it when such a thing is passed as it will pass through validation
regardless.
Comment 20 Martin Renvoize 2021-02-12 19:38:16 UTC
This is the most pragmatic solution I could come up with.

* We set the datatable wrapper to send _order_by as a comma delimited string of fields.
* We update the collectionFormat spec to 'csv' from 'pipes'.. pipes was a bad value to start with as it's 'pipe' and 'csv' is more common.
* We add handling to allow for traditional multiple passed params by name (as we can't rule them out via validation for some reason so it's nicer to handle them than it is to throw a nasty serverside error.
* We add notes to some tests that are expected to result in a nasty 500 error. We cannot prevent arbitrary parameters from being sent to the API (which includes the _order_by[] PHP style multiple pass option, and non-csv delimited string options).

Looking for feedback as to whether this is enough.. I'm thinking perhaps we need to throw a better Koha level exception when catching bad column names in our query string perhaps?
Comment 21 Martin Renvoize 2021-02-25 15:24:51 UTC
Created attachment 117335 [details] [review]
Bug 27680: [DO NOT PUSH] Example patch on cities
Comment 22 Martin Renvoize 2021-02-25 15:24:54 UTC
Created attachment 117336 [details] [review]
Bug 27680: Take first order column for ordering
Comment 23 Martin Renvoize 2021-02-25 15:24:57 UTC
Created attachment 117337 [details] [review]
Bug 27680: Add support for sorting fields with multiple data points

This patch adds proper handling for sorting a single column that is
constructed of multiple data entities.. i.e `"data": "string1:string2"`

It does NOT add support for filtering on multiple columns yet.
Comment 24 Martin Renvoize 2021-02-25 15:25:00 UTC
Created attachment 117338 [details] [review]
Bug 27680: Add tests for various ways of passing multi-params

This patch adds unit test to increase the coverage of parameter that
accept multiple values.  There are a number of different ways end users
can send such parameters and we should test to ensure we are recieving
the correct option.

Options
`?param1=this&param1=that` - traditional multiple pass params
`?param1[]=this&param1[]=that` - php multiple pass params
`?param1=this,that` - comma delimited list param
`?param1=this|that` - pipe delimited list param
Comment 25 Martin Renvoize 2021-02-25 15:25:04 UTC
Created attachment 117339 [details] [review]
Bug 27680: Allow traditional multi-params to work

This patch allows the preceeding test to pass. Strictly, we want a comma
delimited string for our _order_by parameter, but we cannot easily block
a traditional multi-passed parameter.  As such the 'nice' thing to do is
handle it when such a thing is passed as it will pass through validation
regardless.
Comment 26 Martin Renvoize 2021-02-25 15:25:07 UTC
Created attachment 117340 [details] [review]
Bug 27680: (follow-up) Add Koha::Exception for bad queries

This patch adds a PropertyNotFound exception tothe 'as_list' method to
catch DBIx::Class excpetions caused by passing bad column names into a
query.
Comment 27 Martin Renvoize 2021-02-25 15:25:10 UTC
Created attachment 117341 [details] [review]
Bug 27680: (follow-up) Attempt to catch the exception

Help: I'm struggling to catch the point at which the exception should be
thrown to convert it to a 400 error response in the API. '->search' does
not actually trigger the DB hit until we reference some data from it..
so we can't use that.. so I tried '->count', but of course.. that drops
'order_by' which is the failure in this case and so we pass through.
Comment 28 Martin Renvoize 2021-02-26 12:30:44 UTC
Created attachment 117375 [details] [review]
Bug 27680: (follow-up) Attempt to catch the exception

Help: I'm struggling to catch the point at which the exception should be
thrown to convert it to a 400 error response in the API. '->search' does
not actually trigger the DB hit until we reference some data from it..
so we can't use that.. so I tried '->count', but of course.. that drops
'order_by' which is the failure in this case and so we pass through.
Comment 29 Martin Renvoize 2021-02-26 13:30:26 UTC
I've moved the followup bugs to try and render exceptions more cleanly to another bug now.. I'd really like to get this bug unblocked so the dependant cash management bug can start moving again.
Comment 30 Tomás Cohen Arazi 2021-02-26 18:09:13 UTC
Created attachment 117388 [details] [review]
Bug 27680: [DO NOT PUSH] Example patch on cities

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 31 Tomás Cohen Arazi 2021-02-26 18:09:25 UTC
Created attachment 117389 [details] [review]
Bug 27680: Take first order column for ordering

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 32 Tomás Cohen Arazi 2021-02-26 18:09:32 UTC
Created attachment 117390 [details] [review]
Bug 27680: Add support for sorting fields with multiple data points

This patch adds proper handling for sorting a single column that is
constructed of multiple data entities.. i.e `"data": "string1:string2"`

It does NOT add support for filtering on multiple columns yet.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 33 Tomás Cohen Arazi 2021-02-26 18:09:41 UTC
Created attachment 117391 [details] [review]
Bug 27680: Add tests for various ways of passing multi-params

This patch adds unit test to increase the coverage of parameter that
accept multiple values.  There are a number of different ways end users
can send such parameters and we should test to ensure we are recieving
the correct option.

Options
`?param1=this&param1=that` - traditional multiple pass params
`?param1[]=this&param1[]=that` - php multiple pass params
`?param1=this,that` - comma delimited list param
`?param1=this|that` - pipe delimited list param

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 34 Tomás Cohen Arazi 2021-02-26 18:09:50 UTC
Created attachment 117392 [details] [review]
Bug 27680: Allow traditional multi-params to work

This patch allows the preceeding test to pass. Strictly, we want a comma
delimited string for our _order_by parameter, but we cannot easily block
a traditional multi-passed parameter.  As such the 'nice' thing to do is
handle it when such a thing is passed as it will pass through validation
regardless.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 35 Tomás Cohen Arazi 2021-02-26 18:10:00 UTC
Created attachment 117393 [details] [review]
Bug 27680: Add support for param[] syntax

While not that common nowadays, it is the syntax PHP uses, and
DataTables also generates such thing in 'traditional' mode. We should
support it as well.

This patch adds support for that. It does so by adding _order_by[] to
the reserved param names, and proper handling on the dbic_merge_sorting
helper.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
3- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 36 Tomás Cohen Arazi 2021-02-26 18:10:07 UTC
Created attachment 117394 [details] [review]
Bug 27680: (QA follow-up) Minor perlcritic issue

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 37 Jonathan Druart 2021-03-01 12:19:25 UTC
Created attachment 117438 [details] [review]
Bug 27680: Take first order column for ordering

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 38 Jonathan Druart 2021-03-01 12:19:29 UTC
Created attachment 117439 [details] [review]
Bug 27680: Add support for sorting fields with multiple data points

This patch adds proper handling for sorting a single column that is
constructed of multiple data entities.. i.e `"data": "string1:string2"`

It does NOT add support for filtering on multiple columns yet.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 39 Jonathan Druart 2021-03-01 12:19:33 UTC
Created attachment 117440 [details] [review]
Bug 27680: Add tests for various ways of passing multi-params

This patch adds unit test to increase the coverage of parameter that
accept multiple values.  There are a number of different ways end users
can send such parameters and we should test to ensure we are recieving
the correct option.

Options
`?param1=this&param1=that` - traditional multiple pass params
`?param1[]=this&param1[]=that` - php multiple pass params
`?param1=this,that` - comma delimited list param
`?param1=this|that` - pipe delimited list param

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 40 Jonathan Druart 2021-03-01 12:19:37 UTC
Created attachment 117441 [details] [review]
Bug 27680: Allow traditional multi-params to work

This patch allows the preceeding test to pass. Strictly, we want a comma
delimited string for our _order_by parameter, but we cannot easily block
a traditional multi-passed parameter.  As such the 'nice' thing to do is
handle it when such a thing is passed as it will pass through validation
regardless.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 41 Jonathan Druart 2021-03-01 12:19:40 UTC
Created attachment 117442 [details] [review]
Bug 27680: Add support for param[] syntax

While not that common nowadays, it is the syntax PHP uses, and
DataTables also generates such thing in 'traditional' mode. We should
support it as well.

This patch adds support for that. It does so by adding _order_by[] to
the reserved param names, and proper handling on the dbic_merge_sorting
helper.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
3- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Bug 27680: (QA follow-up) Minor perlcritic issue

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 42 Jonathan Druart 2021-03-01 14:24:26 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 43 Jonathan Druart 2021-03-01 16:02:08 UTC
We have a test that is failing:

    #   Failed test 'exact match for JSON Pointer "/order_by"'
    #   at t/Koha/REST/Plugin/Query.t line 338.
    #     Structures begin differing at:
    #          $got = ARRAY(0x55702f09b360)
    #     $expected = HASH(0x55702f094400)
    # Looks like you failed 1 test of 20.

#   Failed test 'dbic_merge_sorting() tests'
#   at t/Koha/REST/Plugin/Query.t line 343.
Comment 44 Martin Renvoize 2021-03-02 08:58:29 UTC
Created attachment 117476 [details] [review]
Bug 27680: (QA follow-up) Update test for dbic_merge_sorting

The order_by handling will now always return an arrayref of ordering
hashrefs even when only one ordering hash is present. This is
interpreted by dbic as equivilent so I just update the test to pass
here.
Comment 45 Jonathan Druart 2021-03-02 09:40:30 UTC
follow-up pushed to master
Comment 46 Fridolin Somers 2021-03-04 09:42:42 UTC
Pushed to 20.11.x for 20.11.04
Comment 47 Andrew Fuerste-Henry 2021-03-05 21:34:38 UTC
Pushed to 20.05.x for 20.05.10
Comment 48 Victor Grousset/tuxayo 2021-03-22 16:10:36 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.