Bug 37389 - REST API queries joining on extended_attributes may cause severe performance issues
Summary: REST API queries joining on extended_attributes may cause severe performance ...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Pedro Amorim
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 30645
Blocks: 35287
  Show dependency treegraph
 
Reported: 2024-07-17 08:54 UTC by Pedro Amorim
Modified: 2024-08-29 17:43 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes a severe performance issue with a REST API SQL query for patron and interlibrary loan request custom attributes. It fixes the problematic join queries using a "mixin" and adds tests. The previous queries could in some circumstance severally affect the database performance.
Version(s) released in:
24.11.00,24.05.04
Circulation function:


Attachments
Bug 37389: Add a builder to dynamically create relations when required (1.89 KB, patch)
2024-07-18 12:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37389: Add AUTOLOAD to dynamically add relation accessors (1.43 KB, patch)
2024-07-18 13:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37389: PoC Follow-up (5.53 KB, patch)
2024-07-18 14:49 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37389: More WIP (12.30 KB, patch)
2024-07-18 16:23 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37389: Working accessors (2.91 KB, patch)
2024-07-18 17:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37389: Working accessors (3.51 KB, patch)
2024-07-19 07:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37389: More silly fixes (1.54 KB, patch)
2024-07-19 09:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37389: (9.08 KB, patch)
2024-07-19 09:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37389: Fix performance problems with extended attributes on the API (9.14 KB, patch)
2024-07-19 09:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37389: Fix performance problems with extended attributes on the API (9.20 KB, patch)
2024-07-19 09:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37389: [SQUASH] Join alias fix (4.57 KB, patch)
2024-07-19 14:35 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37389: _build_extended_attributes_relations update (4.89 KB, patch)
2024-07-19 16:02 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37389: Move AUTOLOAD out of ILL::Request into Mixin (3.84 KB, patch)
2024-07-19 16:02 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37389: Apply ExtendedAttributes Mixin to Patrons (1.11 KB, patch)
2024-07-19 16:02 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37389: [WIP] Update tests (1014 bytes, patch)
2024-07-19 16:02 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37389: [WIP] Update tests (4.13 KB, patch)
2024-07-22 16:20 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37389: Apply ExtendedAttributes Mixin to AdditionalFields (4.00 KB, patch)
2024-07-22 16:20 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37389: Add tests (9.66 KB, patch)
2024-07-25 15:39 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37389: ExtendedAttributes mixin (18.36 KB, patch)
2024-07-25 15:39 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37389: Add tests (9.66 KB, patch)
2024-07-25 15:53 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37389: ExtendedAttributes mixin (18.65 KB, patch)
2024-07-25 15:53 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37389: Add tests (9.66 KB, patch)
2024-07-25 15:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37389: ExtendedAttributes mixin (18.70 KB, patch)
2024-07-25 15:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37389: Add tests (9.71 KB, patch)
2024-07-29 23:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 37389: ExtendedAttributes mixin (18.75 KB, patch)
2024-07-29 23:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 37389: Add tests (9.76 KB, patch)
2024-08-01 12:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37389: ExtendedAttributes mixin (18.81 KB, patch)
2024-08-01 12:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37389: (QA follow-up) Tidy (4.02 KB, patch)
2024-08-01 12:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 37389: Add tests (9.93 KB, patch)
2024-08-01 16:25 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37389: ExtendedAttributes mixin (18.77 KB, patch)
2024-08-01 16:25 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37389: (QA follow-up) Tidy (4.01 KB, patch)
2024-08-01 16:25 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2024-07-17 08:54:26 UTC
(In reply to Tomás Cohen Arazi from comment #38)
> I'm not sure if this is a problem with the ILL requests DT only, but I have:
> 
> > SELECT COUNT(*) FROM illrequests;
> 54
> > SELECT COUNT(*) FROM illrequestattributes;
> 754
> 
> That'd be -roughly- around 13/14 different attributes per request.
> 
> Using the DT filter box for searching "fra" is generating this query:
> 
> ```sql
> SELECT COUNT( * ) FROM (SELECT `me`.`illrequest_id` FROM `illrequests` `me`
> LEFT JOIN `illrequestattributes` `extended_attributes` ON
> `extended_attributes`.`illrequest_id` = `me`.`illrequest_id` LEFT JOIN
> `illrequestattributes` `extended_attributes_2` ON
> `extended_attributes_2`.`illrequest_id` =
> `me`.`illrequest_id` LEFT JOIN `illrequestattributes`
> `extended_attributes_3` ON `extended_attributes_3`.`illrequest_id` =
> `me`.`illrequest_id` LEFT JOIN `illrequestattributes`
> `extended_attributes_4` ON `extended_attributes_4`.`illrequest_id` =
> `me`.`illrequest_id` LEFT JOIN `illrequestattributes`
> `extended_attributes_5` ON `extended_attributes_5`.`illrequest_id` =
> `me`.`illrequest_id` LEFT JOIN `illrequestattributes`
> `extended_attributes_6` ON `extended_attributes_6`.`illrequest_id` =
> `me`.`illrequest_id` LEFT JOIN `illrequestattributes`
> `extended_attributes_7` ON `extended_attributes_7`.`illrequest_id` =
> `me`.`illrequest_id` LEFT JOIN `illrequestattributes`
> `extended_attributes_8` ON `extended_attributes_8`.`illrequest_id` =
> `me`.`illrequest_id` LEFT JOIN `illbatches` `ill_batch` ON
> `ill_batch`.`ill_batch_id` = `me`.`batch_id`  JOIN `branches` `library` ON
> `library`.`branchcode` = `me`.`branchcode` LEFT JOIN `borrowers` `patron` ON
> `patron`.`borrowernumber` = `me`.`borrowernumber` WHERE ( ( ( `me`.`cost`
> LIKE '%fra%' OR `me`.`placed` LIKE '%fra%' OR `me`.`status` LIKE '%fra%' OR
> `me`.`backend` LIKE '%fra%' OR `me`.`orderid` LIKE '%fra%' OR `me`.`replied`
> LIKE '%fra%' OR `me`.`updated` LIKE '%fra%' OR `me`.`accessurl` LIKE '%fra%'
> OR `me`.`biblio_id` LIKE '%fra%' OR `me`.`completed` LIKE '%fra%' OR
> `me`.`notesopac` LIKE '%fra%' OR `me`.`notesstaff` LIKE '%fra%' OR
> `me`.`price_paid` LIKE '%fra%' OR `ill_batch`.`name` LIKE '%fra%' OR
> `me`.`illrequest_id` LIKE '%fra%' OR `library`.`branchname` LIKE '%fra%' OR
> ( `extended_attributes_8`.`type` = 'type' AND
> `extended_attributes_8`.`value` LIKE '%fra%' ) OR (
> `extended_attributes_6`.`type` = 'year' AND `extended_attributes_6`.`value`
> LIKE '%fra%' ) OR ( `extended_attributes_4`.`type` = 'issue' AND
> `extended_attributes_4`.`value` LIKE '%fra%' ) OR (
> `extended_attributes_7`.`type` = 'pages' AND `extended_attributes_7`.`value`
> LIKE '%fra%' ) OR ( `extended_attributes_2`.`type` = 'title' AND
> `extended_attributes_2`.`value` LIKE '%fra%' ) OR (
> `extended_attributes`.`type` = 'author' AND `extended_attributes`.`value`
> LIKE '%fra%' ) OR ( `extended_attributes_5`.`type` = 'volume' AND
> `extended_attributes_5`.`value` LIKE '%fra%' ) OR (
> `extended_attributes_3`.`type` = 'article_title' AND
> `extended_attributes_3`.`value` LIKE '%fra%' ) OR `patron`.`surname` LIKE
> '%fra%' OR `patron`.`firstname` LIKE '%fra%' OR `patron`.`cardnumber` LIKE
> '%fra%' ) AND `status` NOT IN ( 'COMP' ) AND ( `status_alias` NOT IN (
> 'COMP' ) OR `status_alias` IS NULL ) ) ) GROUP BY `me`.`illrequest_id`) `me`;
> ```
> 
> which kills MariaDB \o/

I can reproduce. It's bad. Taking a look.
Comment 1 Tomás Cohen Arazi 2024-07-17 13:19:51 UTC
Pedro, before I got distracted, I found some interesting food for thought:

* illrequestattributes is missing indexes on important fields like `type` and `value`. There's also missing indexes on other queried fields in other tables which could impact performance.
* I understand the need to be able to process this kind of query:

-and: [ { type: X, value: Y }, { type: W, value: Z } ]

and hence the need for the code that was added to Query.pm.

In this particular case, with only 79 requests on the DB, this is generating around 5M rows to later query on. Each different `type` is growing the size exponentially.

Have we tried adding the conditions in the ON portion of the JOIN (not sure that's possible with DBIC, though).

* This mechanism of making a JOIN per type would be useful for a form building boolean queries. But this is not the case. It seems to me that the query it is building doesn't have that structure I mentioned above. So we are paying the price, but not taking advantage of it at all.

I feel like this is a bug in the sense the code should not be generating the JOINs in this case, as the query itself doesn't match the use case. And I didn't find a way to control this behavior either.

* Looking at the generated query, I don't see how we cannot just use a single JOIN with a bit "more evolved" WHERE instead:

SELECT * FROM illrequests
LEFT     JOIN illrequestattributes
ON (illrequests.illrequest_id=illrequestattributes.illrequest_id)
WHERE (
    ( illrequestattributes.type='X' AND illrequestattributes.value LIKE '%fra%' )
    ...
 OR ( illrequestattributes.type='Z' AND illrequestattributes.value LIKE '%fra%' )
)

That would only generate rows*(count_for different_types) rows: 632 vs. 5M.

The problem here is the 5M rows with many left-hand wildcard searches on each (those wouldn't take advantage of the index).
Comment 2 Pedro Amorim 2024-07-17 13:36:00 UTC
I've been looking into this.
I've gotten around the 5M rows by using a GROUP BY and running the query directly on CLI, it's still very slow. But I agree, 5M is obviously not expected or wanted. My exercise was much lighter and it was returning 46k rows when the expected result was 3, but the point stands, by using GROUP BY only the 3 results are returned, but it still takes like 15 seconds to return the query, and that's on a database with 10 ill requests.

Some thoughts:
1) The datatable search input is only there because we moved to an ajax driven table, and that ILL table in particular has 8 extended_attribute columns, which will result in 8 left joins on the final query and rows are added to the final result exponentially.
2) The left-side filters for the ILL table work fine for the use case we want here I believe, and does the following query structure:
          OR (
            (
              `extended_attributes`.`type` = ? 
              OR `extended_attributes`.`type` = ? 
              OR `extended_attributes`.`type` = ? 
              OR `extended_attributes`.`type` = ? 
              OR `extended_attributes`.`type` = ? 
              OR `extended_attributes`.`type` = ? 
              OR `extended_attributes`.`type` = ? 
              OR `extended_attributes`.`type` = ?
            ) 
            AND `extended_attributes`.`value` LIKE ?
          )
Meaning it'll look for the occurrence of the keyword on any extended_attribute, even though it doesn't allow to filter for specific values on specific attributes.
3) My immediate suggestion for now is to disable the datatable search input and have users just use the left-side filters (and keyword search) instead, I can't think of a current use-case the left-side filters don't satisfy that the dt search does (neither allow to filter for specific values in specific attributes).
4) I've tried rewriting the way the query is built to maintain functionality and improve performance but so far I've been unsuccessful. I think this may take a while to find the proper final fix and this is why I'm suggesting the dt search input be disabled for now.
Comment 3 Pedro Amorim 2024-07-17 13:42:27 UTC
The reason I failed to catch this performance issue with my testing on the patron search in bug 30645 is because I was using the left-side filters on patron search and not the patron search datatable search input, where the query is custom-built (like the left side filters on the ILL table).

But even if using the patron datatable search input this problem does not exist there because there are not extended_attribute columns on the patron table, so the query does not contain any extended_attributes entries.
Comment 4 Martin Renvoize 2024-07-18 12:06:12 UTC
Created attachment 169130 [details] [review]
Bug 37389: Add a builder to dynamically create relations when required

This private method is intended to be called from the API query builder
to add relation accessors for later use in the search query
Comment 5 Martin Renvoize 2024-07-18 13:14:34 UTC
Created attachment 169133 [details] [review]
Bug 37389: Add AUTOLOAD to dynamically add relation accessors

This patch adds an AUTOLOAD to Koha::ILL::Request to allows fetching
extended_attributes_$type attributes via dynamically added relations
added into the underlying DBIC resultset class.
Comment 6 Pedro Amorim 2024-07-18 14:49:22 UTC
Created attachment 169140 [details] [review]
Bug 37389: PoC Follow-up

Its close but it appears the dyncamically added accessors are not working properly, or not being registered properly
Comment 7 Pedro Amorim 2024-07-18 16:23:53 UTC
Created attachment 169149 [details] [review]
Bug 37389: More WIP

Hardcoded accessors (for now)
Rewrite the API query builder to dynamically get the extended_attributes from the query

This patch already fixes the performance issue and keeps the functionality, but we're trying to generate the extended_attributes accessors dynamically instead of hardcoding them
Comment 8 Martin Renvoize 2024-07-18 17:21:36 UTC
Created attachment 169159 [details] [review]
Bug 37389: Working accessors
Comment 9 Martin Renvoize 2024-07-18 17:22:24 UTC
These paches won't apply right now as we were working at them from opposite directions.. 

Pedro and I will clean up a branch and submit as a set tomorrow
Comment 10 Martin Renvoize 2024-07-19 07:21:51 UTC
Created attachment 169178 [details] [review]
Bug 37389: Working accessors
Comment 11 Martin Renvoize 2024-07-19 09:38:45 UTC
Created attachment 169185 [details] [review]
Bug 37389: More silly fixes
Comment 12 Martin Renvoize 2024-07-19 09:54:28 UTC
Created attachment 169189 [details] [review]
Bug 37389:

This patch adds a number of methods that work together to result the
problem as a whole.

We add a builder method to dynamically create DBIx::Class relations when
required for the Koha::ILL::Requests resultset.

We update Koha::REST::Plugin::Query to call the aforementioned builder
method so we can subsequently use these new relations to create a
performance join condition. (This also required passing the ResultSet
into the dbic_extended_attributes_join helper from
Koha::REST::Plugin::Objects)

We that add an AUTOLOAD into Koha::ILL::Request to allow use of the
new dynamically added DBIx::Class relations in the above as
extended_attributes_$type from the corresponding Koha::Object.

Finally, we reworked the query builder code to ensure we're using the
new relations added when using the API.
Comment 13 Martin Renvoize 2024-07-19 09:55:43 UTC
Created attachment 169191 [details] [review]
Bug 37389: Fix performance problems with extended attributes on the API

This patch adds a number of methods that work together to result the
problem as a whole.

We add a builder method to dynamically create DBIx::Class relations when
required for the Koha::ILL::Requests resultset.

We update Koha::REST::Plugin::Query to call the aforementioned builder
method so we can subsequently use these new relations to create a
performance join condition. (This also required passing the ResultSet
into the dbic_extended_attributes_join helper from
Koha::REST::Plugin::Objects)

We that add an AUTOLOAD into Koha::ILL::Request to allow use of the
new dynamically added DBIx::Class relations in the above as
extended_attributes_$type from the corresponding Koha::Object.

Finally, we reworked the query builder code to ensure we're using the
new relations added when using the API.
Comment 14 Martin Renvoize 2024-07-19 09:57:56 UTC
Created attachment 169192 [details] [review]
Bug 37389: Fix performance problems with extended attributes on the API

This patch adds a number of methods that work together to result the
problem as a whole.

We add a builder method to dynamically create DBIx::Class relations when
required for the Koha::ILL::Requests resultset.

We update Koha::REST::Plugin::Query to call the aforementioned builder
method so we can subsequently use these new relations to create a
performance join condition. (This also required passing the ResultSet
into the dbic_extended_attributes_join helper from
Koha::REST::Plugin::Objects)

We that add an AUTOLOAD into Koha::ILL::Request to allow use of the
new dynamically added DBIx::Class relations in the above as
extended_attributes_$type from the corresponding Koha::Object.

Finally, we reworked the query builder code to ensure we're using the
new relations added when using the API.

Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Pedro Amorim 2024-07-19 14:35:35 UTC
Created attachment 169234 [details] [review]
Bug 37389: [SQUASH] Join alias fix

This is to be squashed into the previous patch, but I'm leaving an explanation here of this follow-up fix for now.
Only rename the join aliases if extended_attribute entry is a string 'type'.
If its an array, skip.
This 'array' method is used on the left-side search forms for ILL and patrons table.
Its the difference between

      [
        {
          "extended_attributes.type": [
            "title",
            "type",
            "author",
            "article_title",
            "pages",
            "issue",
            "volume",
            "year"
          ],
          "extended_attributes.value": {
            "like": "%book%"
          }
        }
      ]

and

[
    {
      "extended_attributes.type": "year",
      "extended_attributes.value": {
        "like": "%book%"
      }
    }
  ],
  [
    {
      "extended_attributes.type": "pages",
      "extended_attributes.value": {
        "like": "%book%"
      }
    }
  ],

Where the former searches for a single keyword occurrence in a potential list of extended_attributes and the latter allows for different keywords for each extended_attribute type (even though the datatable search input allows only for 1, this is utilized in other places such as ERM licenses additional fields and subscriptions searchable additional fields I believe).
Comment 16 Pedro Amorim 2024-07-19 14:38:34 UTC
We've gone a long way, but there's still work to be done:
1) Update tests and possible write new ones
2) Possibly clean-up the query a bit, there are extraneous WHERE conditions that are already on the LEFT JOIN even though this is not causing any issue that I'm aware of atm
3) Abstract this into a new ExtendedAttributes Mixin to be attached to Patron.pm, ILL::Request and Mixin/AdditionalFields, so this is applied to all supporting classes of extended_attributes
Comment 17 Pedro Amorim 2024-07-19 16:02:49 UTC
Created attachment 169243 [details] [review]
Bug 37389: _build_extended_attributes_relations update

Move this method to the new ExtendedAttributes mixin
Abstract this method and apply to ILL::Requests
Comment 18 Pedro Amorim 2024-07-19 16:02:51 UTC
Created attachment 169244 [details] [review]
Bug 37389: Move AUTOLOAD out of ILL::Request into Mixin

This AUTOLOAD is not being used, but we're keeping this as also being a result of this work if we ever want to support the following syntax in the code:
ill_request->extended_attributes_author; # and get the 'author' extended_attribute for the ill_request
Comment 19 Pedro Amorim 2024-07-19 16:02:54 UTC
Created attachment 169245 [details] [review]
Bug 37389: Apply ExtendedAttributes Mixin to Patrons

This restores the extended_attributes search query functionality as
before, but with the new performance improvements
Comment 20 Pedro Amorim 2024-07-19 16:02:57 UTC
Created attachment 169246 [details] [review]
Bug 37389: [WIP] Update tests

This is not done yet, I've only updated it to have the tests pass, but
we need more.
Comment 21 David Cook 2024-07-22 06:07:02 UTC
Seems interesting! 

It would be good if we could try to inject some malicious code in here. I'm not 100% sure how this all hangs together at the moment, but that would be my only concern overall.
Comment 22 Pedro Amorim 2024-07-22 16:20:23 UTC
Created attachment 169317 [details] [review]
Bug 37389: [WIP] Update tests

This is not done yet, I've only updated it to have the tests pass, but
we need more.
Comment 23 Pedro Amorim 2024-07-22 16:20:26 UTC
Created attachment 169318 [details] [review]
Bug 37389: Apply ExtendedAttributes Mixin to AdditionalFields

This is only applied to subscription for now. More are missing
Comment 24 Pedro Amorim 2024-07-25 15:39:11 UTC
Created attachment 169569 [details] [review]
Bug 37389: Add tests

prove t/Koha/REST/Plugin/Query.t
prove t/db_dependent/Koha/Objects/Mixin/ExtendedAttributes.t

Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Pedro Amorim 2024-07-25 15:39:14 UTC
Created attachment 169570 [details] [review]
Bug 37389: ExtendedAttributes mixin

This is what we're doing here:

- Creating a new mixin called ExtendedAttributes.pm
- 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' is generated dynamically when a search with 'prefetch'=>'extended_attributes' is performed.
-- Rewrites the 'join' entries in the query to have the alias as above.
-- Rewrites the WHERE conditions to match the above ruleset.

Example:

A DBIX search query as follow:

[
        {
            '-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 <martin.renvoize@ptfs-europe.com>
Comment 26 Pedro Amorim 2024-07-25 15:45:52 UTC
Hi guys, this is ready for review+testing.

@Tomas @Liz are you available to test this with for the ILL use cases you were having issues with?

Quite a lot of work went into this but the end code was actually not that extensive so I opted to squash and simplify the commit message to the best of my ability.

I'm postponing applying this ExtendedAttributes Mixin to AdditionalFields classes for the following reasons:
1) No AdditionalFields classes use the extended_attributes as of yet (no performance issues there)
2) The key part that was missing here is actually already present in the additional_field_values accessors of the supporting classes, so no performance issue would happen there anyway
3) I'm considering this to be in the scope of bug 35287 as ERM Licenses will be the first AdditionalFields class to support the extended_attributes accessor, so I'm planning this work for when I rebase that.
Comment 27 Pedro Amorim 2024-07-25 15:53:18 UTC
Created attachment 169583 [details] [review]
Bug 37389: Add tests

prove t/Koha/REST/Plugin/Query.t
prove t/db_dependent/Koha/Objects/Mixin/ExtendedAttributes.t

Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Pedro Amorim 2024-07-25 15:53:21 UTC
Created attachment 169584 [details] [review]
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' is generated dynamically when a search with 'prefetch'=>'extended_attributes' is performed.
-- Rewrites the 'join' entries in the query to have the alias as above.
-- Rewrites the WHERE conditions to match the above ruleset.

Example:

A DBIX search query as follow:

[
        {
            '-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 <martin.renvoize@ptfs-europe.com>
Comment 29 Pedro Amorim 2024-07-25 15:56:44 UTC
Created attachment 169585 [details] [review]
Bug 37389: Add tests

prove t/Koha/REST/Plugin/Query.t
prove t/db_dependent/Koha/Objects/Mixin/ExtendedAttributes.t

Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Pedro Amorim 2024-07-25 15:56:47 UTC
Created attachment 169586 [details] [review]
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 <martin.renvoize@ptfs-europe.com>
Comment 31 Pedro Amorim 2024-07-26 14:34:09 UTC
(In reply to Pedro Amorim from comment #26)
> 3) I'm considering this to be in the scope of bug 35287 as ERM Licenses will
> be the first AdditionalFields class to support the extended_attributes
> accessor, so I'm planning this work for when I rebase that.

The patch this comment is about is at:
https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=35287&attachment=169737
Comment 32 Pedro Amorim 2024-07-26 14:40:44 UTC
I've updated the bug summary to better reflect the issue and scope of things here.
Comment 33 David Nind 2024-07-29 23:18:36 UTC
Created attachment 169850 [details] [review]
Bug 37389: Add tests

prove t/Koha/REST/Plugin/Query.t
prove t/db_dependent/Koha/Objects/Mixin/ExtendedAttributes.t

Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 34 David Nind 2024-07-29 23:18:39 UTC
Created attachment 169851 [details] [review]
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 <martin.renvoize@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 35 David Nind 2024-07-29 23:40:34 UTC
Testing notes (using KTD):

1. I ran the tests after applying the patches - these pass:
   . prove t/Koha/REST/Plugin/Query.t
   . prove t/db_dependent/Koha/Objects/Mixin/ExtendedAttributes.t

2. I don't have any knowledge in this area, so if comment is required on the "mixin" and approach taken, please change the bug status back to needs sign off.

3. I've had a go at a release note, but I'm not sure that I've described it correctly (and I'm not sure what a "mixin" is 8-)).
Comment 36 Tomás Cohen Arazi 2024-08-01 12:28:29 UTC
Created attachment 169944 [details] [review]
Bug 37389: Add tests

prove t/Koha/REST/Plugin/Query.t
prove t/db_dependent/Koha/Objects/Mixin/ExtendedAttributes.t

Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 37 Tomás Cohen Arazi 2024-08-01 12:28:32 UTC
Created attachment 169945 [details] [review]
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 <martin.renvoize@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 38 Tomás Cohen Arazi 2024-08-01 12:28:36 UTC
Created attachment 169946 [details] [review]
Bug 37389: (QA follow-up) Tidy

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 39 Tomás Cohen Arazi 2024-08-01 12:43:27 UTC
I tested with the original test DB I had, and the results are excellent. Great job Pedro!
Comment 40 Katrin Fischer 2024-08-01 16:05:07 UTC
I am sorry, but this no longer applies. I fixed the first conflict in the test file, but then got stuck here:


kohadev-koha@kohadevbox:koha(main|AM 1/1)$ git bz apply --continue
Applying: Bug 37389: Add tests
Applying: Bug 37389: ExtendedAttributes mixin
Using index info to reconstruct a base tree...
M	Koha/REST/Plugin/Objects.pm
M	Koha/REST/Plugin/Query.pm
Falling back to patching base and 3-way merge...
Auto-merging Koha/REST/Plugin/Query.pm
CONFLICT (content): Merge conflict in Koha/REST/Plugin/Query.pm
Auto-merging Koha/REST/Plugin/Objects.pm
CONFLICT (content): Merge conflict in Koha/REST/Plugin/Objects.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 37389: ExtendedAttributes mixin
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-37389-ExtendedAttributes-mixin-iims1jsb.patch
Comment 41 Pedro Amorim 2024-08-01 16:25:28 UTC
Created attachment 169954 [details] [review]
Bug 37389: Add tests

prove t/Koha/REST/Plugin/Query.t
prove t/db_dependent/Koha/Objects/Mixin/ExtendedAttributes.t

Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 42 Pedro Amorim 2024-08-01 16:25:32 UTC
Created attachment 169955 [details] [review]
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 <martin.renvoize@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 43 Pedro Amorim 2024-08-01 16:25:35 UTC
Created attachment 169956 [details] [review]
Bug 37389: (QA follow-up) Tidy

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 44 Katrin Fischer 2024-08-01 16:32:18 UTC
Can you please rebase? I got past the conflict with the test, but then got stuck on the second patch.
Comment 45 Katrin Fischer 2024-08-01 16:36:17 UTC
(In reply to Katrin Fischer from comment #44)
> Can you please rebase? I got past the conflict with the test, but then got
> stuck on the second patch.

OH. Pedro, you were too fast for me. Back to this tomorrow!
Comment 46 Katrin Fischer 2024-08-02 17:01:37 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 47 Lucas Gass 2024-08-29 17:43:49 UTC
Backported to 24.05.x for upcoming 24.05.04