Bug 23770 - Add Koha::Object(s)->to_api method
Summary: Add Koha::Object(s)->to_api method
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (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: 23843
  Show dependency treegraph
 
Reported: 2019-10-07 18:53 UTC by Tomás Cohen Arazi
Modified: 2020-06-04 20:35 UTC (History)
7 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:
19.11.00


Attachments
Bug 23770: Add Koha::Object(s)->to_api method (1.76 KB, patch)
2019-10-07 18:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23770: Add Koha::Object(s)->to_api method (2.36 KB, patch)
2019-10-08 15:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23770: Add to_api_mapping method to Koha::City (1.00 KB, patch)
2019-10-08 15:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23770: Unit tests (4.83 KB, patch)
2019-10-08 15:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23770: (follow-up) Add tests for Koha::Objects->TO_JSON (1.65 KB, patch)
2019-10-08 15:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23770: (follow-up) Use ->to_api in /cities (1.93 KB, patch)
2019-10-08 18:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23770: Add Koha::Object(s)->to_api method (2.41 KB, patch)
2019-10-08 19:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 23770: Add to_api_mapping method to Koha::City (1.06 KB, patch)
2019-10-08 19:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 23770: Unit tests (4.89 KB, patch)
2019-10-08 19:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 23770: (follow-up) Add tests for Koha::Objects->TO_JSON (1.70 KB, patch)
2019-10-08 19:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 23770: (follow-up) Use ->to_api in /cities (1.98 KB, patch)
2019-10-08 19:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 23770: Add Koha::Object(s)->to_api method (2.51 KB, patch)
2019-10-18 08:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23770: Add to_api_mapping method to Koha::City (1.15 KB, patch)
2019-10-18 09:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23770: Unit tests (4.99 KB, patch)
2019-10-18 09:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23770: (follow-up) Add tests for Koha::Objects->TO_JSON (1.80 KB, patch)
2019-10-18 09:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23770: (follow-up) Use ->to_api in /cities (2.08 KB, patch)
2019-10-18 09:00 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 2019-10-07 18:53:32 UTC
Given how the API codebase is growing and the plans we have to incorporate HAL representations, we need to allow the Koha::Object-based classes to be able to render themselves, so we can think of recursive structures embedding others in a generic way (i.e. not manually crafting things).
Comment 1 Tomás Cohen Arazi 2019-10-07 18:57:11 UTC
Created attachment 93831 [details] [review]
Bug 23770: Add Koha::Object(s)->to_api method

Generic method that changes the attribute names of an object in the
presence of the to_api_mapping method. It otherwise falls back to
returning the TO_JSON output.

This is WIP submitted early for scrutiny. Tests and example usage in an
API controller coming.
Comment 2 Josef Moravec 2019-10-08 05:44:19 UTC
+1

Thanks Tomás.

I do think that's the way we should go.
Comment 3 Tomás Cohen Arazi 2019-10-08 15:18:58 UTC
(In reply to Josef Moravec from comment #2)
> +1
> 
> Thanks Tomás.
> 
> I do think that's the way we should go.

Thanks for the feedback!
Comment 4 Tomás Cohen Arazi 2019-10-08 15:19:19 UTC
Created attachment 93881 [details] [review]
Bug 23770: Add Koha::Object(s)->to_api method

Generic method that changes the attribute names of an object in the
presence of the to_api_mapping method. It otherwise falls back to
returning the TO_JSON output.

This is WIP submitted early for scrutiny. Tests and example usage in an
API controller coming.
Comment 5 Tomás Cohen Arazi 2019-10-08 15:19:22 UTC
Created attachment 93882 [details] [review]
Bug 23770: Add to_api_mapping method to Koha::City

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2019-10-08 15:19:26 UTC
Created attachment 93883 [details] [review]
Bug 23770: Unit tests

This patch introduces tests for the new to_api method introduced in
Koha::Object(s).

It uses Koha::City as a sample for simplicity. And it also uses
Koha::Illrequests to test the case in which there's no to_api_mapping
method defined on the class, to highlight that a fallback to calling
TO_JSON is done. [1]

[1] This is done under the assumption Illrequests controller code
doesn't use any kind of mapping as the rest of the API controllers do,
so there's little chance it would be added in a future.

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

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Sponsored-by: ByWater Solutions
Comment 7 Tomás Cohen Arazi 2019-10-08 15:52:11 UTC
Created attachment 93888 [details] [review]
Bug 23770: (follow-up) Add tests for Koha::Objects->TO_JSON

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2019-10-08 18:45:45 UTC
Created attachment 93890 [details] [review]
Bug 23770: (follow-up) Use ->to_api in /cities

This patch aims to prove the behaviour does not differ. So you can run
the tests for the endpoints, and the behaviour remains.

The mappings and methods cannot be removed until $c->objects->search is
fixed to make use of them as well. That will happen once all used
Koha::Object-based classes implement the mappings.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/cities.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Josef Moravec 2019-10-08 19:44:37 UTC
Created attachment 93891 [details] [review]
Bug 23770: Add Koha::Object(s)->to_api method

Generic method that changes the attribute names of an object in the
presence of the to_api_mapping method. It otherwise falls back to
returning the TO_JSON output.

This is WIP submitted early for scrutiny. Tests and example usage in an
API controller coming.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 10 Josef Moravec 2019-10-08 19:44:42 UTC
Created attachment 93892 [details] [review]
Bug 23770: Add to_api_mapping method to Koha::City

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 11 Josef Moravec 2019-10-08 19:44:46 UTC
Created attachment 93893 [details] [review]
Bug 23770: Unit tests

This patch introduces tests for the new to_api method introduced in
Koha::Object(s).

It uses Koha::City as a sample for simplicity. And it also uses
Koha::Illrequests to test the case in which there's no to_api_mapping
method defined on the class, to highlight that a fallback to calling
TO_JSON is done. [1]

[1] This is done under the assumption Illrequests controller code
doesn't use any kind of mapping as the rest of the API controllers do,
so there's little chance it would be added in a future.

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

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 12 Josef Moravec 2019-10-08 19:44:51 UTC
Created attachment 93894 [details] [review]
Bug 23770: (follow-up) Add tests for Koha::Objects->TO_JSON

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 13 Josef Moravec 2019-10-08 19:44:55 UTC
Created attachment 93895 [details] [review]
Bug 23770: (follow-up) Use ->to_api in /cities

This patch aims to prove the behaviour does not differ. So you can run
the tests for the endpoints, and the behaviour remains.

The mappings and methods cannot be removed until $c->objects->search is
fixed to make use of them as well. That will happen once all used
Koha::Object-based classes implement the mappings.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/cities.t
=> SUCCESS: Tests pass!

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Marcel de Rooy 2019-10-11 05:26:10 UTC
QAing
Comment 15 Marcel de Rooy 2019-10-11 05:35:47 UTC
Depending on strict mode I have two results of the Object.t test:

        not ok 1 - Exception thrown correctly
        #   Failed test 'Exception thrown correctly'
        #   at t/db_dependent/Koha/Object.t line 411.
        # expecting: Koha::Exceptions::Object::BadValue
        # found: normal exit
        # Looks like you failed 1 test of 1.
    not ok 16 - Bad value tests

        #   Failed test 'Exception thrown correctly'
        #   at t/db_dependent/Koha/Object.t line 411.
        # expecting: Koha::Exceptions::Object::BadValue
        # found: DBIx::Class::Exception (DBIx::Class::Storage::DBI::_dbh_execute(): Incorrect datetime value: 'wrong_value' for column `koha_master`.`borrowers`.`lastseen` at row 1 at /usr/share/koha/Koha/Object.pm line 156
        # )
        # Looks like you failed 1 test of 1.
Comment 16 Marcel de Rooy 2019-10-11 05:37:08 UTC
Somehow does not meet:
elsif( $_->{msg} =~ /Incorrect (?<type>\w+) value: '(?<value>.*)' for column '(?<property>\w+)'/ ) {
Comment 17 Jonathan Druart 2019-10-15 11:02:42 UTC
Looks great to me!

What's next?

* Can we now remove ->_to_api (and so $to_api_mappinp) from Koha/REST/V1/Cities.pm

* What about ->_to_model (and so $to_model_mapping)?
Comment 18 Tomás Cohen Arazi 2019-10-15 13:23:49 UTC
(In reply to Jonathan Druart from comment #17)
> Looks great to me!
> 
> * Can we now remove ->_to_api (and so $to_api_mappinp) from
> Koha/REST/V1/Cities.pm

we still can't.

> * What about ->_to_model (and so $to_model_mapping)?

I was thinking of:

Koha::Object->new_from_api
Koha::Object->update_from_api

> What's next?

The next step is to file a bug in which we do the same for the other controllers. And then another one for implementing those two methods I propose. And then another one depending on these, for changing Koha::REST::Plugins::Objects so it uses the internal methods instead of expecting the to_api and to_model params.
Comment 19 Jonathan Druart 2019-10-15 13:34:23 UTC
(In reply to Tomás Cohen Arazi from comment #18)
> (In reply to Jonathan Druart from comment #17)
> > Looks great to me!
> > 
> > * Can we now remove ->_to_api (and so $to_api_mappinp) from
> > Koha/REST/V1/Cities.pm
> 
> we still can't.

Why? That seems to be the very next step to me :)
Comment 20 Tomás Cohen Arazi 2019-10-15 19:52:06 UTC
(In reply to Jonathan Druart from comment #19)
> (In reply to Tomás Cohen Arazi from comment #18)
> > (In reply to Jonathan Druart from comment #17)
> > > Looks great to me!
> > > 
> > > * Can we now remove ->_to_api (and so $to_api_mappinp) from
> > > Koha/REST/V1/Cities.pm
> > 
> > we still can't.
> 
> Why? That seems to be the very next step to me :)

Yes, yes. But we can't until we adapt Koha::REST::Plugin::Objects->search or provide an alternative implementation (maybe in Koha::Objects->search_from_api)
Comment 21 Tomás Cohen Arazi 2019-10-15 20:01:26 UTC
(In reply to Marcel de Rooy from comment #15)
> Depending on strict mode I have two results of the Object.t test:
> 
>         not ok 1 - Exception thrown correctly
>         #   Failed test 'Exception thrown correctly'
>         #   at t/db_dependent/Koha/Object.t line 411.
>         # expecting: Koha::Exceptions::Object::BadValue
>         # found: normal exit
>         # Looks like you failed 1 test of 1.
>     not ok 16 - Bad value tests
> 
>         #   Failed test 'Exception thrown correctly'
>         #   at t/db_dependent/Koha/Object.t line 411.
>         # expecting: Koha::Exceptions::Object::BadValue
>         # found: DBIx::Class::Exception
> (DBIx::Class::Storage::DBI::_dbh_execute(): Incorrect datetime value:
> 'wrong_value' for column `koha_master`.`borrowers`.`lastseen` at row 1 at
> /usr/share/koha/Koha/Object.pm line 156
>         # )
>         # Looks like you failed 1 test of 1.

Ok, but that's not something related to this bug, you just found that if we manage not to run the tests in strict mode on a dev env (i.e. using perl instead of prove) the exception is not thrown, fair enough.

I'm setting this one back to Signed-off.
Comment 22 Marcel de Rooy 2019-10-16 11:15:16 UTC
(In reply to Tomás Cohen Arazi from comment #21)
> Ok, but that's not something related to this bug, you just found that if we
> manage not to run the tests in strict mode on a dev env (i.e. using perl
> instead of prove) the exception is not thrown, fair enough.
> 
> I'm setting this one back to Signed-off.

Hmm. Did you misread my comment? I tried both modes. And I see two failures. Please confirm.
Comment 23 Jonathan Druart 2019-10-16 13:06:46 UTC
(In reply to Marcel de Rooy from comment #22)
> (In reply to Tomás Cohen Arazi from comment #21)
> > Ok, but that's not something related to this bug, you just found that if we
> > manage not to run the tests in strict mode on a dev env (i.e. using perl
> > instead of prove) the exception is not thrown, fair enough.
> > 
> > I'm setting this one back to Signed-off.
> 
> Hmm. Did you misread my comment? I tried both modes. And I see two failures.
> Please confirm.

I confirm I get the failure on master :)
Comment 24 Tomás Cohen Arazi 2019-10-16 13:08:38 UTC
(In reply to Marcel de Rooy from comment #22)
> (In reply to Tomás Cohen Arazi from comment #21)
> > Ok, but that's not something related to this bug, you just found that if we
> > manage not to run the tests in strict mode on a dev env (i.e. using perl
> > instead of prove) the exception is not thrown, fair enough.
> > 
> > I'm setting this one back to Signed-off.
> 
> Hmm. Did you misread my comment? I tried both modes. And I see two failures.
> Please confirm.

I only managed to see failures when I had sql_strict_modes disabled and run the tests using 'perl' instead of 'prove'.

I set it back to SO because the failure I found was not related to the patchset. Will re-check. Using koha-testing-docker here.
Comment 25 Jonathan Druart 2019-10-16 13:17:07 UTC
See bug 23825. I get the failure using prove.
Comment 26 Marcel de Rooy 2019-10-18 08:59:55 UTC
Created attachment 94396 [details] [review]
Bug 23770: Add Koha::Object(s)->to_api method

Generic method that changes the attribute names of an object in the
presence of the to_api_mapping method. It otherwise falls back to
returning the TO_JSON output.

This is WIP submitted early for scrutiny. Tests and example usage in an
API controller coming.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Marcel de Rooy 2019-10-18 09:00:00 UTC
Created attachment 94397 [details] [review]
Bug 23770: Add to_api_mapping method to Koha::City

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2019-10-18 09:00:04 UTC
Created attachment 94398 [details] [review]
Bug 23770: Unit tests

This patch introduces tests for the new to_api method introduced in
Koha::Object(s).

It uses Koha::City as a sample for simplicity. And it also uses
Koha::Illrequests to test the case in which there's no to_api_mapping
method defined on the class, to highlight that a fallback to calling
TO_JSON is done. [1]

[1] This is done under the assumption Illrequests controller code
doesn't use any kind of mapping as the rest of the API controllers do,
so there's little chance it would be added in a future.

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

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2019-10-18 09:00:09 UTC
Created attachment 94399 [details] [review]
Bug 23770: (follow-up) Add tests for Koha::Objects->TO_JSON

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2019-10-18 09:00:14 UTC
Created attachment 94400 [details] [review]
Bug 23770: (follow-up) Use ->to_api in /cities

This patch aims to prove the behaviour does not differ. So you can run
the tests for the endpoints, and the behaviour remains.

The mappings and methods cannot be removed until $c->objects->search is
fixed to make use of them as well. That will happen once all used
Koha::Object-based classes implement the mappings.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/cities.t
=> SUCCESS: Tests pass!

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2019-10-18 09:02:04 UTC
(In reply to Tomás Cohen Arazi from comment #24)
> I set it back to SO because the failure I found was not related to the
> patchset. Will re-check. Using koha-testing-docker here.

Although I can cause a failure with prove too, I will not block this patch set since it seems that the regex in sub store should be improved and we opened a report for that.
Comment 32 Martin Renvoize 2019-10-21 09:04:43 UTC
Nice work!

Pushed to master for 19.11.00