Bug 24528 - Add a syntax for specifying counts on x-koha-embed
Summary: Add a syntax for specifying counts on x-koha-embed
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 20212 24467 24554 24965
  Show dependency treegraph
 
Reported: 2020-01-28 15:46 UTC by Tomás Cohen Arazi
Modified: 2020-03-24 12:21 UTC (History)
9 users (show)

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


Attachments
Bug 24528: Unit tests for Koha::Object->to_api (2.10 KB, patch)
2020-01-28 19:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24528: Add *_count support to to_api (2.67 KB, patch)
2020-01-28 19:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24528: Add a syntax to x-koha-embed to specify counts (2.71 KB, patch)
2020-01-29 12:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24528: Unit tests for Koha::Object->to_api (2.15 KB, patch)
2020-01-31 06:36 UTC, David Nind
Details | Diff | Splinter Review
Bug 24528: Add *_count support to to_api (2.72 KB, patch)
2020-01-31 06:36 UTC, David Nind
Details | Diff | Splinter Review
Bug 24528: Add a syntax to x-koha-embed to specify counts (2.81 KB, patch)
2020-01-31 06:36 UTC, David Nind
Details | Diff | Splinter Review
Bug 24528: Unit tests for Koha::Object->to_api (2.24 KB, patch)
2020-01-31 07:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24528: Add *_count support to to_api (2.82 KB, patch)
2020-01-31 07:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24528: Add a syntax to x-koha-embed to specify counts (2.90 KB, patch)
2020-01-31 07:58 UTC, Marcel de Rooy
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-01-28 15:46:32 UTC
We need a way to specify that we want the count of the relation result and not the result itself, this way we don't need to implement *_count methods (see bug 24467)
Comment 1 Tomás Cohen Arazi 2020-01-28 19:30:39 UTC
Created attachment 98046 [details] [review]
Bug 24528: Unit tests for Koha::Object->to_api

This patch introduces unit tests for the new to_api bejaviour for
*_count methods.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: Tests fail because the feature is not implemented
Comment 2 Tomás Cohen Arazi 2020-01-28 19:30:44 UTC
Created attachment 98047 [details] [review]
Bug 24528: Add *_count support to to_api

This patch adds a way to tell to_api an attribute needs to be calculated
as the count on an existing method/relationship result. For example, if
we wanted to include the holds_count attribute for a Koha::Patron
object, we would call it:

    $ patron_json = $patron->to_api({ embed => { holds_count => { is_count => 1 } } });

This way to_api will internally call

    $json->{holds_count} = $self->holds->count;

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat (2)
=> SUCCESS: Tests pass!
5. Sign off :-D
Comment 3 Tomás Cohen Arazi 2020-01-28 19:32:49 UTC
Submitting the first bit once I got it for review. It is pretty simple, but I had a nightmare because I picked Koha::Biblio as an example... Will need to fix the overloaded to_api once I get this sorted.

Next step is parsing relation+count on the API code, and adding is_count => 1 to the structure.
Comment 4 Tomás Cohen Arazi 2020-01-29 12:33:34 UTC
Created attachment 98065 [details] [review]
Bug 24528: Add a syntax to x-koha-embed to specify counts

This patch adds a + syntax to specify on the x-koha-embed header that we
want the count of a relation in the response. For example:

GET /patrons/1
x-koha-embed: checkouts+count

Would return a JSON representation of a Koha::Patron, with a new
attribute added: checkouts_count, which will be the result of calling
$patron->checkouts->count.

This is all done automatically in to_api. This patch makes parsing the
x-koha-embed header build the right structure for passing to the to_api
method.

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

Sponsored-by: ByWater Solutions
Comment 5 Tomás Cohen Arazi 2020-01-29 12:33:59 UTC
Ready for testing!
Comment 6 David Nind 2020-01-31 06:36:52 UTC
Created attachment 98199 [details] [review]
Bug 24528: Unit tests for Koha::Object->to_api

This patch introduces unit tests for the new to_api bejaviour for
*_count methods.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: Tests fail because the feature is not implemented

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2020-01-31 06:36:55 UTC
Created attachment 98200 [details] [review]
Bug 24528: Add *_count support to to_api

This patch adds a way to tell to_api an attribute needs to be calculated
as the count on an existing method/relationship result. For example, if
we wanted to include the holds_count attribute for a Koha::Patron
object, we would call it:

    $ patron_json = $patron->to_api({ embed => { holds_count => { is_count => 1 } } });

This way to_api will internally call

    $json->{holds_count} = $self->holds->count;

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat (2)
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2020-01-31 06:36:59 UTC
Created attachment 98201 [details] [review]
Bug 24528: Add a syntax to x-koha-embed to specify counts

This patch adds a + syntax to specify on the x-koha-embed header that we
want the count of a relation in the response. For example:

GET /patrons/1
x-koha-embed: checkouts+count

Would return a JSON representation of a Koha::Patron, with a new
attribute added: checkouts_count, which will be the result of calling
$patron->checkouts->count.

This is all done automatically in to_api. This patch makes parsing the
x-koha-embed header build the right structure for passing to the to_api
method.

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

Sponsored-by: ByWater Solutions

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 Marcel de Rooy 2020-01-31 07:01:30 UTC
QAing
Comment 10 Marcel de Rooy 2020-01-31 07:58:42 UTC
Created attachment 98212 [details] [review]
Bug 24528: Unit tests for Koha::Object->to_api

This patch introduces unit tests for the new to_api bejaviour for
*_count methods.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: Tests fail because the feature is not implemented

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2020-01-31 07:58:47 UTC
Created attachment 98213 [details] [review]
Bug 24528: Add *_count support to to_api

This patch adds a way to tell to_api an attribute needs to be calculated
as the count on an existing method/relationship result. For example, if
we wanted to include the holds_count attribute for a Koha::Patron
object, we would call it:

    $ patron_json = $patron->to_api({ embed => { holds_count => { is_count => 1 } } });

This way to_api will internally call

    $json->{holds_count} = $self->holds->count;

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat (2)
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2020-01-31 07:58:51 UTC
Created attachment 98214 [details] [review]
Bug 24528: Add a syntax to x-koha-embed to specify counts

This patch adds a + syntax to specify on the x-koha-embed header that we
want the count of a relation in the response. For example:

GET /patrons/1
x-koha-embed: checkouts+count

Would return a JSON representation of a Koha::Patron, with a new
attribute added: checkouts_count, which will be the result of calling
$patron->checkouts->count.

This is all done automatically in to_api. This patch makes parsing the
x-koha-embed header build the right structure for passing to the to_api
method.

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

Sponsored-by: ByWater Solutions

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Jonathan Druart 2020-01-31 11:18:35 UTC
+            if ( $embed =~ m/^(?<relation>.*)_count$/

and

+        if ( $curr =~ m/^(?<relation>.*)\+count/ ) {
+            my $key = $+{relation} . "_count";

Why did not you keep "relation+count"?
Comment 14 Tomás Cohen Arazi 2020-01-31 11:33:28 UTC
(In reply to Jonathan Druart from comment #13)
> +            if ( $embed =~ m/^(?<relation>.*)_count$/
> 
> and
> 
> +        if ( $curr =~ m/^(?<relation>.*)\+count/ ) {
> +            my $key = $+{relation} . "_count";
> 
> Why did not you keep "relation+count"?

Inertia. Coding at 2 am. I agree with keeping the +.
Comment 15 Martin Renvoize 2020-01-31 13:51:32 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 16 Tomás Cohen Arazi 2020-01-31 13:58:52 UTC
(In reply to Tomás Cohen Arazi from comment #14)
> (In reply to Jonathan Druart from comment #13)
> > +            if ( $embed =~ m/^(?<relation>.*)_count$/
> > 
> > and
> > 
> > +        if ( $curr =~ m/^(?<relation>.*)\+count/ ) {
> > +            my $key = $+{relation} . "_count";
> > 
> > Why did not you keep "relation+count"?
> 
> Inertia. Coding at 2 am. I agree with keeping the +.

We decided to keep with the snake_case syntax as using + would (1) not follow the API naming conventions, and (2) it would become problematic to access the hash elements i.e.  order.biblio+count means concatenating order.biblio and count.
Comment 17 Joy Nelson 2020-02-10 23:48:16 UTC
Enhancement not backported to 19.11.x