From 3b613fb73f0522c61c7e8941afdb4a3ee366064f Mon Sep 17 00:00:00 2001 From: Pedro Amorim Date: Thu, 25 Jul 2024 15:28:37 +0000 Subject: [PATCH] Bug 37389: ExtendedAttributes mixin This is what we're doing here: - Creating a new mixin called ExtendedAttributes.pm - Moving the extended_attributes 'join' logic out of REST/Plugin/Query and instead applying it to the aforementioned Mixin. Moving this to this level allows for this consistent behavior to happen on all search queries including, but not limited to, search queries happening on the REST API. - Applying this Mixin to Patrons and ILL::Requests (we don't apply it to AdditionalFields.pm here yet because no AdditionalFields supporting classes have the extended_attributes accessor yet, I'll tackle this when rebasing 35287) - The aforementioned mixin does the following: -- Generates dynamic accessors for extended_attributes e.g. if there is a borrower attribute with code 'height', the 'extended_attributes_height' accessor is generated dynamically if a search with 'prefetch'=>'extended_attributes' AND the extended_attribute.code = 'height' is performed. -- Rewrites the 'join' entries in the query to have the aliases as above. -- Rewrites the WHERE conditions to match the above ruleset. Example: A DBIX search query as follows: [ { '-and' => [ [ { 'extended_attributes.attribute' => { 'like' => 'abc%' }, 'extended_attributes.code' => 'CODE_1' } ], [ { 'extended_attributes.code' => 'CODE_2', 'extended_attributes.attribute' => { 'like' => '123%' } } ] ] } ] Results in the following SQL: SELECT `me`.`borrowernumber` FROM `borrowers` `me` LEFT JOIN `borrower_attributes` `extended_attributes_CODE_1` ON ( `extended_attributes_CODE_1`.`borrowernumber` = `me`.`borrowernumber` AND `extended_attributes_CODE_1`.`code` = ? ) LEFT JOIN `borrower_attributes` `extended_attributes_CODE_2` ON ( `extended_attributes_CODE_2`.`borrowernumber` = `me`.`borrowernumber` AND `extended_attributes_CODE_2`.`code` = ? ) WHERE ( ( ( `extended_attributes_CODE_1`.`attribute` LIKE ? AND `extended_attributes_CODE_1`.`code` = ? ) AND ( `extended_attributes_CODE_2`.`attribute` LIKE ? AND `extended_attributes_CODE_2`.`code` = ? ) ) ) What fixes the performance issue that originated this work is the 'AND `extended_attributes_CODE_1`.`code` = ?' that was missing on the LEFT JOIN. All of the above is explained using Borrowers and Borrower attributes, but it all also applies to ILL::Requests and ILL::Request::Attributes. Co-authored-by: Martin Renvoize Signed-off-by: David Nind Signed-off-by: Tomas Cohen Arazi Signed-off-by: Katrin Fischer --- Koha/ILL/Requests.pm | 16 +- Koha/Objects/Mixin/ExtendedAttributes.pm | 234 +++++++++++++++++++++++ Koha/Patrons.pm | 15 +- Koha/REST/Plugin/Objects.pm | 8 - Koha/REST/Plugin/Query.pm | 153 --------------- 5 files changed, 263 insertions(+), 163 deletions(-) create mode 100644 Koha/Objects/Mixin/ExtendedAttributes.pm diff --git a/Koha/ILL/Requests.pm b/Koha/ILL/Requests.pm index 468bd34d82..39ca48d3e9 100644 --- a/Koha/ILL/Requests.pm +++ b/Koha/ILL/Requests.pm @@ -22,8 +22,9 @@ use Modern::Perl; use Koha::Database; use Koha::ILL::Request; use Koha::ILL::Request::Config; +use Koha::Objects::Mixin::ExtendedAttributes; -use base qw(Koha::Objects); +use base qw(Koha::Objects::Mixin::ExtendedAttributes Koha::Objects); =head1 NAME @@ -106,6 +107,19 @@ sub search_incomplete { } ); } +=head3 extended_attributes_config + +=cut + +sub extended_attributes_config { + my ( $self ) = @_; + return { + 'id_field' => { 'foreign' => 'illrequest_id', 'self' => 'illrequest_id' }, + 'key_field' => 'type', + 'schema_class' => 'Koha::Schema::Result::Illrequestattribute', + }; +} + =head2 Internal methods =head3 _type diff --git a/Koha/Objects/Mixin/ExtendedAttributes.pm b/Koha/Objects/Mixin/ExtendedAttributes.pm new file mode 100644 index 0000000000..386d657535 --- /dev/null +++ b/Koha/Objects/Mixin/ExtendedAttributes.pm @@ -0,0 +1,234 @@ +package Koha::Objects::Mixin::ExtendedAttributes; + +# Copyright 2024 PTFS Europe Ltd +# +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; +use Scalar::Util qw( reftype ); + +=head1 NAME + +Koha::Objects::Mixin::ExtendedAttributes + +=head2 Class methods + + +=head3 search + + Overwrites the search method to include extended attributes rewrite and dynamic relation accessors + +=cut + +sub search { + my ( $self, $params, $attributes ) = @_; + + my $class = ref($self) ? ref($self) : $self; + + $self->handle_query_extended_attributes( + { + attributes => $attributes, + filtered_params => $params, + } + ); + + my $rs = $self->_resultset()->search( $params, $attributes ); + return $class->_new_from_dbic($rs); +} + +=head3 handle_query_extended_attributes + + Checks for the presence of extended_attributes in a query + If present, it builds the dynamic extended attributes relations and rewrites the query to include the extended_attributes relation + +=cut + +sub handle_query_extended_attributes { + my ( $self, $args ) = @_; + + my $attributes = $args->{attributes}; + my $filtered_params = $args->{filtered_params}; + + if ( reftype( $attributes->{prefetch} ) + && reftype( $attributes->{prefetch} ) eq 'ARRAY' + && grep ( /extended_attributes/, @{ $attributes->{prefetch} } ) ) + { + + my @array = $self->_get_extended_attributes_entries($filtered_params); + + # Calling our private method to build the extended attributes relations + my @joins = $self->_build_extended_attributes_relations( \@array ); + push @{ $attributes->{join} }, @joins; + + } +} + +=head3 _get_extended_attributes_entries + + $self->_get_extended_attributes_entries( $filtered_params, 0 ) + +Recursive function that returns the rewritten extended_attributes query entries. + +Given: +[ + '-and', + [ + { + 'extended_attributes.code' => 'CODE_1', + 'extended_attributes.attribute' => { 'like' => '%Bar%' } + }, + { + 'extended_attributes.attribute' => { 'like' => '%Bar%' }, + 'extended_attributes.code' => 'CODE_2' + } + ] +]; + +Returns : + +[ + 'CODE_1', + 'CODE_2' +] + +=cut + +sub _get_extended_attributes_entries { + my ( $self, $params, @array ) = @_; + + if ( reftype($params) && reftype($params) eq 'HASH' ) { + + # rewrite additional_field_values table query params + @array = _rewrite_related_metadata_query( $params, 'field_id', 'value', @array ) + if $params->{'extended_attributes.field_id'}; + + # rewrite borrower_attributes table query params + @array = _rewrite_related_metadata_query( $params, 'code', 'attribute', @array ) + if $params->{'extended_attributes.code'}; + + # rewrite illrequestattributes table query params + @array = _rewrite_related_metadata_query( $params, 'type', 'value', @array ) + if $params->{'extended_attributes.type'}; + + foreach my $key ( keys %{$params} ) { + return $self->_get_extended_attributes_entries( $params->{$key}, @array ); + } + } elsif ( reftype($params) && reftype($params) eq 'ARRAY' ) { + foreach my $ea_instance (@$params) { + @array = $self->_get_extended_attributes_entries( $ea_instance, @array ); + } + return @array; + } else { + return @array; + } +} + +=head3 _rewrite_related_metadata_query + + $extended_attributes_entries = + _rewrite_related_metadata_query( $params, 'field_id', 'value', @array ) + +Helper function that rewrites all subsequent extended_attributes queries to match the alias generated by the dbic self left join +Take the below example (patrons): + [ + { + "extended_attributes.attribute":{"like":"%123%"}, + "extended_attributes.code":"CODE_1" + } + ], + [ + { + "extended_attributes.attribute":{"like":"%abc%" }, + "extended_attributes.code":"CODE_2" + } + ] + +It'll be rewritten as: + [ + { + 'extended_attributes_CODE_1.attribute' => { 'like' => '%123%' }, + 'extended_attributes_CODE_1.code' => 'CODE_1' + } + ], + [ + { + 'extended_attributes_CODE_2.attribute' => { 'like' => '%abc%' }, + 'extended_attributes_CODE_2.code' => 'CODE_2' + } + ] + +=cut + +sub _rewrite_related_metadata_query { + my ( $params, $key, $value, @array ) = @_; + + if ( ref \$params->{ 'extended_attributes.' . $key } eq 'SCALAR' ) { + my $old_key_value = delete $params->{ 'extended_attributes.' . $key }; + my $new_key_value = "extended_attributes_$old_key_value" . "." . $key; + $params->{$new_key_value} = $old_key_value; + + my $old_value_value = delete $params->{ 'extended_attributes.' . $value }; + my $new_value_value = "extended_attributes_$old_key_value" . "." . $value; + $params->{$new_value_value} = $old_value_value; + push @array, $old_key_value; + } + + return @array; +} + +=head3 _build_extended_attributes_relations + + Method to dynamically add has_many relations for Koha classes that support extended_attributes. + + Returns a list of relation accessor names. + +=cut + +sub _build_extended_attributes_relations { + my ( $self, $types ) = @_; + + return if( !grep ( /extended_attributes/, keys %{$self->_resultset->result_source->_relationships} ) ); + + my $ea_config = $self->extended_attributes_config; + + my $result_source = $self->_resultset->result_source; + for my $type ( @{$types} ) { + $result_source->add_relationship( + "extended_attributes_$type", + "$ea_config->{schema_class}", + sub { + my $args = shift; + + return { + "$args->{foreign_alias}.$ea_config->{id_field}->{foreign}" => + { -ident => "$args->{self_alias}.$ea_config->{id_field}->{self}" }, + "$args->{foreign_alias}.$ea_config->{key_field}" => { '=', $type }, + }; + }, + { + accessor => 'multi', + join_type => 'LEFT', + cascade_copy => 0, + cascade_delete => 0, + is_depends_on => 0 + }, + ); + + } + return map { 'extended_attributes_' . $_ } @{$types}; +} + +1; \ No newline at end of file diff --git a/Koha/Patrons.pm b/Koha/Patrons.pm index eb89573312..813d741c2a 100644 --- a/Koha/Patrons.pm +++ b/Koha/Patrons.pm @@ -29,7 +29,7 @@ use Koha::Patron; use Koha::Exceptions::Patron; use Koha::Patron::Categories; -use base qw(Koha::Objects); +use base qw(Koha::Objects::Mixin::ExtendedAttributes Koha::Objects); =head1 NAME @@ -566,6 +566,19 @@ sub filter_by_have_permission { ); } +=head3 extended_attributes_config + +=cut + +sub extended_attributes_config { + my ( $self ) = @_; + return { + 'id_field' => { 'foreign' => 'borrowernumber', 'self' => 'borrowernumber' }, + 'key_field' => 'code', + 'schema_class' => 'Koha::Schema::Result::BorrowerAttribute', + }; +} + =head3 _type =cut diff --git a/Koha/REST/Plugin/Objects.pm b/Koha/REST/Plugin/Objects.pm index 96c4fcaeff..a6b9ab6c3d 100644 --- a/Koha/REST/Plugin/Objects.pm +++ b/Koha/REST/Plugin/Objects.pm @@ -287,14 +287,6 @@ controller, and thus shouldn't be called twice in it. $c->stash('koha.pagination.base_total' => $result_set->count); $c->stash('koha.pagination.query_params' => $args); - # Check and handle related metadata table joins - $c->dbic_extended_attributes_join( - { - attributes => $attributes, - filtered_params => $filtered_params - } - ); - $c->dbic_validate_operators( { filtered_params => $filtered_params } ); # Generate the resultset diff --git a/Koha/REST/Plugin/Query.pm b/Koha/REST/Plugin/Query.pm index cfabc44d31..01fa0e8cd4 100644 --- a/Koha/REST/Plugin/Query.pm +++ b/Koha/REST/Plugin/Query.pm @@ -160,34 +160,6 @@ Generates the DBIC prefetch attribute based on embedded relations, and merges in } ); -=head3 dbic_extended_attributes_join - - $attributes = $c->dbic_extended_attributes_join({ attributes => $attributes, result_set => $result_set }); - -Generates the DBIC join attribute based on extended_attributes query entries, and merges into I<$attributes>. - -=cut - - $app->helper( - 'dbic_extended_attributes_join' => sub { - my ( $c, $args ) = @_; - - my $attributes = $args->{attributes}; - my $filtered_params = $args->{filtered_params}; - - if ( reftype( $attributes->{prefetch} ) - && reftype( $attributes->{prefetch} ) eq 'ARRAY' - && grep ( /extended_attributes/, @{ $attributes->{prefetch} } ) ) - { - my $ea_entries = $self->_get_extended_attributes_entries( $filtered_params, 0 ); - while ( $ea_entries > 0 ) { - push( @{ $attributes->{join} }, 'extended_attributes' ); - $ea_entries--; - } - } - } - ); - =head3 dbic_validate_operators $attributes = $c->dbic_validate_operators( { filtered_params => $filtered_params } ); @@ -532,131 +504,6 @@ sub _parse_dbic_query { } } -=head3 _get_extended_attributes_entries - - $self->_get_extended_attributes_entries( $filtered_params, 0 ) - -Recursive function that returns the number of extended_attributes entries present in a query. -Example: Returns 2 if given a $filtered_params containing the below: - -{ - "-and":[ - [ - { - "extended_attributes.value":{ - "like":"abc%" - }, - "extended_attributes.code":[ - [ - "arbitrary_attr_code", - "another_attr_code" - ] - ] - } - ], - [ - { - "extended_attributes.value":{ - "like":"123%" - }, - "extended_attributes.code":[ - [ - "arbitrary_attr_code", - "another_attr_code" - ] - ] - } - ] - ] -} - -=cut - -sub _get_extended_attributes_entries { - my ( $self, $params, $extended_attributes_entries ) = @_; - - if ( reftype($params) && reftype($params) eq 'HASH' ) { - - # rewrite additional_field_values table query params - $extended_attributes_entries = - _rewrite_related_metadata_query( $params, $extended_attributes_entries, 'field_id', 'value' ) - if $params->{'extended_attributes.field_id'}; - - # rewrite borrower_attributes table query params - $extended_attributes_entries = - _rewrite_related_metadata_query( $params, $extended_attributes_entries, 'code', 'attribute' ) - if $params->{'extended_attributes.code'}; - - # rewrite illrequestattributes table query params - $extended_attributes_entries = - _rewrite_related_metadata_query( $params, $extended_attributes_entries, 'type', 'value' ) - if $params->{'extended_attributes.type'}; - - foreach my $key ( keys %{$params} ) { - return $self->_get_extended_attributes_entries( $params->{$key}, $extended_attributes_entries ); - } - } elsif ( reftype($params) && reftype($params) eq 'ARRAY' ) { - foreach my $ea_instance (@$params) { - $extended_attributes_entries = - +$self->_get_extended_attributes_entries( $ea_instance, $extended_attributes_entries ); - } - return $extended_attributes_entries; - } else { - return $extended_attributes_entries; - } -} - -=head3 _rewrite_related_metadata_query - - $extended_attributes_entries = - _rewrite_related_metadata_query( $params, $extended_attributes_entries, 'field_id', 'value' ) - -Helper function that rewrites all subsequent extended_attributes queries to match the alias generated by the dbic self left join -Take the below example (patrons), assuming it is the third instance of an extended_attributes query: -{ - "extended_attributes.value":{ - "like":"123%" - }, - "extended_attributes.code":[ - [ - "arbitrary_attr_code", - "another_attr_code" - ] - ] -} - -It'll be rewritten as: -{ - "extended_attributes_3.value":{ - "like":"123%" - }, - "extended_attributes_3.code":[ - [ - "arbitrary_attr_code", - "another_attr_code" - ] - ] -} - -=cut - -sub _rewrite_related_metadata_query { - my ( $params, $extended_attributes_entries, $key, $value ) = @_; - - $extended_attributes_entries++; - if ( $extended_attributes_entries > 1 ) { - my $old_key_value = delete $params->{ 'extended_attributes.' . $key }; - my $new_key_value = "extended_attributes_$extended_attributes_entries" . "." . $key; - $params->{$new_key_value} = $old_key_value; - - my $old_value_value = delete $params->{ 'extended_attributes.' . $value }; - my $new_value_value = "extended_attributes_$extended_attributes_entries" . "." . $value; - $params->{$new_value_value} = $old_value_value; - } - - return $extended_attributes_entries; -} - =head3 _validate_operator =cut -- 2.39.5