Created attachment 144329 [details] [review] Bug 32370: Unit tests
Created attachment 144330 [details] [review] Bug 32370: Provide a generic set of tools for JSON fields This patch introduces a new class for extending Koha::Object-based classes so they have reusable ways to deal with JSON encoded fields. As documented in the POD, classes can now ```perl use base qw(Koha::Object Koha::Object::JSONFields); ``` and will have the following methods available: * decode_json_field * set_encoded_json_field For convenience, the Koha::Auth::Identity::Provider class is tweaked to make use of this, and also to prove the behavior is correct (expected results, exceptions, encoding, etc). To test: 1. Run: $ kshell k$ prove t/db_dependent/Koha/Auth/Identity/Provider.t \ t/db_dependent/api/v1/idp.t => SUCCESS: Identity provider related tests pass! 2. Apply this patches 3. Repeat 1 => SUCCESS: Tests still pass! 4. Run the specific tests: k$ prove t/db_dependent/Koha/Object/JSONField.t => SUCCESS: Tests pass! 5. Sign off :-D
Created attachment 144331 [details] [review] Bug 32370: (follow-up) Make Koha::BackgroundJob use Koha::Object::JSONFields
(In reply to Tomás Cohen Arazi from comment #3) > Created attachment 144331 [details] [review] [review] > Bug 32370: (follow-up) Make Koha::BackgroundJob use Koha::Object::JSONFields ^^ to avoid the 'completeness' discussion we always have (y)
Created attachment 144332 [details] [review] Bug 32370: Unit tests
Created attachment 144333 [details] [review] Bug 32370: Provide a generic set of tools for JSON fields This patch introduces a new class for extending Koha::Object-based classes so they have reusable ways to deal with JSON encoded fields. As documented in the POD, classes can now ```perl use base qw(Koha::Object Koha::Object::JSONFields); ``` and will have the following methods available: * decode_json_field * set_encoded_json_field For convenience, the Koha::Auth::Identity::Provider class is tweaked to make use of this, and also to prove the behavior is correct (expected results, exceptions, encoding, etc). To test: 1. Run: $ kshell k$ prove t/db_dependent/Koha/Auth/Identity/Provider.t \ t/db_dependent/api/v1/idp.t => SUCCESS: Identity provider related tests pass! 2. Apply this patches 3. Repeat 1 => SUCCESS: Tests still pass! 4. Run the specific tests: k$ prove t/db_dependent/Koha/Object/JSONField.t => SUCCESS: Tests pass! 5. Sign off :-D
Created attachment 144334 [details] [review] Bug 32370: (follow-up) Make Koha::BackgroundJob use Koha::Object::JSONFields
I am certainly missing the context here, but this feels like unnecessary overhead. You are basically adding a Koha::Object base class for JSON decode and encode calls. That's all. The exception is there only to raise a Koha exception, but having a JSON exception is also fine. The MissingParameter exceptions are not needed either IMO, you will get a perl error if you are calling the method with missing mandatory parameter. If we really need these methods, and that I am missing the root idea and context of these patches, can you clarify why a base class instead of a regular class?
I'm still contemplating this... Considering https://metacpan.org/pod/DBIx::Class::InflateColumn::Serializer::JSON as an example to compare to. I think I still prefer the idea of a Koha::Object::InflateField::* hierarchy if we want something like this... to me, I think it's more elegant to outright replace the field accessor with automatic inflation rather than add additional accessors.. then fall back to get_column should one need the raw data.. but perhaps just because I'm so familiar with 'the DBIC way' I'm not seeing what we'd be losing in Koha land.
(In reply to Jonathan Druart from comment #8) > I am certainly missing the context here, but this feels like unnecessary > overhead. I thought the examples in IdP and BackgroundJob were enough context. But fair. > You are basically adding a Koha::Object base class for JSON decode and > encode calls. That's all. We are repeating code everywhere, and I provide a way to extend Koha::Object-derived classes with accessors for JSON fields. Yes. It sounds like a simple enough approach. If we did a regular class, we would need to instantiate it as needed and it feels counter productive and unnecessary. (In reply to Martin Renvoize from comment #9) > I'm still contemplating this... > > Considering > https://metacpan.org/pod/DBIx::Class::InflateColumn::Serializer::JSON as an > example to compare to. I agree with this approach and I found it interesting. YOu mentioned it after I already wrote this so I thought I would post this to start the discussion. Will think about it. The main drawback is that devs might be caught by surprise with inflation, but it shouldn't be a problem. It will jus tbe a general behavior change and we would need to measure performance/memory overhead, as my implementation will only introduce overhead if the new accessors are really called. I tried to provide real-life examples on how it would be used to make the point code-wise. (In reply to Jonathan Druart from comment #8) > The exception is there only to raise a Koha exception, but having a JSON > exception is also fine. The MissingParameter exceptions are not needed > either IMO, you will get a perl error if you are calling the method with > missing mandatory parameter. Yeah, I tried that. You get - "Can't call method undefined", I prefer a named exception but this is really taste. - As of JSON, I think I picked what we already had. But it is fair not to throw our own exception; again, a matter of taste.
malformed JSON string, neither tag, array, object, number, string or atom, at character offset 0 (before "HASH(0x55ae880d9628)") at /kohadevbox/koha/misc/background_jobs_worker.pl line 93.
(In reply to Jonathan Druart from comment #11) > malformed JSON string, neither tag, array, object, number, string or atom, > at character offset 0 (before "HASH(0x55ae880d9628)") at > /kohadevbox/koha/misc/background_jobs_worker.pl line 93. Hum, forget that, I guess I need to copy the new version of the misc script.
Created attachment 144347 [details] [review] Bug 32370: (follow-up) Make Koha::BackgroundJob use Koha::Object::JSONFields
Created attachment 144376 [details] [review] Bug 32370: Unit tests
Created attachment 144377 [details] [review] Bug 32370: Provide a generic set of tools for JSON fields This patch introduces a new class for extending Koha::Object-based classes so they have reusable ways to deal with JSON encoded fields. As documented in the POD, classes can now ```perl use base qw(Koha::Object Koha::Object::JSONFields); ``` and will have the following methods available: * decode_json_field * set_encoded_json_field For convenience, the Koha::Auth::Identity::Provider class is tweaked to make use of this, and also to prove the behavior is correct (expected results, exceptions, encoding, etc). To test: 1. Run: $ kshell k$ prove t/db_dependent/Koha/Auth/Identity/Provider.t \ t/db_dependent/api/v1/idp.t => SUCCESS: Identity provider related tests pass! 2. Apply this patches 3. Repeat 1 => SUCCESS: Tests still pass! 4. Run the specific tests: k$ prove t/db_dependent/Koha/Object/JSONField.t => SUCCESS: Tests pass! 5. Sign off :-D
Created attachment 144378 [details] [review] Bug 32370: (follow-up) Make Koha::BackgroundJob use Koha::Object::JSONFields
108 $self->set_encoded_json_field( { data => $job_args, field => 'data' } ); 120 $job_args->{job_id} = $self->id; 130 my $json_args = $self->data; # we pass the JSON string 137 my $encoded_args = Encode::encode_utf8( $json_args ); # FIXME We should better leave this to Net::Stomp? This is terribly wrong, we are loosing job_id...
I think attention must be on bug 32393, bug 32394 and bug 32395 first.
(In reply to Jonathan Druart from comment #12) > (In reply to Jonathan Druart from comment #11) > > malformed JSON string, neither tag, array, object, number, string or atom, > > at character offset 0 (before "HASH(0x55ae880d9628)") at > > /kohadevbox/koha/misc/background_jobs_worker.pl line 93. > > Hum, forget that, I guess I need to copy the new version of the misc script. I have faced some very weird behaviours testing this (and linked bugs). At some point I got background_jobs.pl processes that were not attached to the koha-worker daemon, and were using an old version of the code (like this message that was because of this bug, but was also in the log after a checkout master && restart_all). Just noting, but good to keep in mind.
(In reply to Jonathan Druart from comment #19) > (In reply to Jonathan Druart from comment #12) > > (In reply to Jonathan Druart from comment #11) > > > malformed JSON string, neither tag, array, object, number, string or atom, > > > at character offset 0 (before "HASH(0x55ae880d9628)") at > > > /kohadevbox/koha/misc/background_jobs_worker.pl line 93. > > > > Hum, forget that, I guess I need to copy the new version of the misc script. > > I have faced some very weird behaviours testing this (and linked bugs). > > At some point I got background_jobs.pl processes that were not attached to > the koha-worker daemon, and were using an old version of the code (like this > message that was because of this bug, but was also in the log after a > checkout master && restart_all). Just noting, but good to keep in mind. Not having the job_id might be a good source for weirdness :-D well spotted, Jonathan. I should've put the background jobs related change on a separate bug.
Created attachment 144445 [details] [review] Bug 32370: (follow-up) Make Koha::BackgroundJob use Koha::Object::JSONFields https://bugs.koha-community.org/show_bug.cgi?id=32371
Created attachment 145213 [details] [review] Bug 32370: Unit tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 145214 [details] [review] Bug 32370: Provide a generic set of tools for JSON fields This patch introduces a new class for extending Koha::Object-based classes so they have reusable ways to deal with JSON encoded fields. As documented in the POD, classes can now ```perl use base qw(Koha::Object Koha::Object::JSONFields); ``` and will have the following methods available: * decode_json_field * set_encoded_json_field For convenience, the Koha::Auth::Identity::Provider class is tweaked to make use of this, and also to prove the behavior is correct (expected results, exceptions, encoding, etc). To test: 1. Run: $ kshell k$ prove t/db_dependent/Koha/Auth/Identity/Provider.t \ t/db_dependent/api/v1/idp.t => SUCCESS: Identity provider related tests pass! 2. Apply this patches 3. Repeat 1 => SUCCESS: Tests still pass! 4. Run the specific tests: k$ prove t/db_dependent/Koha/Object/JSONField.t => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 145215 [details] [review] Bug 32370: (follow-up) Make Koha::BackgroundJob use Koha::Object::JSONFields https://bugs.koha-community.org/show_bug.cgi?id=32371 Signed-off-by: Nick Clemens <nick@bywatersolutions.com>