Allow instantiating the Meilisearch client with custom HTTP headers #721
Conversation
|
You can pass a http client configured with your headers, as it's a http client responsibility 🤔 |
|
I get it, and that's how I am solving this issue right now. But the 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 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 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 |
|
I may have not been clear enough on the fact that I want to simplify the implementation of 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 |
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 |
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. |
Yes, for this you could open another issue :) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #721 +/- ##
==========================================
- Coverage 90.74% 89.82% -0.92%
==========================================
Files 52 59 +7
Lines 1340 1455 +115
==========================================
+ Hits 1216 1307 +91
- Misses 124 148 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
So far, since we only use it for the search analytics, and since I don't have any idea if we will extend that to other places, I think it makes sense to be a SearchQuery option indeed.
This could be a central issue in the https://github.com/meilisearch/integrations-guides repo btw! CC: @macraig @gmourier feedback regarding the future of the analytics :) |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new option to pass custom headers when building the Meilisearch client, addressing issue #720 by allowing developers to merge custom headers with the pre-configured ones.
- Added a new $headers parameter (defaulting to an empty array) to the constructors in both the HTTP client and the main client.
- Merges the custom headers with the existing Authorization header when applicable.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Http/Client.php | Added an optional $headers parameter and merged it with defaults. |
| src/Client.php | Propagated the new $headers option into the Meilisearch client adapter. |
| @@ -41,7 +41,8 @@ public function __construct( | |||
| ?ClientInterface $httpClient = null, | |||
| ?RequestFactoryInterface $reqFactory = null, | |||
| array $clientAgents = [], | |||
| ?StreamFactoryInterface $streamFactory = null | |||
| ?StreamFactoryInterface $streamFactory = null, | |||
| $headers = [] | |||
There was a problem hiding this comment.
Consider adding a type declaration (e.g. array) for the $headers parameter to improve API consistency and type safety.
Copilot uses AI. Check for mistakes.
| @@ -47,9 +47,10 @@ public function __construct( | |||
| ?ClientInterface $httpClient = null, | |||
| ?RequestFactoryInterface $requestFactory = null, | |||
| array $clientAgents = [], | |||
| ?StreamFactoryInterface $streamFactory = null | |||
| ?StreamFactoryInterface $streamFactory = null, | |||
| $headers = [] | |||
There was a problem hiding this comment.
Consider adding a type declaration (e.g. array) for the $headers parameter in the constructor to maintain consistency across the client API.
Copilot uses AI. Check for mistakes.
|
""" WalkthroughThe constructors of two Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Client.php (1)
54-54: Add type declaration for $headers parameterThe $headers parameter lacks a type declaration, which is inconsistent with the rest of the parameters in this function signature. Consider adding an explicit
arraytype declaration to maintain consistency and improve type safety.- $headers = [] + array $headers = []src/Http/Client.php (1)
45-45: Add type declaration for $headers parameterThe $headers parameter lacks a type declaration, which is inconsistent with the rest of the parameters in this function signature. Consider adding an explicit
arraytype declaration to maintain consistency and improve type safety.- $headers = [] + array $headers = []
🧹 Nitpick comments (3)
src/Http/Client.php (3)
38-45: Document the headers parameter formatConsider adding a PHPDoc comment for the
$headersparameter to document its expected format and usage. This would align with the class property which is documented as@var array<string,string>./** * @param array<int, string> $clientAgents + * @param array<string, string> $headers Custom HTTP headers to include in all requests */ public function __construct( string $url, ?string $apiKey = null, ?ClientInterface $httpClient = null, ?RequestFactoryInterface $reqFactory = null, array $clientAgents = [], ?StreamFactoryInterface $streamFactory = null, array $headers = [] ) {
57-57: Document header override behaviorThe current implementation allows custom headers to override default headers like 'User-Agent' and 'Authorization'. Consider documenting this behavior, either in the PHPDoc or as a code comment, to prevent unintended consequences.
- $this->headers = array_merge($this->headers, $headers); + // Custom headers take precedence over default headers + $this->headers = array_merge($this->headers, $headers);
44-57: Consider architectural alternatives for analytics headersBased on the PR discussion, there seems to be consensus that analytics headers like
X-MS-USER-IDmight be better handled at the query level (e.g., as a SearchQuery option) rather than at the client level, since they currently only apply to search endpoints.While this implementation provides a working solution for custom headers, consider exploring SearchQuery options for analytics-specific headers in a future PR, as this might provide a more appropriate abstraction for this specific use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/Client.php(1 hunks)src/Http/Client.php(2 hunks)
Strift
changed the title
Pull Request
Related issue
Fixes #720
What does this PR do?
Improvement
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.addHeader()PR checklist
Please check if your PR fulfills the following requirements:
Extra info
First contribution of mine to this repo, maybe I missed some subtilities.
Summary by CodeRabbit