Description
Pedro Amorim
2024-07-17 08:54:26 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). 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. 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. 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 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. 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 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 Created attachment 169159 [details] [review] Bug 37389: Working accessors 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 Created attachment 169178 [details] [review] Bug 37389: Working accessors Created attachment 169185 [details] [review] Bug 37389: More silly fixes 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. 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. 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> 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). 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 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 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 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 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. 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. 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. Created attachment 169318 [details] [review] Bug 37389: Apply ExtendedAttributes Mixin to AdditionalFields This is only applied to subscription for now. More are missing 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> 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> 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. 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> 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> 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> 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> (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 I've updated the bug summary to better reflect the issue and scope of things here. 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> 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> 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-)). 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> 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> Created attachment 169946 [details] [review] Bug 37389: (QA follow-up) Tidy Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> I tested with the original test DB I had, and the results are excellent. Great job Pedro! 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 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> 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> Created attachment 169956 [details] [review] Bug 37389: (QA follow-up) Tidy Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Can you please rebase? I got past the conflict with the test, but then got stuck on the second patch. (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! Pushed for 24.11! Well done everyone, thank you! Backported to 24.05.x for upcoming 24.05.04 |