Bug 16965 - Add the Koha::Objects->search_related method
Summary: Add the Koha::Objects->search_related method
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 13019 16961
Blocks: 16966
  Show dependency treegraph
 
Reported: 2016-07-22 16:07 UTC by Jonathan Druart
Modified: 2017-06-14 22:16 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 16965: Add Koha::Objects->search_related (2.40 KB, patch)
2016-07-22 16:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16965: search_related returns an instanciated Koha::Objects-based object (4.78 KB, patch)
2016-07-25 10:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16965: [Follow-up] Adjust get_object_class (3.36 KB, patch)
2016-07-28 12:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16965: Add Koha::Objects->search_related (2.59 KB, patch)
2016-10-04 09:34 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16965: search_related returns an instanciated Koha::Objects-based object (4.91 KB, patch)
2016-10-04 09:35 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16965: [Follow-up] Adjust get_object_class (3.44 KB, patch)
2016-10-04 09:36 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16965: Add Koha::Objects->search_related (2.65 KB, patch)
2016-10-12 09:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16965: search_related returns an instanciated Koha::Objects-based object (4.97 KB, patch)
2016-10-12 09:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16965: [Follow-up] Adjust get_object_class (3.50 KB, patch)
2016-10-12 09:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16965: Add Koha::Objects->search_related (2.72 KB, patch)
2016-10-13 12:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16965: search_related returns an instanciated Koha::Objects-based object (4.97 KB, patch)
2016-10-13 12:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16965: [Follow-up] Adjust get_object_class (3.50 KB, patch)
2016-10-13 12:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16965: Allow Koha::Objects->result_class (777 bytes, patch)
2016-10-13 12:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 16965: Add Koha::Objects->search_related (2.80 KB, patch)
2016-10-19 10:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 16965: search_related returns an instanciated Koha::Objects-based object (5.05 KB, patch)
2016-10-19 10:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 16965: [Follow-up] Adjust get_object_class (3.57 KB, patch)
2016-10-19 10:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 16965: Allow Koha::Objects->result_class (854 bytes, patch)
2016-10-19 10:27 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-07-22 16:07:35 UTC

    
Comment 1 Jonathan Druart 2016-07-22 16:21:31 UTC
Created attachment 53656 [details] [review]
Bug 16965: Add Koha::Objects->search_related

In order to search on relations, we need this new method.

Test plan:
Confirm that the changes in Objects.t make sense and that the tests
pass.
Comment 2 Marcel de Rooy 2016-07-22 16:45:52 UTC
Shouldn't you wrap them like search does ?
Comment 3 Jonathan Druart 2016-07-22 17:16:28 UTC
(In reply to Marcel de Rooy from comment #2)
> Shouldn't you wrap them like search does ?

I don't think so:
"In list context, ->all() is called implicitly on the resultset, thus returning a list of result objects instead."
https://metacpan.org/pod/DBIx::Class::ResultSet#search_related
Comment 4 Marcel de Rooy 2016-07-25 06:50:18 UTC
(In reply to Jonathan Druart from comment #3)
> (In reply to Marcel de Rooy from comment #2)
> > Shouldn't you wrap them like search does ?
> 
> I don't think so:
> "In list context, ->all() is called implicitly on the resultset, thus
> returning a list of result objects instead."
> https://metacpan.org/pod/DBIx::Class::ResultSet#search_related

I mean the other search:
sub search {
    my ( $self, $params, $attributes ) = @_;

    if (wantarray) {
        my @dbic_rows = $self->_resultset()->search($params, $attributes);

        return $self->_wrap(@dbic_rows);
etc.
Comment 5 Jonathan Druart 2016-07-25 10:25:01 UTC
Created attachment 53673 [details] [review]
Bug 16965: search_related returns an instanciated Koha::Objects-based object

Koha::Objects->search_related should return a Koha::Objects-based
object.
This search_related method should follow the same rules as the search
method, i.e. take into account what the caller want (scalar or list).
The problem here is that we do not know (in Koha::Objects) what is the
kind of objects we want to instanciate. To know it, this patch adds a
get_object_class, it will return the class of the object and the
resultset Koha::Object-based object.

The drawback of this method is that we will have to keep it up-to-date
every time we add a new Koha::Object class.
Comment 6 Marcel de Rooy 2016-07-28 12:54:46 UTC
Created attachment 53777 [details] [review]
Bug 16965: [Follow-up] Adjust get_object_class

In many cases it should be possible to derive the Koha::Objects class
name from the DBIx result class name in a trivial manner.
If not, the DBIx result class should have a class method called
koha_objects_class providing that non-trivial name, such as Libraries
for Branch.

Note: We are only interested in the plural form here (Koha::Objects).

Test plan:
Run t/db_dependent/Koha/Objects.t
Comment 7 Marcel de Rooy 2016-07-28 12:55:24 UTC
Jonathan: What do you think of the approach in the third patch?
Comment 8 Jonathan Druart 2016-08-01 10:56:07 UTC
(In reply to Marcel de Rooy from comment #7)
> Jonathan: What do you think of the approach in the third patch?

I liked the idea to have the mappings in one place, it will be easier to know which classes are already mapped.

Why did you make this change:
-        return $object_class->_wrap(@dbic_rows);
+        return _wrap( $object_class, @dbic_rows );
?
Comment 9 Jonathan Druart 2016-08-01 10:57:07 UTC
Ccing Tomas, Kyle and Martin to get their opinions on this last patch.
Comment 10 Marcel de Rooy 2016-08-01 11:05:45 UTC
(In reply to Jonathan Druart from comment #8)
> Why did you make this change:
> -        return $object_class->_wrap(@dbic_rows);
> +        return _wrap( $object_class, @dbic_rows );
> ?

Hmm. Somewhere in the process of testing. But we can redo it? We should not expect a Koha Object class to override a private underscored routine. So this should be exactly the same.
Comment 11 Jonathan Druart 2016-08-01 11:13:49 UTC
(In reply to Marcel de Rooy from comment #10)
> (In reply to Jonathan Druart from comment #8)
> > Why did you make this change:
> > -        return $object_class->_wrap(@dbic_rows);
> > +        return _wrap( $object_class, @dbic_rows );
> > ?
> 
> Hmm. Somewhere in the process of testing. But we can redo it? We should not
> expect a Koha Object class to override a private underscored routine. So
> this should be exactly the same.

Yes, that's the same, that's why I have asked why :)
Comment 12 Katrin Fischer 2016-09-18 14:53:21 UTC
Hm, I think Marcel is offline for the next 2 weeks (according to his last goodbye on IRC) - can we move on with this somewhow? It's the next bug needed to unlock others in the dependency tree.
Comment 13 Martin Renvoize 2016-09-19 07:43:46 UTC
To be frank, I think someone needs to go back to basics with me and explain why/what advantages Koha::Objects actually bring us above and beyond DBIx::Class.. It's feeling more and more like we're just wrapping DBIx::Class for the sake of wrapping it and not really winning anything.

As such, I can't really comment here.. I use ->search_related allot within a DBIx::Class based app I've written and believe it's certainly a useful method. Beyond that, I can't comment.
Comment 14 Jonathan Druart 2016-09-19 08:43:26 UTC
(In reply to Martin Renvoize from comment #13)
> To be frank, I think someone needs to go back to basics with me and explain
> why/what advantages Koha::Objects actually bring us above and beyond
> DBIx::Class.. It's feeling more and more like we're just wrapping
> DBIx::Class for the sake of wrapping it and not really winning anything.

See
 Bug 17091 - Add AUTOLOAD to Koha::Objects
and
 Bug 17226 - Improve AUTOLOAD of Koha::Object
Comment 15 Marc Véron 2016-10-04 09:34:31 UTC
Created attachment 56009 [details] [review]
Bug 16965: Add Koha::Objects->search_related

In order to search on relations, we need this new method.

Test plan:
Confirm that the changes in Objects.t make sense and that the tests
pass.

Tested all 3 patches together, followed test plan, result OK
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 16 Marc Véron 2016-10-04 09:35:31 UTC
Created attachment 56010 [details] [review]
Bug 16965: search_related returns an instanciated Koha::Objects-based object

Koha::Objects->search_related should return a Koha::Objects-based
object.
This search_related method should follow the same rules as the search
method, i.e. take into account what the caller want (scalar or list).
The problem here is that we do not know (in Koha::Objects) what is the
kind of objects we want to instanciate. To know it, this patch adds a
get_object_class, it will return the class of the object and the
resultset Koha::Object-based object.

The drawback of this method is that we will have to keep it up-to-date
every time we add a new Koha::Object class.

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 17 Marc Véron 2016-10-04 09:36:44 UTC
Created attachment 56011 [details] [review]
Bug 16965: [Follow-up] Adjust get_object_class

In many cases it should be possible to derive the Koha::Objects class
name from the DBIx result class name in a trivial manner.
If not, the DBIx result class should have a class method called
koha_objects_class providing that non-trivial name, such as Libraries
for Branch.

Note: We are only interested in the plural form here (Koha::Objects).

Test plan:
Run t/db_dependent/Koha/Objects.t

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 18 Tomás Cohen Arazi 2016-10-12 09:39:08 UTC
Created attachment 56238 [details] [review]
Bug 16965: Add Koha::Objects->search_related

In order to search on relations, we need this new method.

Test plan:
Confirm that the changes in Objects.t make sense and that the tests
pass.

Tested all 3 patches together, followed test plan, result OK
Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Tomás Cohen Arazi 2016-10-12 09:39:14 UTC
Created attachment 56239 [details] [review]
Bug 16965: search_related returns an instanciated Koha::Objects-based object

Koha::Objects->search_related should return a Koha::Objects-based
object.
This search_related method should follow the same rules as the search
method, i.e. take into account what the caller want (scalar or list).
The problem here is that we do not know (in Koha::Objects) what is the
kind of objects we want to instanciate. To know it, this patch adds a
get_object_class, it will return the class of the object and the
resultset Koha::Object-based object.

The drawback of this method is that we will have to keep it up-to-date
every time we add a new Koha::Object class.

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Tomás Cohen Arazi 2016-10-12 09:39:18 UTC
Created attachment 56240 [details] [review]
Bug 16965: [Follow-up] Adjust get_object_class

In many cases it should be possible to derive the Koha::Objects class
name from the DBIx result class name in a trivial manner.
If not, the DBIx result class should have a class method called
koha_objects_class providing that non-trivial name, such as Libraries
for Branch.

Note: We are only interested in the plural form here (Koha::Objects).

Test plan:
Run t/db_dependent/Koha/Objects.t

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Jonathan Druart 2016-10-13 12:26:36 UTC
Created attachment 56372 [details] [review]
Bug 16965: Add Koha::Objects->search_related

In order to search on relations, we need this new method.

Test plan:
Confirm that the changes in Objects.t make sense and that the tests
pass.

Tested all 3 patches together, followed test plan, result OK
Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Jonathan Druart 2016-10-13 12:26:41 UTC
Created attachment 56373 [details] [review]
Bug 16965: search_related returns an instanciated Koha::Objects-based object

Koha::Objects->search_related should return a Koha::Objects-based
object.
This search_related method should follow the same rules as the search
method, i.e. take into account what the caller want (scalar or list).
The problem here is that we do not know (in Koha::Objects) what is the
kind of objects we want to instanciate. To know it, this patch adds a
get_object_class, it will return the class of the object and the
resultset Koha::Object-based object.

The drawback of this method is that we will have to keep it up-to-date
every time we add a new Koha::Object class.

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Jonathan Druart 2016-10-13 12:26:46 UTC
Created attachment 56374 [details] [review]
Bug 16965: [Follow-up] Adjust get_object_class

In many cases it should be possible to derive the Koha::Objects class
name from the DBIx result class name in a trivial manner.
If not, the DBIx result class should have a class method called
koha_objects_class providing that non-trivial name, such as Libraries
for Branch.

Note: We are only interested in the plural form here (Koha::Objects).

Test plan:
Run t/db_dependent/Koha/Objects.t

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Jonathan Druart 2016-10-13 12:40:56 UTC
Created attachment 56375 [details] [review]
Bug 16965: Allow Koha::Objects->result_class
Comment 25 Martin Renvoize 2016-10-14 16:54:35 UTC
So.. I'm mostly OK with these patches.. but I have a related question:

What's the history around the continual regeneration of dbic result classes?  

My general understanding of dbic over the past couple of years is that 'dbicdump' is really about allowing a migration path from a non-dbic system to a system with dbic classes. It's a one time thing, and then you should really maintain the classes and instead generate the db changes from them as opposed to the other way around.  If we did this, we could happily rename our resultset and result classes and remove some of the code here that requires mapping from ::Object to ::Result classes.

Just something to bare in mind.  I could happily write a proof of concept for such a trivial dbic ::Result object example should people be interested in such.
Comment 26 Marcel de Rooy 2016-10-17 07:34:24 UTC
(In reply to Martin Renvoize from comment #25)
> So.. I'm mostly OK with these patches.. but I have a related question:
> 
> What's the history around the continual regeneration of dbic result classes?
> 
> 
> My general understanding of dbic over the past couple of years is that
> 'dbicdump' is really about allowing a migration path from a non-dbic system
> to a system with dbic classes. It's a one time thing, and then you should
> really maintain the classes and instead generate the db changes from them as
> opposed to the other way around.  If we did this, we could happily rename
> our resultset and result classes and remove some of the code here that
> requires mapping from ::Object to ::Result classes.
> 
> Just something to bare in mind.  I could happily write a proof of concept
> for such a trivial dbic ::Result object example should people be interested
> in such.

Yes, I think we should move forward too and obsolete kohastructure.sql etc.
iirc there were problems with deploying (instead of running kohastructure for a new install), perhaps on some specific systems? Running the sql file probably was considered safer.
Furthermore we would need to address how to deploy for existing installs in updatedatabase.
Comment 27 Martin Renvoize 2016-10-17 09:39:37 UTC
This is probably a conversation for elsewhere.. I'm going to pass qa on these for now to prevent a holdup in the queue and in the knowledge that we can fix the dbic usage later fairly trivially and just clean out this code again when we have.

For deployment, I actually use DBIC::DeploymentHandler myself in other apps.. it's a great tool, and would happily cope with a move from kohastructure.sql to a versioned schema scheme I believe.. It is however very dependency heavy so we may need to brew our own :(
Comment 28 Martin Renvoize 2016-10-19 10:26:49 UTC
Created attachment 56650 [details] [review]
[PASSED QA] Bug 16965: Add Koha::Objects->search_related

In order to search on relations, we need this new method.

Test plan:
Confirm that the changes in Objects.t make sense and that the tests
pass.

Tested all 3 patches together, followed test plan, result OK
Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2016-10-19 10:27:00 UTC
Created attachment 56651 [details] [review]
[PASSED QA] Bug 16965: search_related returns an instanciated Koha::Objects-based object

Koha::Objects->search_related should return a Koha::Objects-based
object.
This search_related method should follow the same rules as the search
method, i.e. take into account what the caller want (scalar or list).
The problem here is that we do not know (in Koha::Objects) what is the
kind of objects we want to instanciate. To know it, this patch adds a
get_object_class, it will return the class of the object and the
resultset Koha::Object-based object.

The drawback of this method is that we will have to keep it up-to-date
every time we add a new Koha::Object class.

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Martin Renvoize 2016-10-19 10:27:05 UTC
Created attachment 56652 [details] [review]
[PASSED QA] Bug 16965: [Follow-up] Adjust get_object_class

In many cases it should be possible to derive the Koha::Objects class
name from the DBIx result class name in a trivial manner.
If not, the DBIx result class should have a class method called
koha_objects_class providing that non-trivial name, such as Libraries
for Branch.

Note: We are only interested in the plural form here (Koha::Objects).

Test plan:
Run t/db_dependent/Koha/Objects.t

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize 2016-10-19 10:27:09 UTC
Created attachment 56653 [details] [review]
[PASSED QA] Bug 16965: Allow Koha::Objects->result_class

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Martin Renvoize 2016-10-19 10:29:41 UTC
So much as I'm an opponent of Koha::Objects (They still just feel like boilerplate for the point of boilerplate to me), I can't find anything desperately wrong with this code. 

Passing QA
Comment 33 Kyle M Hall 2016-10-21 17:32:51 UTC
Pushed to master for 16.11, thanks Jonathan, Marcel!