[Bug 33971] New: Remove support for x-koha-query header
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 Bug ID: 33971 Summary: Remove support for x-koha-query header Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: REST API Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com CC: tomascohen@gmail.com On bug 32401 we removed the use of the x-koha-query header on the DataTables wrapper. The feature remains in the codebase, and I propose we remove it. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |32401 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32401 [Bug 32401] x-koha-query cannot contain non-ISO-8859-1 values -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com, | |kyle@bywatersolutions.com, | |m.de.rooy@rijksmuseum.nl, | |martin.renvoize@ptfs-europe | |.com, | |nick@bywatersolutions.com, | |pedro.amorim@ptfs-europe.co | |m -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |In Discussion --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Marcel has expressed his preference for keeping this header, but some transition would need to take place to allow support for UTF-8 there as well. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 --- Comment #2 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- I am voting to remove it. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #3 from David Cook <dcook@prosentient.com.au> --- +1 for removing it -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 --- Comment #4 from Kyle M Hall <kyle@bywatersolutions.com> --- +1 for removal -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Status|In Discussion |ASSIGNED -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |33996 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33996 [Bug 33996] Authority objects missing mapping -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 152302 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152302&action=edit Bug 33971: Remove x-koha-query usage in biblios.t This patch removes handling of x-koha-query from the tests. To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/biblios.t => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! Change from x-koha-header to q= gives same results. 4. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 --- Comment #6 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 152303 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152303&action=edit Bug 33971: Remove x-koha-query handing in Orders.pm This patch removes manual handling of `x-koha-query` in the Orders:list() controller. There are no tests for the feature but you can verify no behavior is changed by doing: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/acquisitions_orders.t => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests pass 4. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 --- Comment #7 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 152304 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152304&action=edit Bug 33971: Remove x-koha-query header support This patch removes support for the `x-koha-query` header entirely from the codebase. For the purpose: - $c->objects->search is adjusted - Relevant tests are cleared/adapted - The OpenAPI spec gets the header removed To test: 1. Apply this patch and run: $ ktd --shell k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t \ t/Koha/REST/Plugin/Query.t \ t/db_dependent/api/v1/*.t => SUCCESS: Tests pass! 2. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 --- Comment #8 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- https://git.koha-community.org/Koha-community/Koha/src/branch/master/api/v1/... will also need cutting out ;-). Generally this all looks good to me. What were Marcel's reasons for wanting to keep it as an option? I've used it in the past I believe, and it seemed to work nicely, but I'm not against dropping it.. one less code path to maintain. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Martin Renvoize from comment #8)
https://git.koha-community.org/Koha-community/Koha/src/branch/master/api/v1/ swagger/swagger.yaml#L714 will also need cutting out ;-).
Generally this all looks good to me. What were Marcel's reasons for wanting to keep it as an option? I've used it in the past I believe, and it seemed to work nicely, but I'm not against dropping it.. one less code path to maintain.
See bug 32406 comment5 and bug 32401 comment9. In short, search terms in URL could be a privacy issue. Not allowing searching for UTF-8 in a multilingual system would be rather poor. But no blocker :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Marcel de Rooy from comment #9)
(In reply to Martin Renvoize from comment #8)
https://git.koha-community.org/Koha-community/Koha/src/branch/master/api/v1/ swagger/swagger.yaml#L714 will also need cutting out ;-).
Generally this all looks good to me. What were Marcel's reasons for wanting to keep it as an option? I've used it in the past I believe, and it seemed to work nicely, but I'm not against dropping it.. one less code path to maintain.
See bug 32406 comment5 and bug 32401 comment9. In short, search terms in URL could be a privacy issue. Not allowing searching for UTF-8 in a multilingual system would be rather poor. But no blocker :)
I think we should remove it, and if someone wants to implement it right, just do it properly on a new bug, in which we discuss the implementation details (e.g. requiring base64 encoding, documentation, etc). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 --- Comment #11 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- What about the occurrence in api/v1/swagger/swagger.yaml? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152304|0 |1 is obsolete| | --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 152422 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152422&action=edit Bug 33971: Remove x-koha-query header support This patch removes support for the `x-koha-query` header entirely from the codebase. For the purpose: - $c->objects->search is adjusted - Relevant tests are cleared/adapted - The OpenAPI spec gets the header removed To test: 1. Apply this patch and run: $ ktd --shell k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t \ t/Koha/REST/Plugin/Query.t \ t/db_dependent/api/v1/*.t => SUCCESS: Tests pass! 2. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 --- Comment #13 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #11)
What about the occurrence in api/v1/swagger/swagger.yaml?
Done! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 Jonathan Druart <jonathan.druart+koha@gmail.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=33971 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152302|0 |1 is obsolete| | Attachment #152303|0 |1 is obsolete| | Attachment #152422|0 |1 is obsolete| | --- Comment #14 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 152429 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152429&action=edit Bug 33971: Remove x-koha-query usage in biblios.t This patch removes handling of x-koha-query from the tests. To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/biblios.t => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! Change from x-koha-header to q= gives same results. 4. Sign off :-D Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 --- Comment #15 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 152430 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152430&action=edit Bug 33971: Remove x-koha-query handing in Orders.pm This patch removes manual handling of `x-koha-query` in the Orders:list() controller. There are no tests for the feature but you can verify no behavior is changed by doing: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/acquisitions_orders.t => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests pass 4. Sign off :-D Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 --- Comment #16 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 152431 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152431&action=edit Bug 33971: Remove x-koha-query header support This patch removes support for the `x-koha-query` header entirely from the codebase. For the purpose: - $c->objects->search is adjusted - Relevant tests are cleared/adapted - The OpenAPI spec gets the header removed To test: 1. Apply this patch and run: $ ktd --shell k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t \ t/Koha/REST/Plugin/Query.t \ t/db_dependent/api/v1/*.t => SUCCESS: Tests pass! 2. Sign off :-D Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152429|0 |1 is obsolete| | --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 152690 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152690&action=edit Bug 33971: Remove x-koha-query usage in biblios.t This patch removes handling of x-koha-query from the tests. To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/biblios.t => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! Change from x-koha-header to q= gives same results. 4. Sign off :-D Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-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=33971 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152430|0 |1 is obsolete| | --- Comment #18 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 152691 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152691&action=edit Bug 33971: Remove x-koha-query handing in Orders.pm This patch removes manual handling of `x-koha-query` in the Orders:list() controller. There are no tests for the feature but you can verify no behavior is changed by doing: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/acquisitions_orders.t => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests pass 4. Sign off :-D Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-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=33971 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152431|0 |1 is obsolete| | --- Comment #19 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 152692 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152692&action=edit Bug 33971: Remove x-koha-query header support This patch removes support for the `x-koha-query` header entirely from the codebase. For the purpose: - $c->objects->search is adjusted - Relevant tests are cleared/adapted - The OpenAPI spec gets the header removed To test: 1. Apply this patch and run: $ ktd --shell k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t \ t/Koha/REST/Plugin/Query.t \ t/db_dependent/api/v1/*.t => SUCCESS: Tests pass! 2. Sign off :-D Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-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=33971 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA QA Contact| |martin.renvoize@ptfs-europe | |.com --- Comment #20 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- All working, tests all still passing.. I'm happy with this. Passing QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |23.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=33971 --- Comment #21 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Needs documenting --- Comment #22 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- This is a breaking change.. no backport for 23.05.x series. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33971 Bug 33971 depends on bug 33996, which changed state. Bug 33996 Summary: Authority objects missing mapping https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33996 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=33971 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Text to go in the| |This patch removes support release notes| |for the `x-koha-query` HTTP | |header. The implementation | |was problematic because no | |URL/Base64 encoding was | |being expected, and it | |broke things. As it didn't | |have real usage in the | |codebase, we decided to | |remove it. | | | |It could | |eventually be restored if | |there was interest on it, | |but it wouldn't work as | |before anyway, because of | |the aforementioned issue | |with non-ASCII queries. Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org