[Bug 37389] New: ILL requests table hangs when searching
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Bug ID: 37389 Summary: ILL requests table hangs when searching Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: ILL Assignee: pedro.amorim@ptfs-europe.com Reporter: pedro.amorim@ptfs-europe.com QA Contact: testopia@bugs.koha-community.org CC: liz@bywatersolutions.com, martin.renvoize@ptfs-europe.com, pedro.amorim@ptfs-europe.com, tomascohen@gmail.com Depends on: 30645 (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. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30645 [Bug 30645] Generated DBIC query incorrect for API searches across joined extended attributes when several terms are passed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com, | |nick@bywatersolutions.com --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #2 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #3 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #4 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 169130 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169130&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #5 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 169133 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169133&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #6 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169140 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169140&action=edit Bug 37389: PoC Follow-up Its close but it appears the dyncamically added accessors are not working properly, or not being registered properly -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #7 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169149 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169149&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #8 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 169159 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169159&action=edit Bug 37389: Working accessors -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169159|0 |1 is obsolete| | --- Comment #10 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 169178 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169178&action=edit Bug 37389: Working accessors -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 169185 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169185&action=edit Bug 37389: More silly fixes -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #12 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 169189 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169189&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 169191 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169191&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169130|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169133|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169140|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169149|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169178|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169185|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169189|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169191|0 |1 is obsolete| | --- Comment #14 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 169192 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169192&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #15 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169234 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169234&action=edit 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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #16 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #17 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169243 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169243&action=edit Bug 37389: _build_extended_attributes_relations update Move this method to the new ExtendedAttributes mixin Abstract this method and apply to ILL::Requests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #18 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169244 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169244&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #19 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169245 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169245&action=edit Bug 37389: Apply ExtendedAttributes Mixin to Patrons This restores the extended_attributes search query functionality as before, but with the new performance improvements -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #20 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169246 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169246&action=edit Bug 37389: [WIP] Update tests This is not done yet, I've only updated it to have the tests pass, but we need more. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #21 from David Cook <dcook@prosentient.com.au> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169246|0 |1 is obsolete| | --- Comment #22 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169317 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169317&action=edit Bug 37389: [WIP] Update tests This is not done yet, I've only updated it to have the tests pass, but we need more. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #23 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169318 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169318&action=edit Bug 37389: Apply ExtendedAttributes Mixin to AdditionalFields This is only applied to subscription for now. More are missing -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169192|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169234|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169243|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169244|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169245|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169317|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169318|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #24 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169569 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169569&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #25 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169570 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169570&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #26 from Pedro Amorim <pedro.amorim@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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169569|0 |1 is obsolete| | --- Comment #27 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169583 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169583&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169570|0 |1 is obsolete| | --- Comment #28 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169584 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169584&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169583|0 |1 is obsolete| | --- Comment #29 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169585 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169585&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169584|0 |1 is obsolete| | --- Comment #30 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169586 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169586&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=37180 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |35287 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35287 [Bug 35287] Add additional fields support to ERM Licenses -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #31 from Pedro Amorim <pedro.amorim@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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|ILL requests table hangs |REST API queries joining on |when searching |extended_attributes may | |cause severe performance | |issues --- Comment #32 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- I've updated the bug summary to better reflect the issue and scope of things here. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169585|0 |1 is obsolete| | --- Comment #33 from David Nind <david@davidnind.com> --- Created attachment 169850 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169850&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169586|0 |1 is obsolete| | --- Comment #34 from David Nind <david@davidnind.com> --- Created attachment 169851 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169851&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |This fixes a severe release notes| |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. CC| |david@davidnind.com --- Comment #35 from 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-)). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |tomascohen@gmail.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169850|0 |1 is obsolete| | Attachment #169851|0 |1 is obsolete| | --- Comment #36 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 169944 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169944&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #37 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 169945 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169945&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #38 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 169946 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169946&action=edit Bug 37389: (QA follow-up) Tidy Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #39 from Tomás Cohen Arazi <tomascohen@gmail.com> --- I tested with the original test DB I had, and the results are excellent. Great job Pedro! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Patch doesn't apply --- Comment #40 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169944|0 |1 is obsolete| | --- Comment #41 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169954 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169954&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169945|0 |1 is obsolete| | --- Comment #42 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169955 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169955&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169946|0 |1 is obsolete| | --- Comment #43 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 169956 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169956&action=edit Bug 37389: (QA follow-up) Tidy Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Patch doesn't apply --- Comment #44 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Can you please rebase? I got past the conflict with the test, but then got stuck on the second patch. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Passed QA --- Comment #45 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |24.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 --- Comment #46 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.11! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Pushed to stable Version(s)|24.11.00 |24.11.00,24.05.04 released in| | CC| |lucas@bywatersolutions.com --- Comment #47 from Lucas Gass <lucas@bywatersolutions.com> --- Backported to 24.05.x for upcoming 24.05.04 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Status|Pushed to stable |Needs documenting --- Comment #48 from Fridolin Somers <fridolin.somers@biblibre.com> --- Depends on Bug 30645 not in 23.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 Bug 37389 depends on bug 30645, which changed state. Bug 30645 Summary: Generated DBIC query incorrect for API searches across joined extended attributes when several terms are passed https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30645 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37389 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Needs documenting |RESOLVED --- Comment #49 from David Nind <david@davidnind.com> --- The API is not covered in the manual, no user interface changes, so no changes required to the manual. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org