Bug 32370 - Provide a generic set of tools for JSON fields
Summary: Provide a generic set of tools for JSON fields
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 30943 31351
Blocks:
  Show dependency treegraph
 
Reported: 2022-11-29 17:28 UTC by Tomás Cohen Arazi
Modified: 2023-02-24 10:50 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 32370: Unit tests (4.59 KB, patch)
2022-11-29 18:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32370: Provide a generic set of tools for JSON fields (6.41 KB, patch)
2022-11-29 18:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32370: (follow-up) Make Koha::BackgroundJob use Koha::Object::JSONFields (10.52 KB, patch)
2022-11-29 19:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32370: Unit tests (4.59 KB, patch)
2022-11-29 19:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32370: Provide a generic set of tools for JSON fields (6.72 KB, patch)
2022-11-29 19:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32370: (follow-up) Make Koha::BackgroundJob use Koha::Object::JSONFields (10.52 KB, patch)
2022-11-29 19:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32370: (follow-up) Make Koha::BackgroundJob use Koha::Object::JSONFields (10.52 KB, patch)
2022-11-30 13:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32370: Unit tests (4.59 KB, patch)
2022-12-01 12:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32370: Provide a generic set of tools for JSON fields (6.72 KB, patch)
2022-12-01 12:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32370: (follow-up) Make Koha::BackgroundJob use Koha::Object::JSONFields (11.78 KB, patch)
2022-12-01 12:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32370: (follow-up) Make Koha::BackgroundJob use Koha::Object::JSONFields (11.83 KB, patch)
2022-12-06 12:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32370: Unit tests (4.64 KB, patch)
2023-01-11 16:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32370: Provide a generic set of tools for JSON fields (6.77 KB, patch)
2023-01-11 16:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32370: (follow-up) Make Koha::BackgroundJob use Koha::Object::JSONFields (11.91 KB, patch)
2023-01-11 16:41 UTC, Nick Clemens (kidclamp)
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 2022-11-29 17:28:13 UTC

    
Comment 1 Tomás Cohen Arazi 2022-11-29 18:29:00 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2022-11-29 18:29:05 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2022-11-29 19:02:24 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2022-11-29 19:03:36 UTC
(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)
Comment 5 Tomás Cohen Arazi 2022-11-29 19:16:55 UTC
Created attachment 144332 [details] [review]
Bug 32370: Unit tests
Comment 6 Tomás Cohen Arazi 2022-11-29 19:17:02 UTC
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
Comment 7 Tomás Cohen Arazi 2022-11-29 19:17:09 UTC
Created attachment 144334 [details] [review]
Bug 32370: (follow-up) Make Koha::BackgroundJob use Koha::Object::JSONFields
Comment 8 Jonathan Druart 2022-11-30 07:21:09 UTC
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?
Comment 9 Martin Renvoize 2022-11-30 08:23:31 UTC
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.
Comment 10 Tomás Cohen Arazi 2022-11-30 11:47:47 UTC
(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.
Comment 11 Jonathan Druart 2022-11-30 12:59:27 UTC
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.
Comment 12 Jonathan Druart 2022-11-30 13:00:52 UTC
(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.
Comment 13 Tomás Cohen Arazi 2022-11-30 13:15:29 UTC
Created attachment 144347 [details] [review]
Bug 32370: (follow-up) Make Koha::BackgroundJob use Koha::Object::JSONFields
Comment 14 Tomás Cohen Arazi 2022-12-01 12:30:54 UTC
Created attachment 144376 [details] [review]
Bug 32370: Unit tests
Comment 15 Tomás Cohen Arazi 2022-12-01 12:30:59 UTC
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
Comment 16 Tomás Cohen Arazi 2022-12-01 12:31:05 UTC
Created attachment 144378 [details] [review]
Bug 32370: (follow-up) Make Koha::BackgroundJob use Koha::Object::JSONFields
Comment 17 Jonathan Druart 2022-12-02 14:26:52 UTC
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...
Comment 18 Jonathan Druart 2022-12-02 14:44:44 UTC
I think attention must be on bug 32393, bug 32394 and bug 32395 first.
Comment 19 Jonathan Druart 2022-12-02 14:48:27 UTC
(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.
Comment 20 Tomás Cohen Arazi 2022-12-02 15:32:17 UTC
(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.
Comment 21 Tomás Cohen Arazi 2022-12-06 12:41:27 UTC
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
Comment 22 Nick Clemens (kidclamp) 2023-01-11 16:41:10 UTC
Created attachment 145213 [details] [review]
Bug 32370: Unit tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 23 Nick Clemens (kidclamp) 2023-01-11 16:41:14 UTC
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>
Comment 24 Nick Clemens (kidclamp) 2023-01-11 16:41:19 UTC
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>