Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add option to pass headers in the meilisearch client builder #721

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wandyou
Copy link

@wandyou wandyou commented Jan 31, 2025

Pull Request

Related issue

Fixes #720

What does this PR do?

  • Fixes the issue by introducing a new option in the meilisearch client builder.
  • Passes it down to the http client
  • Merges the headers passed in the option with the other headers already configured (ex: bearer)

Improvement

  • This solution is not the cleanest because if forces us to null all the optional parameters before it (if we don't use them).
    For example : new Meilisearch(MEILISEARCH_HOST, MEILISEARCH_SEARCH_KEY, null, null, [], null, ["X-MS-USER-ID" => example]) => But it was the fastest solution and the less destructive.
  • An other way would be to create a method in the Meilisearch Client object addHeader()
  • An other way would be to pass all Meilisearch Client options in the form of an array, allowing us to only define used values in any order we like.

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Extra info

First contribution of mine to this repo, maybe I missed some subtilities.

@norkunas
Copy link
Collaborator

You can pass a http client configured with your headers, as it's a http client responsibility 🤔

@wandyou
Copy link
Author

wandyou commented Feb 4, 2025

I get it, and that's how I am solving this issue right now.

But the X-MS-USER-ID header is specific to meilisearch's analytics and I find it more appealing and "light" to just be able to pass my unique user identifier while calling new MeilisearchClient(), then passing an instance of a full http client, add configuration options, to "just" give an Analytics header.

In fact in some cases, I would like to be able to change the user-id passed in the header on the go. Maybe with a $client->setHeader()or $client->setUserId(). Because I want to perform a search while being the user and an other while being the server. And having to instanciate two meilisearch clients seems overkill.

It is obvious to me that few only projects will need to pass a "custom" http client to the MeilisearchClient, but all of them would like to take advantage of precise analytics. And therefore pass a "X-MS-USER-ID". So I find it interesting to make that task more "accessible".

Maybe the right thing to do is not to touch the headers upfront, but to expose a method like setAnalyticsUserId()and under the hood it sets the header.

I know analytics is in beta for now, so all of this is maybe new and not much used. But as a developer, not needing to customise my http client. I would prefer having a simple way to pass this info. I did not find any Analytics methods in the php doc neither. For example to report a clic on a search hit, or a conversion.

Maybe I am missing a key concept here and not seeing the big picture, and maybe I am not expressing myself clearly enough because english is not my native language. So obviously feel free to jump in and explain why it would not be the way to go.

Thanks

@wandyou
Copy link
Author

wandyou commented Feb 4, 2025

I may have not been clear enough on the fact that I want to simplify the implementation of X-MS-USER-ID because in my opinion, analytics is a mandatory option for most use cases. And most devs will want to pass it, so why not make it dev friendly ?

Obviously if I want to configure and use custom headers, the best practice would be to pass a custom http client.

I get that adding an optional headers array, whilst seeming the easiest implementation on my end, is not the best suited for the long run.

Maybe a method like setAnalyticsUserId()on the client would ? Or any other suggestions ?

@norkunas
Copy link
Collaborator

norkunas commented Feb 4, 2025

And having to instanciate two meilisearch clients seems overkill.

I wouldn't say so. Stateful singleton is more harmful than instantiating single per user :)

I see in docs it is explicitly related to search endpoint. So I think this could be accepted as a SearchQuery option, but not as a global setting for whole Meilisearch client

@norkunas norkunas requested review from Strift and curquiza February 4, 2025 08:20
@wandyou
Copy link
Author

wandyou commented Feb 4, 2025

I see in docs it is explicitly related to search endpoint. So I think this could be accepted as a SearchQuery option, but not as a global setting for whole Meilisearch client

Well as of for now, analytics is only done on searches, so yes indeed it would make more sense to add it as a Search Query option.

And regarding other analytics endpoints ? Like hit clicking, conversions, etc ? Would be nice to implement those as well.

@norkunas
Copy link
Collaborator

norkunas commented Feb 4, 2025

And regarding other analytics endpoints ? Like hit clicking, conversions, etc ? Would be nice to implement those as well.

Yes, for this you could open another issue :)

Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.53%. Comparing base (368c73f) to head (f88abd0).
Report is 51 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #721      +/-   ##
==========================================
- Coverage   90.74%   89.53%   -1.21%     
==========================================
  Files          52       56       +4     
  Lines        1340     1424      +84     
==========================================
+ Hits         1216     1275      +59     
- Misses        124      149      +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Strift Strift added the enhancement New feature or request label Feb 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to pass headers directly in the Client builder - For example to easily pass X-MS-USER-ID
3 participants