Skip to content

Conversation

@kai687
Copy link
Contributor

@kai687 kai687 commented Oct 3, 2024

🧭 What and Why

Apply the query parameters in _session.request.

The Python transporter didn't apply query parameters.
Methods with options passed as query parameters, such as partial_update_object with create_if_not_exists didn't work correctly.

Fixes: algolia/algoliasearch-client-python#572

@algolia-bot
Copy link
Collaborator

algolia-bot commented Oct 3, 2024

🪓 The generated code will be pushed at the end of the CI.

Action triggered by commit 816e58c68200f87dd752ff9013f794c4ce1d4f76.

Please do not push any generated code to this pull request.

@kai687 kai687 changed the title fix: pass query parameters to request fix(python): pass query parameters to request Oct 3, 2024
@kai687 kai687 assigned kai687 and unassigned kai687 Oct 3, 2024
@kai687 kai687 marked this pull request as ready for review October 3, 2024 16:47
@kai687 kai687 requested a review from a team as a code owner October 3, 2024 16:47
@kai687 kai687 requested review from millotp and shortcuts October 3, 2024 16:47
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the python client is working properly, query params are already part of the path, and they are working properly, we have e2e tests for that, maybe the client is running an older version ?

I tried it myself in the playground and it works as expected, with False or True

@kai687
Copy link
Contributor Author

kai687 commented Oct 4, 2024

Maybe I didn't look carefully enough, but based on my analysis, no option that should be a query parameter in the API gets properly forwarded with the Python client. This includes create_if_not_exists and forward_to_replicas (for the rules or synonyms methods).

I manually tested save_synonym and partial_update_object, and I can see that no query param is sent with the request in the Algolia dashboard.

What I found was:

  1. In partial_update_object_with_http_info, we add the create_if_not_exists option to the request_options dictionary as query param.

  2. In the transporter.request() method, the query params are not forwarded, only headers and request bodies are extracted from the request_options dictionary.

I tested this fix manually, and it correctly applies the query params.
The only things I couldn't figure out yet are the errors in the CI.

@millotp
Copy link
Collaborator

millotp commented Oct 7, 2024

This test asserts that query params are correctly passed thanks to this, we encode the params ourself instead of relying on the aiohttp because we do custom stuff for arrays and whitespaces

@millotp
Copy link
Collaborator

millotp commented Oct 7, 2024

maybe there is something broken for the search client.
Did you try with the sync or async client ?

@kai687
Copy link
Contributor Author

kai687 commented Oct 7, 2024

@millotp Thanks! I indeed missed that. I found the real issue, but it's already fixed :)

In version 4.5.0, BaseTransporter.prepare() would return None when use_read_transporter is false. So, here was where the query parameters disappeared for endpoints like 'save synonyms' or 'partial update' since they weren't GET requests.

Luckily, 6b42a26 already fixed this, so it shouldn't be an issue in the next release. I'll close this PR.

@kai687 kai687 closed this Oct 7, 2024
@kai687 kai687 deleted the fix/python-pass-query-params branch October 7, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug]: create_if_not_exists param in partial_update_object is ignored

4 participants