Bug 36483 - Calling $object->to_api directly should be avoided
Summary: Calling $object->to_api directly should be avoided
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 33080
Blocks: 36495
  Show dependency treegraph
 
Reported: 2024-04-01 21:15 UTC by Tomás Cohen Arazi
Modified: 2024-04-12 10:13 UTC (History)
4 users (show)

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


Attachments
Bug 36483: Make API controllers use $c->objects->to_api (32.11 KB, patch)
2024-04-02 12:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36483: Make API controllers use $c->objects->to_api (32.66 KB, patch)
2024-04-02 12:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36483: Patrons.pm use case (1.96 KB, patch)
2024-04-02 13:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36483: Make API controllers use $c->objects->to_api (32.64 KB, patch)
2024-04-02 18:24 UTC, David Nind
Details | Diff | Splinter Review
Bug 36483: Patrons.pm use case (2.01 KB, patch)
2024-04-02 18:24 UTC, David Nind
Details | Diff | Splinter Review
Bug 36483: Make API controllers use $c->objects->to_api (32.74 KB, patch)
2024-04-12 08:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36483: Patrons.pm use case (2.10 KB, patch)
2024-04-12 08:39 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 2024-04-01 21:15:45 UTC
Highlighted on bug 36482, calling $object->to_api directly on our API controllers makes several parameters ineffective. Notably, x-koha-embed. We should call instead:

```perl
    $c->objects->to_api( $object )
```

A follow-up bug replacing all existing calls should've been filled but we somehow missed it.
Comment 1 Tomás Cohen Arazi 2024-04-01 21:16:19 UTC
I meant to say that when bug 33080 introduced this, we should've reviewed all controllers.
Comment 2 Tomás Cohen Arazi 2024-04-02 12:49:20 UTC
Created attachment 164264 [details] [review]
Bug 36483: Make API controllers use $c->objects->to_api

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2024-04-02 12:59:59 UTC
Created attachment 164266 [details] [review]
Bug 36483: Make API controllers use $c->objects->to_api

This patch makes all controller methods using direct $object->to_api, rely on $c->objects->to_api.

Direct ->to_api calling prevents things like embeds, +strings, etc to be dealt with correctly.
In some cases this change reduces the code as the authors extracted the embed structure from the stash and passed it explicitly.

I left out two Patrons.pm cases because I need to study it a bit more.

To test:
1. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi 2024-04-02 13:00:02 UTC
Created attachment 164267 [details] [review]
Bug 36483: Patrons.pm use case

This case brought my attention as there was a different 'user' parameter being passed.
It turns out we already thought about this, and `objects.to_api` is of course always passing it :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 David Nind 2024-04-02 18:24:10 UTC
Created attachment 164293 [details] [review]
Bug 36483: Make API controllers use $c->objects->to_api

This patch makes all controller methods using direct $object->to_api, rely on $c->objects->to_api.

Direct ->to_api calling prevents things like embeds, +strings, etc to be dealt with correctly.
In some cases this change reduces the code as the authors extracted the embed structure from the stash and passed it explicitly.

I left out two Patrons.pm cases because I need to study it a bit more.

To test:
1. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2024-04-02 18:24:12 UTC
Created attachment 164294 [details] [review]
Bug 36483: Patrons.pm use case

This case brought my attention as there was a different 'user' parameter being passed.
It turns out we already thought about this, and `objects.to_api` is of course always passing it :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Comment 7 Jonathan Druart 2024-04-03 07:48:47 UTC
> Created attachment 164294 [details] [review] [review]
> Bug 36483: Patrons.pm use case
> 
> This case brought my attention as there was a different 'user' parameter
> being passed.
> It turns out we already thought about this, and `objects.to_api` is of
> course always passing it :-D

Do we have a test for this? I don't find it.
Comment 8 Tomás Cohen Arazi 2024-04-03 13:19:26 UTC
(In reply to Jonathan Druart from comment #7)
> > Created attachment 164294 [details] [review] [review] [review]
> > Bug 36483: Patrons.pm use case
> > 
> > This case brought my attention as there was a different 'user' parameter
> > being passed.
> > It turns out we already thought about this, and `objects.to_api` is of
> > course always passing it :-D
> 
> Do we have a test for this? I don't find it.

I've been thinking about those two cases. It is trivially correct that the 'user' parameter is passed (universally) by $c->objects->to_api.

But I don't fully understand the semantics here. It would mean the API user could add/modify a patron, but then only get (say) the library_id in the response for adding/modifying.

Am I right, Martin?
Comment 9 Marcel de Rooy 2024-04-05 08:06:42 UTC
Looks good to me. Do we actually have examples where passing this user makes a difference in to_api ? Seems to do nothing in Koha::Patron??
Comment 10 Tomás Cohen Arazi 2024-04-11 12:50:21 UTC
(In reply to Marcel de Rooy from comment #9)
> Looks good to me. Do we actually have examples where passing this user makes
> a difference in to_api ? Seems to do nothing in Koha::Patron??

For understanding it, you need to refer to:

Koha/Object.pm:585~599
and once you got it, you read
Koha/Patron.pm:2361~2370
Comment 11 Martin Renvoize 2024-04-11 16:06:44 UTC
It's the redaction stuff. Though talking to Tomas at hackfest I think we both agreed it doesn't entirely make sense that one could edit, add a user with fields that aren't accessible to them.. but I reckon that's a different bug.. I'll record it when I'm not on my phone. Just packed up for the day
Comment 12 Marcel de Rooy 2024-04-12 08:33:17 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> (In reply to Marcel de Rooy from comment #9)
> > Looks good to me. Do we actually have examples where passing this user makes
> > a difference in to_api ? Seems to do nothing in Koha::Patron??
> 
> For understanding it, you need to refer to:
> 
> Koha/Object.pm:585~599
> and once you got it, you read
> Koha/Patron.pm:2361~2370

(In reply to Martin Renvoize from comment #11)
> It's the redaction stuff. Though talking to Tomas at hackfest I think we
> both agreed it doesn't entirely make sense that one could edit, add a user
> with fields that aren't accessible to them.. but I reckon that's a different
> bug.. I'll record it when I'm not on my phone. Just packed up for the day

Ah I vaguely remember QAing that stuff ;)
Comment 13 Marcel de Rooy 2024-04-12 08:39:42 UTC
Created attachment 164803 [details] [review]
Bug 36483: Make API controllers use $c->objects->to_api

This patch makes all controller methods using direct $object->to_api, rely on $c->objects->to_api.

Direct ->to_api calling prevents things like embeds, +strings, etc to be dealt with correctly.
In some cases this change reduces the code as the authors extracted the embed structure from the stash and passed it explicitly.

I left out two Patrons.pm cases because I need to study it a bit more.

To test:
1. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2024-04-12 08:39:44 UTC
Created attachment 164804 [details] [review]
Bug 36483: Patrons.pm use case

This case brought my attention as there was a different 'user' parameter being passed.
It turns out we already thought about this, and `objects.to_api` is of course always passing it :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Katrin Fischer 2024-04-12 10:13:01 UTC
Pushed for 24.05!

Well done everyone, thank you!