From 488818df2d7a90ce2dea0d031ee382c3aa2435ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Cohen=20Arazi?= Date: Fri, 27 Feb 2026 12:31:30 -0300 Subject: [PATCH] Bug 41950: Make +count embeds sortable via SQL COUNT subqueries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, +count embeds (e.g. claims+count) are computed in Perl via $object->$relation->count during to_api serialization. This means they cannot be used in _order_by and each count triggers a separate SQL query per row (N+1). This patch makes dbic_merge_prefetch detect +count embeds and, when a matching DBIC relationship exists, inject a correlated COUNT subquery via +select/+as instead of adding a prefetch. The order_by entries referencing count aliases are fixed up to strip the me. prefix, since +as aliases are virtual columns not bound to a table. In to_api, the is_count handler now checks for the pre-computed column via get_column before falling back to the Perl-level ->count call. This preserves backward compatibility for Koha-level methods that don't have a corresponding DBIC relationship. To test: 1. Run: $ prove t/Koha/REST/Plugin/Query.t => SUCCESS 2. Run: $ prove t/db_dependent/Koha/Object.t => SUCCESS 3. Run: $ prove t/db_dependent/Koha/REST/Plugin/Objects.t => SUCCESS 4. Run: $ prove -r t/db_dependent/api/v1 => SUCCESS Signed-off-by: Tomás Cohen Arazi --- Koha/Object.pm | 14 ++- Koha/REST/Plugin/Exceptions.pm | 8 ++ Koha/REST/Plugin/Query.pm | 157 ++++++++++++++++++++++++++++++++- 3 files changed, 172 insertions(+), 7 deletions(-) diff --git a/Koha/Object.pm b/Koha/Object.pm index 55c7e489a0..65887574c5 100644 --- a/Koha/Object.pm +++ b/Koha/Object.pm @@ -203,7 +203,7 @@ sub store { value => $value, property => $property =~ /(\w+\.\w+)$/ ? $1 - : $property, # results in table.column without quotes or backtics + : $property, # results in table.column without quotes or backticks ); } elsif ( $_->{msg} =~ /Data truncated for column \W?(?\w+)/ ) { # The optional \W in the regex might be a quote or backtick @@ -213,7 +213,7 @@ sub store { type => 'enum', property => $property =~ /(\w+\.\w+)$/ ? $1 - : $property, # results in table.column without quotes or backtics + : $property, # results in table.column without quotes or backticks value => $self->$property, ) if $type eq 'enum'; } @@ -680,7 +680,15 @@ sub to_api { { my $relation = $+{relation}; - $json_object->{$embed} = $self->$relation->count; + + # If the query was built with a SQL-level COUNT subquery (via +select/+as + # in _merge_count_select), the count is already available as a virtual column + # on the DBIC row. Use it to avoid an N+1 query per row. + # When no subquery was added (no DBIC relationship, or not called via the + # API search path), get_column returns undef and we fall back to the + # original $relation->count call, which always works. + my $count = eval { $self->_result->get_column($embed) }; + $json_object->{$embed} = defined $count ? $count : $self->$relation->count; } else { my $curr = $embed; my $next = $embeds->{$curr}->{children}; diff --git a/Koha/REST/Plugin/Exceptions.pm b/Koha/REST/Plugin/Exceptions.pm index 555633bb10..3ad1450176 100644 --- a/Koha/REST/Plugin/Exceptions.pm +++ b/Koha/REST/Plugin/Exceptions.pm @@ -73,6 +73,14 @@ sub register { error_code => 'invalid_query', } ); + } elsif ( blessed $exception && ref($exception) eq 'Koha::Exceptions::BadParameter' ) { + return $c->render( + status => 400, + json => { + error => "$exception", + error_code => 'bad_parameter', + } + ); } elsif ( blessed $exception && ref($exception) eq 'Koha::Exceptions::REST::Public::Authentication::Required' ) { diff --git a/Koha/REST/Plugin/Query.pm b/Koha/REST/Plugin/Query.pm index 5544d02ead..194a10085a 100644 --- a/Koha/REST/Plugin/Query.pm +++ b/Koha/REST/Plugin/Query.pm @@ -148,14 +148,64 @@ Generates the DBIC prefetch attribute based on embedded relations, and merges in return unless defined $embed; my @prefetches; + my $count_aliases = {}; + foreach my $key ( sort keys( %{$embed} ) ) { - my $parsed = _parse_prefetch( $key, $embed, $result_set ); + + # Handle +count embeds with SQL-level subquery + if ( $embed->{$key}->{is_count} ) { + my $subquery = _merge_count_select( $key, $attributes, $result_set ); + if ($subquery) { + $count_aliases->{$key} = $subquery; + next; + } + + # No DBIC relationship found; mark as unsortable (0) so the order_by + # fixup rejects it with a 400. Fall through so to_api can use the + # Koha method ->count fallback for display. + $count_aliases->{$key} = 0; + } + + my $parsed = _parse_prefetch( $key, $embed, $result_set, $attributes, undef, $count_aliases ); push @prefetches, $parsed if defined $parsed; } if ( scalar(@prefetches) ) { $attributes->{prefetch} = \@prefetches; } + + # Fix order_by entries referencing count aliases (me.X_count => X_count). + # dbic_merge_sorting (which runs before this) translates _order_by=-me.claims_count + # into { -desc => 'me.claims_count' }. But +as aliases are not real table columns, + # so DBIC needs them without the 'me.' prefix to match the +select subquery. + if ( $attributes->{order_by} ) { + + for my $i ( 0 .. $#{ $attributes->{order_by} } ) { + my $atom = $attributes->{order_by}[$i]; + if ( ref($atom) eq 'HASH' ) { + for my $dir ( keys %{$atom} ) { + ( my $col = $atom->{$dir} ) =~ s/^me\.//; + + # Reject sorting on +count embeds without a DBIC relationship. + # $count_aliases->{$col} is 0 (unsortable) or missing (top-level unsortable) + Koha::Exceptions::BadParameter->throw( + "Cannot sort on $col: no matching database relationship") + if exists $count_aliases->{$col} && !$count_aliases->{$col}; + + # Replace with the literal subquery so DBIC puts it directly + # in the ORDER BY clause (DBIC +as is Perl-side only, not a SQL alias) + $atom->{$dir} = $count_aliases->{$col} if $count_aliases->{$col}; + } + } elsif ( !ref($atom) ) { + ( my $col = $atom ) =~ s/^me\.//; + + Koha::Exceptions::BadParameter->throw("Cannot sort on $col: no matching database relationship") + if exists $count_aliases->{$col} && !$count_aliases->{$col}; + + $attributes->{order_by}[$i] = $count_aliases->{$col} if $count_aliases->{$col}; + } + } + } } ); @@ -449,19 +499,118 @@ sub _merge_embed { } } +=head3 _merge_count_select + + my $added = _merge_count_select( $key, $attributes, $result_set ); + +Given a I<$key> like C (from a C embed), strips the C<_count> +suffix to derive the DBIC relationship name, then uses the relationship metadata to build +a correlated COUNT subquery added via C<+select>/C<+as>. + +This makes the count available as a virtual column in the SQL result, enabling both +server-side sorting (ORDER BY) and an efficient single-query count instead of N+1. + +Returns the subquery scalar reference if added (used by the caller to substitute +into ORDER BY clauses), or undef if the relationship was not found in the +C or has no DBIC relationship info. In the latter case, the count +embed still works for display: C falls back transparently to the original +C<< $object->$relation->count >> Perl-level call. Only sorting is unavailable. + +Handles both C (plural, via C<_resultset>) and C (singular, +via C<_result>) for accessing the DBIC result source. + +=cut + +sub _merge_count_select { + my ( $key, $attributes, $result_set ) = @_; + + ( my $rel = $key ) =~ s/_count$//; + return unless exists $result_set->prefetch_whitelist->{$rel}; + + my $subquery = _build_count_subquery( $rel, $result_set, 'me' ); + return unless $subquery; + + push @{ $attributes->{'+select'} }, $subquery; + push @{ $attributes->{'+as'} }, $key; + + return $subquery; +} + +=head3 _build_count_subquery + + my $subquery = _build_count_subquery( $rel, $result_set, $parent_alias ); + +Builds a correlated COUNT subquery scalar ref for the given DBIC relationship. +I<$parent_alias> is the SQL alias of the parent table (e.g. C for top-level, +or the relation name for nested embeds like C). + +Returns the scalar ref, or undef if the relationship has no DBIC metadata. + +=cut + +sub _build_count_subquery { + my ( $rel, $result_set, $parent_alias ) = @_; + + my $source = + $result_set->can('_resultset') + ? $result_set->_resultset->result_source + : $result_set->_result->result_source; + my $rel_info = $source->relationship_info($rel); + return unless $rel_info; + + my $rel_table = $source->related_source($rel)->name; + my $cond = $rel_info->{cond}; + my ($fk_foreign) = keys %{$cond}; + my $fk_self = $cond->{$fk_foreign}; + $fk_foreign =~ s/^foreign\.//; + $fk_self =~ s/^self\.//; + + return \"(SELECT COUNT(*) FROM $rel_table WHERE $rel_table.$fk_foreign = $parent_alias.$fk_self)"; +} + sub _parse_prefetch { - my ( $key, $embed, $result_set ) = @_; + my ( $key, $embed, $result_set, $attributes, $parent_alias, $count_aliases ) = @_; + + $parent_alias //= 'me'; my $pref_key = $key; $pref_key =~ s/_count$// if $embed->{$key}->{is_count}; - return unless exists $result_set->prefetch_whitelist->{$pref_key}; + + if ( !exists $result_set->prefetch_whitelist->{$pref_key} ) { + + # Track unsortable nested +count embeds (no DBIC relationship) so the + # order_by fixup can reject them with a 400 instead of a DBIC 500 + if ( $embed->{$key}->{is_count} && $count_aliases ) { + my $alias_key = $parent_alias eq 'me' ? $key : "$parent_alias.$key"; + $count_aliases->{$alias_key} = 0; # 0 = unsortable + } + return; + } + + # Handle nested +count embeds with SQL-level subquery + if ( $embed->{$key}->{is_count} && $attributes ) { + my $subquery = _build_count_subquery( $pref_key, $result_set, $parent_alias ); + if ($subquery) { + + # Use the full dotted path as the alias key so order_by fixup + # can match e.g. "biblio.items_count" from _from_api_param + my $alias_key = $parent_alias eq 'me' ? $key : "$parent_alias.$key"; + push @{ $attributes->{'+select'} }, $subquery; + push @{ $attributes->{'+as'} }, $alias_key; + $count_aliases->{$alias_key} = $subquery if $count_aliases; + return; # no prefetch needed, count is in the subquery + } + } my $ko_class = $result_set->prefetch_whitelist->{$pref_key}; return $pref_key unless defined $embed->{$key}->{children} && defined $ko_class; my @prefetches; foreach my $child ( sort keys( %{ $embed->{$key}->{children} } ) ) { - my $parsed = _parse_prefetch( $child, $embed->{$key}->{children}, $ko_class->new ); + my $parsed = _parse_prefetch( + $child, $embed->{$key}->{children}, $ko_class->new, $attributes, $pref_key, + $count_aliases + ); push @prefetches, $parsed if defined $parsed; } -- 2.50.1 (Apple Git-155)