Summary: | Remove support for x-koha-query header | ||
---|---|---|---|
Product: | Koha | Reporter: | Tomás Cohen Arazi <tomascohen> |
Component: | REST API | Assignee: | Tomás Cohen Arazi <tomascohen> |
Status: | RESOLVED FIXED | QA Contact: | Martin Renvoize <martin.renvoize> |
Severity: | normal | ||
Priority: | P5 - low | CC: | dcook, jonathan.druart, kyle, m.de.rooy, martin.renvoize, nick, pedro.amorim, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: |
This patch removes support 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.
|
Version(s) released in: |
23.11.00
|
Bug Depends on: | 32401, 33996 | ||
Bug Blocks: | |||
Attachments: |
Bug 33971: Remove x-koha-query usage in biblios.t
Bug 33971: Remove x-koha-query handing in Orders.pm Bug 33971: Remove x-koha-query header support Bug 33971: Remove x-koha-query header support Bug 33971: Remove x-koha-query usage in biblios.t Bug 33971: Remove x-koha-query handing in Orders.pm Bug 33971: Remove x-koha-query header support Bug 33971: Remove x-koha-query usage in biblios.t Bug 33971: Remove x-koha-query handing in Orders.pm Bug 33971: Remove x-koha-query header support |
Description
Tomás Cohen Arazi
2023-06-09 12:53:08 UTC
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. I am voting to remove it. +1 for removing it +1 for removal Created attachment 152302 [details] [review] 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 Created attachment 152303 [details] [review] 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 Created attachment 152304 [details] [review] 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 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. (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 :) (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). What about the occurrence in api/v1/swagger/swagger.yaml? Created attachment 152422 [details] [review] 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 (In reply to Jonathan Druart from comment #11) > What about the occurrence in api/v1/swagger/swagger.yaml? Done! Created attachment 152429 [details] [review] 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> Created attachment 152430 [details] [review] 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> Created attachment 152431 [details] [review] 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> Created attachment 152690 [details] [review] 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> Created attachment 152691 [details] [review] 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> Created attachment 152692 [details] [review] 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> All working, tests all still passing.. I'm happy with this. Passing QA Pushed to master for 23.11. Nice work everyone, thanks! This is a breaking change.. no backport for 23.05.x series. |