The Koha::Object class should provide a TO_JSON method that prepares data for JSON output. Right now, booleans are returned as integer values, and thus invalid for JSON output as boolean values.
*** Bug 17926 has been marked as a duplicate of this bug. ***
Created attachment 59431 [details] [review] Bug 17932: Add a TO_JSON method to Koha::Object(s) Bug 17927 fixes data types on the current REST endpoints. If you test those endpoints, you will notice trying to access them (for listing or retrieving single objects yields a data type error. Notably on booleans but also on integers. Integers fail due to https://rt.cpan.org/Ticket/Display.html?id=119904 but it needs some global solution until there's a DBD::mysql release backported to the supported distros. There's the option to use http://search.cpan.org/~frew/DBIx-Class-Helpers-2.033002/lib/DBIx/Class/Helper/Row/NumifyGet.pm to get the integer columns fixed as a workaround: __PACKAGE__->add_columns( borrowernumber => { data_type => 'integer', is_nullable => 0, is_numeric => 1, } ); I didn't find bug reports related to this (maybe because we don't use warnings everywhere) But I don't think is worth going such a heavy overhead. A similar situation takes place for Boolean values. They need to be prepared for JSON output. This could have been done using DBIx filters as pointed out by Martin: __PACKAGE__->filter_column( lost => { filter_to_storage => sub { $_[1] ? 1 : 0 }, filter_from_storage => sub { $_[1] ? Mojo::JSON->true : Mojo::JSON->false } } ); but this could have other consequences that are worth exploring on another bug (i.e. it would mean we need to take care of every place where this boolean data is used/set needs to handle this data types nicely. Such would be the case if we were a Mojo-only app, but we aren't. We use Koha::Obect(s) in the whole app. Period. This patch adds the need to specify on the schema files, columns that are actually boolean, because we have no way to detect them for now (i.e. they are all tinyint, but we use tinyint for non-boolean stuff too). So if this patch is accepted, we would need to specify boolean columns like this: __PACKAGE__->add_columns( '+lost' => { is_boolean => 1 } ); This patch adds a TO_JSON method for Koha::Object(s) to be used for serializing Koha::Object-derived objects into JSON strings. To test it (as Koha::Object(s) need to be instantiated) I provide tests on top of the Koha::Patron(s) classes in the followup patches. [1] Yes, we use TINYINT(1) for booleans, but from DBIC's perspective there's no way to read the (1) in runtime. Sponsored-by: ByWater Solutions
Created attachment 59432 [details] [review] Bug 17932: (followup) Fix /patrons endpoint Bug 17927 introduced data type fixes on the /patrons endpoint (integer and boolean types got fixed). This led to the /patrons endpoint not to work because the underlying code didn't provide the right data. With the introduction of TO_JSON on Koha::Object(s) we now have a way to output the proper data types. This patch does so by: - Adding is_boolean => 1 to the relevant columns on the Borrower.pm schema file. - Tweaking the controller class for the /patrons endpoint so it doesn't use the $object(s)->unblessed call but just let the Mojo::JSON library pick out TO_JSON implementation instead on rendering the output. - It adds a new test for booleans. To test: - Have 17927 applied - Run: $ prove t/db_dependent/api/v1/patrons.t => FAIL: Tests fail [1] - Apply this patches - Run: $ prove t/db_dependent/api/v1/patrons.t => SUCCESS: Tests pass! - Sign off! :-D [1] It is self explanatory to just try the API using any of the available tools (I use HttpRequester on Firefox)
Created attachment 59498 [details] [review] Bug 17932: Add a TO_JSON method to Koha::Object(s) Bug 17927 fixes data types on the current REST endpoints. If you test those endpoints, you will notice trying to access them (for listing or retrieving single objects yields a data type error. Notably on booleans but also on integers. Integers fail due to https://rt.cpan.org/Ticket/Display.html?id=119904 but it needs some global solution until there's a DBD::mysql release backported to the supported distros. There's the option to use http://search.cpan.org/~frew/DBIx-Class-Helpers-2.033002/lib/DBIx/Class/Helper/Row/NumifyGet.pm to get the integer columns fixed as a workaround: __PACKAGE__->add_columns( borrowernumber => { data_type => 'integer', is_nullable => 0, is_numeric => 1, } ); I didn't find bug reports related to this (maybe because we don't use warnings everywhere) But I don't think is worth going such a heavy overhead. A similar situation takes place for Boolean values. They need to be prepared for JSON output. This could have been done using DBIx filters as pointed out by Martin: __PACKAGE__->filter_column( lost => { filter_to_storage => sub { $_[1] ? 1 : 0 }, filter_from_storage => sub { $_[1] ? Mojo::JSON->true : Mojo::JSON->false } } ); but this could have other consequences that are worth exploring on another bug (i.e. it would mean we need to take care of every place where this boolean data is used/set needs to handle this data types nicely. Such would be the case if we were a Mojo-only app, but we aren't. We use Koha::Obect(s) in the whole app. Period. This patch adds the need to specify on the schema files, columns that are actually boolean, because we have no way to detect them for now (i.e. they are all tinyint, but we use tinyint for non-boolean stuff too). So if this patch is accepted, we would need to specify boolean columns like this: __PACKAGE__->add_columns( '+lost' => { is_boolean => 1 } ); This patch adds a TO_JSON method for Koha::Object(s) to be used for serializing Koha::Object-derived objects into JSON strings. To test it (as Koha::Object(s) need to be instantiated) I provide tests on top of the Koha::Patron(s) classes in the followup patches. [1] Yes, we use TINYINT(1) for booleans, but from DBIC's perspective there's no way to read the (1) in runtime. Sponsored-by: ByWater Solutions
Created attachment 59499 [details] [review] Bug 17932: (followup) Fix /patrons endpoint Bug 17927 introduced data type fixes on the /patrons endpoint (integer and boolean types got fixed). This led to the /patrons endpoint not to work because the underlying code didn't provide the right data. With the introduction of TO_JSON on Koha::Object(s) we now have a way to output the proper data types. This patch does so by: - Adding is_boolean => 1 to the relevant columns on the Borrower.pm schema file. - Tweaking the controller class for the /patrons endpoint so it doesn't use the $object(s)->unblessed call but just let the Mojo::JSON library pick out TO_JSON implementation instead on rendering the output. - It adds a new test for booleans. To test: - Have 17927 applied - Run: $ prove t/db_dependent/api/v1/patrons.t => FAIL: Tests fail [1] - Apply this patches - Run: $ prove t/db_dependent/api/v1/patrons.t => SUCCESS: Tests pass! - Sign off! :-D [1] It is self explanatory to just try the API using any of the available tools (I use HttpRequester on Firefox)
Created attachment 59500 [details] [review] Bug 17932: Unit tests This patch adds unit tests for the Koha::Object::TO_JSON function. It tests on top of Koha::Patron as Koha::Object needs to be instantiated. To test: - Apply the patch - Run: $ prove -v t/db_dependent/Koha/Object.t => SUCCESS: New tests for TO_JSON are run and return green. - Sign off :-D
Created attachment 59501 [details] [review] Bug 17932: (followup) Tidy tests This patch makes the tests create its own data instead of searching the DB for a branchcode and a categorycode. It does so on each subtest, because there shouldn't be side effects between subtests. I also wrapped each subtest inside a transaction, for the same reason. To test: - Apply this patch - Run: $ prove t/db_dependent/Koha/Object.t => SUCCESS: Tests return green with this patch - Sign off :-D
Looks good to me :)
(In reply to Martin Renvoize from comment #8) > Looks good to me :) Is it a signoff?
Created attachment 59696 [details] [review] Bug 17932: Add a TO_JSON method to Koha::Object(s) Bug 17927 fixes data types on the current REST endpoints. If you test those endpoints, you will notice trying to access them (for listing or retrieving single objects yields a data type error. Notably on booleans but also on integers. Integers fail due to https://rt.cpan.org/Ticket/Display.html?id=119904 but it needs some global solution until there's a DBD::mysql release backported to the supported distros. There's the option to use http://search.cpan.org/~frew/DBIx-Class-Helpers-2.033002/lib/DBIx/Class/Helper/Row/NumifyGet.pm to get the integer columns fixed as a workaround: __PACKAGE__->add_columns( borrowernumber => { data_type => 'integer', is_nullable => 0, is_numeric => 1, } ); I didn't find bug reports related to this (maybe because we don't use warnings everywhere) But I don't think is worth going such a heavy overhead. A similar situation takes place for Boolean values. They need to be prepared for JSON output. This could have been done using DBIx filters as pointed out by Martin: __PACKAGE__->filter_column( lost => { filter_to_storage => sub { $_[1] ? 1 : 0 }, filter_from_storage => sub { $_[1] ? Mojo::JSON->true : Mojo::JSON->false } } ); but this could have other consequences that are worth exploring on another bug (i.e. it would mean we need to take care of every place where this boolean data is used/set needs to handle this data types nicely. Such would be the case if we were a Mojo-only app, but we aren't. We use Koha::Obect(s) in the whole app. Period. This patch adds the need to specify on the schema files, columns that are actually boolean, because we have no way to detect them for now (i.e. they are all tinyint, but we use tinyint for non-boolean stuff too). So if this patch is accepted, we would need to specify boolean columns like this: __PACKAGE__->add_columns( '+lost' => { is_boolean => 1 } ); This patch adds a TO_JSON method for Koha::Object(s) to be used for serializing Koha::Object-derived objects into JSON strings. To test it (as Koha::Object(s) need to be instantiated) I provide tests on top of the Koha::Patron(s) classes in the followup patches. [1] Yes, we use TINYINT(1) for booleans, but from DBIC's perspective there's no way to read the (1) in runtime. Sponsored-by: ByWater Solutions Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 59697 [details] [review] Bug 17932: (followup) Fix /patrons endpoint Bug 17927 introduced data type fixes on the /patrons endpoint (integer and boolean types got fixed). This led to the /patrons endpoint not to work because the underlying code didn't provide the right data. With the introduction of TO_JSON on Koha::Object(s) we now have a way to output the proper data types. This patch does so by: - Adding is_boolean => 1 to the relevant columns on the Borrower.pm schema file. - Tweaking the controller class for the /patrons endpoint so it doesn't use the $object(s)->unblessed call but just let the Mojo::JSON library pick out TO_JSON implementation instead on rendering the output. - It adds a new test for booleans. To test: - Have 17927 applied - Run: $ prove t/db_dependent/api/v1/patrons.t => FAIL: Tests fail [1] - Apply this patches - Run: $ prove t/db_dependent/api/v1/patrons.t => SUCCESS: Tests pass! - Sign off! :-D [1] It is self explanatory to just try the API using any of the available tools (I use HttpRequester on Firefox) Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 59698 [details] [review] Bug 17932: Unit tests This patch adds unit tests for the Koha::Object::TO_JSON function. It tests on top of Koha::Patron as Koha::Object needs to be instantiated. To test: - Apply the patch - Run: $ prove -v t/db_dependent/Koha/Object.t => SUCCESS: New tests for TO_JSON are run and return green. - Sign off :-D Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 59699 [details] [review] Bug 17932: (followup) Tidy tests This patch makes the tests create its own data instead of searching the DB for a branchcode and a categorycode. It does so on each subtest, because there shouldn't be side effects between subtests. I also wrapped each subtest inside a transaction, for the same reason. To test: - Apply this patch - Run: $ prove t/db_dependent/Koha/Object.t => SUCCESS: Tests return green with this patch - Sign off :-D Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Thanks, Nick. You beat me in signing off :)
(In reply to Marcel de Rooy from comment #14) > Thanks, Nick. You beat me in signing off :) You can QA it ;-)
(In reply to Tomás Cohen Arazi from comment #15) > (In reply to Marcel de Rooy from comment #14) > > Thanks, Nick. You beat me in signing off :) > > You can QA it ;-) Good plan. Just a dumb question btw: why do you use uppercase TO_JSON? Is it that important :) Note that I see everywhere get_whatever etc.
(In reply to Marcel de Rooy from comment #16) > (In reply to Tomás Cohen Arazi from comment #15) > > (In reply to Marcel de Rooy from comment #14) > > > Thanks, Nick. You beat me in signing off :) > > > > You can QA it ;-) > > Good plan. Just a dumb question btw: why do you use uppercase TO_JSON? > Is it that important :) > Note that I see everywhere get_whatever etc. I personally dislike ->get_whatever, and prefer ->whatever in most cases (->as_json would be acceptable, because it is clear we want the object in a specific serialization format). Anyway, this time we are just sticking to what JSON libraries expect from blessed objects to accept them for rendering as JSON strings. See http://search.cpan.org/~makamaka/JSON-2.90/lib/JSON.pm#allow_blessed
(In reply to Tomás Cohen Arazi from comment #17) > Anyway, this time we are just sticking to what JSON libraries expect from > blessed objects to accept them for rendering as JSON strings. See > http://search.cpan.org/~makamaka/JSON-2.90/lib/JSON.pm#allow_blessed Good argument.
What about this output for /api/v1/patrons/xxx after the second patch: {"errors":[{"message":"([0] Expected string - got boolean. [1] Not null.)","path":"\/gonenoaddress"},{"message":"([0] Expected string - got number. [1] Not null.)","path":"\/flags"},{"message":"Expected string - got number.","path":"\/privacy"},{"message":"Expected string - got number.","path":"\/borrowernumber"},{"path":"\/guarantorid","message":"([0] Expected string - got number. [1] Not null.)"},{"message":"([0] Expected string - got number. [1] Not null.)","path":"\/sms_provider_id"},{"path":"\/privacy_guarantor_checkouts","message":"Expected string - got number."},{"path":"\/lost","message":"([0] Expected string - got boolean. [1] Not null.)"}]}
Created attachment 60072 [details] [review] Bug 17932: Add a TO_JSON method to Koha::Object(s) Bug 17927 fixes data types on the current REST endpoints. If you test those endpoints, you will notice trying to access them (for listing or retrieving single objects yields a data type error. Notably on booleans but also on integers. Integers fail due to https://rt.cpan.org/Ticket/Display.html?id=119904 but it needs some global solution until there's a DBD::mysql release backported to the supported distros. There's the option to use http://search.cpan.org/~frew/DBIx-Class-Helpers-2.033002/lib/DBIx/Class/Helper/Row/NumifyGet.pm to get the integer columns fixed as a workaround: __PACKAGE__->add_columns( borrowernumber => { data_type => 'integer', is_nullable => 0, is_numeric => 1, } ); I didn't find bug reports related to this (maybe because we don't use warnings everywhere) But I don't think is worth going such a heavy overhead. A similar situation takes place for Boolean values. They need to be prepared for JSON output. This could have been done using DBIx filters as pointed out by Martin: __PACKAGE__->filter_column( lost => { filter_to_storage => sub { $_[1] ? 1 : 0 }, filter_from_storage => sub { $_[1] ? Mojo::JSON->true : Mojo::JSON->false } } ); but this could have other consequences that are worth exploring on another bug (i.e. it would mean we need to take care of every place where this boolean data is used/set needs to handle this data types nicely. Such would be the case if we were a Mojo-only app, but we aren't. We use Koha::Obect(s) in the whole app. Period. This patch adds the need to specify on the schema files, columns that are actually boolean, because we have no way to detect them for now (i.e. they are all tinyint, but we use tinyint for non-boolean stuff too). So if this patch is accepted, we would need to specify boolean columns like this: __PACKAGE__->add_columns( '+lost' => { is_boolean => 1 } ); This patch adds a TO_JSON method for Koha::Object(s) to be used for serializing Koha::Object-derived objects into JSON strings. To test it (as Koha::Object(s) need to be instantiated) I provide tests on top of the Koha::Patron(s) classes in the followup patches. [1] Yes, we use TINYINT(1) for booleans, but from DBIC's perspective there's no way to read the (1) in runtime. Sponsored-by: ByWater Solutions Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60073 [details] [review] Bug 17932: (followup) Fix /patrons endpoint Bug 17927 introduced data type fixes on the /patrons endpoint (integer and boolean types got fixed). This led to the /patrons endpoint not to work because the underlying code didn't provide the right data. With the introduction of TO_JSON on Koha::Object(s) we now have a way to output the proper data types. This patch does so by: - Adding is_boolean => 1 to the relevant columns on the Borrower.pm schema file. - Tweaking the controller class for the /patrons endpoint so it doesn't use the $object(s)->unblessed call but just let the Mojo::JSON library pick out TO_JSON implementation instead on rendering the output. - It adds a new test for booleans. To test: - Have 17927 applied - Run: $ prove t/db_dependent/api/v1/patrons.t => FAIL: Tests fail [1] - Apply this patches - Run: $ prove t/db_dependent/api/v1/patrons.t => SUCCESS: Tests pass! - Sign off! :-D [1] It is self explanatory to just try the API using any of the available tools (I use HttpRequester on Firefox) Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60074 [details] [review] Bug 17932: Unit tests This patch adds unit tests for the Koha::Object::TO_JSON function. It tests on top of Koha::Patron as Koha::Object needs to be instantiated. To test: - Apply the patch - Run: $ prove -v t/db_dependent/Koha/Object.t => SUCCESS: New tests for TO_JSON are run and return green. - Sign off :-D Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60075 [details] [review] Bug 17932: (followup) Tidy tests This patch makes the tests create its own data instead of searching the DB for a branchcode and a categorycode. It does so on each subtest, because there shouldn't be side effects between subtests. I also wrapped each subtest inside a transaction, for the same reason. To test: - Apply this patch - Run: $ prove t/db_dependent/Koha/Object.t => SUCCESS: Tests return green with this patch - Sign off :-D Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Looks good to me. One small thing: the isvstring test looks somewhat useless to me. It also passes on other borrower text columns like phonepro or B_streetnumber. Could be handled on a follow-up. Passed QA
Pushed to master for 17.05, thanks Tomas!
This won't get ported back to 16.11.x as it is an enhancement.
Followup Bug 18290 fixes tests to cover a change in Mojolicious
With this patch now in master, we now have to update C4/Installer/PerlDependencies.pm to add Mojolicious as a required dependency.