Bug 33971 - Remove support for x-koha-query header
Summary: Remove support for x-koha-query header
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 32401 33996
Blocks:
  Show dependency treegraph
 
Reported: 2023-06-09 12:53 UTC by Tomás Cohen Arazi
Modified: 2023-07-25 17:32 UTC (History)
8 users (show)

See Also:
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


Attachments
Bug 33971: Remove x-koha-query usage in biblios.t (4.07 KB, patch)
2023-06-13 14:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33971: Remove x-koha-query handing in Orders.pm (1.66 KB, patch)
2023-06-13 14:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33971: Remove x-koha-query header support (35.92 KB, patch)
2023-06-13 14:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33971: Remove x-koha-query header support (36.23 KB, patch)
2023-06-16 14:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33971: Remove x-koha-query usage in biblios.t (4.14 KB, patch)
2023-06-16 15:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33971: Remove x-koha-query handing in Orders.pm (1.73 KB, patch)
2023-06-16 15:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33971: Remove x-koha-query header support (36.30 KB, patch)
2023-06-16 15:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33971: Remove x-koha-query usage in biblios.t (4.20 KB, patch)
2023-06-26 14:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33971: Remove x-koha-query handing in Orders.pm (1.79 KB, patch)
2023-06-26 14:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33971: Remove x-koha-query header support (36.28 KB, patch)
2023-06-26 14:35 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2023-06-09 12:53:08 UTC
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.
Comment 1 Tomás Cohen Arazi 2023-06-09 13:03:31 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.
Comment 2 Jonathan Druart 2023-06-09 13:15:43 UTC
I am voting to remove it.
Comment 3 David Cook 2023-06-12 23:46:05 UTC
+1 for removing it
Comment 4 Kyle M Hall 2023-06-13 11:31:46 UTC
+1 for removal
Comment 5 Tomás Cohen Arazi 2023-06-13 14:40:34 UTC
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
Comment 6 Tomás Cohen Arazi 2023-06-13 14:40:37 UTC
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
Comment 7 Tomás Cohen Arazi 2023-06-13 14:40:40 UTC
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
Comment 8 Martin Renvoize 2023-06-14 05:30:19 UTC
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.
Comment 9 Marcel de Rooy 2023-06-14 11:18:00 UTC
(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 :)
Comment 10 Tomás Cohen Arazi 2023-06-16 13:28:44 UTC
(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).
Comment 11 Jonathan Druart 2023-06-16 14:15:13 UTC
What about the occurrence in api/v1/swagger/swagger.yaml?
Comment 12 Tomás Cohen Arazi 2023-06-16 14:58:39 UTC
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
Comment 13 Tomás Cohen Arazi 2023-06-16 14:59:56 UTC
(In reply to Jonathan Druart from comment #11)
> What about the occurrence in api/v1/swagger/swagger.yaml?

Done!
Comment 14 Jonathan Druart 2023-06-16 15:37:34 UTC
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>
Comment 15 Jonathan Druart 2023-06-16 15:37:37 UTC
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>
Comment 16 Jonathan Druart 2023-06-16 15:37:40 UTC
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>
Comment 17 Martin Renvoize 2023-06-26 14:35:29 UTC
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>
Comment 18 Martin Renvoize 2023-06-26 14:35:32 UTC
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>
Comment 19 Martin Renvoize 2023-06-26 14:35:35 UTC
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>
Comment 20 Martin Renvoize 2023-06-26 14:36:02 UTC
All working, tests all still passing.. I'm happy with this.

Passing QA
Comment 21 Tomás Cohen Arazi 2023-07-03 13:34:44 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 22 Martin Renvoize 2023-07-17 14:52:25 UTC
This is a breaking change.. no backport for 23.05.x series.