[Bug 36483] New: Calling $object->to_api directly should be avoided
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Bug ID: 36483 Summary: Calling $object->to_api directly should be avoided Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: REST API Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com CC: tomascohen@gmail.com 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |33080 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33080 [Bug 33080] Add helpers that return result_set for further processing -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- I meant to say that when bug 33080 introduced this, we should've reviewed all controllers. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=36482 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com, | |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 164264 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164264&action=edit Bug 36483: Make API controllers use $c->objects->to_api Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #164264|0 |1 is obsolete| | --- Comment #3 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 164266 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164266&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 164267 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164267&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |36495 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36495 [Bug 36495] Add resource_not_found() and resource_deleted() helpers -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #164266|0 |1 is obsolete| | --- Comment #5 from David Nind <david@davidnind.com> --- Created attachment 164293 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164293&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #164267|0 |1 is obsolete| | --- Comment #6 from David Nind <david@davidnind.com> --- Created attachment 164294 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164294&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 --- Comment #7 from Jonathan Druart <jonathan.druart@gmail.com> ---
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
Do we have a test for this? I don't find it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 --- Comment #8 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #7)
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.
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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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?? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact| |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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 ;) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|martin.renvoize@ptfs-europe |m.de.rooy@rijksmuseum.nl |.com | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #164293|0 |1 is obsolete| | --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 164803 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164803&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #164294|0 |1 is obsolete| | --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 164804 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164804&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |24.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 --- Comment #15 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.05! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|24.05.00 |24.05.00,23.11.06 released in| | CC| |fridolin.somers@biblibre.co | |m Status|Pushed to main |Pushed to stable --- Comment #16 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 23.11.x for 23.11.06 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Status|Pushed to stable |RESOLVED Resolution|--- |FIXED --- Comment #17 from Lucas Gass <lucas@bywatersolutions.com> --- Does not apply cleanly to 23.05.x, no backport. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36483 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=38177 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org