Update search and rawSearch methods to use SearchQuery#833
Update search and rawSearch methods to use SearchQuery#833tw2066 wants to merge 11 commits intomeilisearch:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSearch methods in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Endpoints/Indexes.php (2)
191-203: Widenedsearch()parameter type is BC‑safe and consistentAccepting
SearchQuery|arrayfor$searchParamswhile delegating normalization torawSearch()cleanly extends the API without breaking existing array callers. Consider adding a brief phpdoc@param SearchQuery|array $searchParamsfor tools that still lean on annotations, but that’s optional.
205-212: Confirm intended precedence between$queryandSearchQuery::qNormalizing
$searchParamswithinstanceof SearchQueryis correct and keepsrawSearch()working for both arrays and objects. Note that with:$parameters = array_merge(['q' => $query], $searchParams);a
qkey coming fromSearchQuery::toArray()will override the$queryargument when both are set. If you instead want the explicit$queryparameter to always win, you could swap the merge order:- $parameters = array_merge( - - ['q' => $query], - $searchParams - ); + $parameters = array_merge( + $searchParams, + ['q' => $query], + );Please confirm which behavior you intend.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Endpoints/Indexes.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Endpoints/Indexes.php (1)
src/Contracts/SearchQuery.php (2)
SearchQuery(7-495)toArray(463-494)
🔇 Additional comments (1)
src/Endpoints/Indexes.php (1)
13-13: ImportingSearchQuerymatches the new method signaturesThe added
use Meilisearch\Contracts\SearchQuery;cleanly supports the new union-typed parameters without redundancy.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #833 +/- ##
==========================================
- Coverage 89.78% 87.06% -2.73%
==========================================
Files 59 82 +23
Lines 1449 1809 +360
==========================================
+ Hits 1301 1575 +274
- Misses 148 234 +86 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Endpoints/SearchQueryTest.php`:
- Line 483: In testShowRankingScoreDetails there's an unused local variable
$http created by "new Client(...)" — remove the unused declaration to clean up
the test; locate the instantiation in testShowRankingScoreDetails and delete the
"$http = new Client($this->host, getenv('MEILISEARCH_API_KEY'));" line so the
test no longer defines an unused variable.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Endpoints/SearchQueryTest.php`:
- Line 631: The async index operations (updateFilterableAttributes and
updateDocuments called on $this->index) return Task objects and must be awaited
to avoid racing with subsequent assertions; update each call to append ->wait()
so the test waits for the index mutation to complete (e.g., ensure calls to
$this->index->updateFilterableAttributes(...) and
$this->index->updateDocuments(...) use ->wait() before performing
searches/assertions).
| public function testSearchAndRetrieveFacetStats(): void | ||
| { | ||
| $this->index = $this->createEmptyIndex($this->safeIndexName()); | ||
| $this->index->updateFilterableAttributes(['info.reviewNb']); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/Endpoints/SearchQueryTest.php"
# Check if file exists and get its line count
if [ ! -f "$FILE" ]; then
echo "File not found: $FILE"
exit 1
fi
echo "=== Examining lines around 631 ==="
sed -n '625,640p' "$FILE"
echo
echo "=== Examining lines around 647 ==="
sed -n '641,655p' "$FILE"
echo
echo "=== Examining lines around 676 ==="
sed -n '670,680p' "$FILE"
echo
echo "=== All updateFilterableAttributes and updateDocuments calls ==="
rg -n 'updateFilterableAttributes|updateDocuments' "$FILE" | head -20Repository: meilisearch/meilisearch-php
Length of output: 3287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where the index property is defined to understand its type
echo "=== Looking for index property definition ==="
rg -n '\$this->index' tests/Endpoints/SearchQueryTest.php | head -5
echo
echo "=== Looking for test class setup/initialization ==="
rg -n 'function setUp|function setUpBeforeClass|class SearchQueryTest' tests/Endpoints/SearchQueryTest.php | head -10
echo
echo "=== Check what type of index is being used ==="
rg -B5 -A5 'protected.*index|private.*index' tests/Endpoints/SearchQueryTest.php | head -30Repository: meilisearch/meilisearch-php
Length of output: 835
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for the Index class and check if updateFilterableAttributes returns something awaitable
echo "=== Searching for Index class ==="
find . -name "*.php" -path "*/src/*" | xargs grep -l "class Index" | head -5
echo
echo "=== Looking for method signatures of updateFilterableAttributes and updateDocuments ==="
rg -A10 'public.*function updateFilterableAttributes|public.*function updateDocuments' --type php | grep -E '(function|return|Task|Index|->)' | head -20Repository: meilisearch/meilisearch-php
Length of output: 3512
Add ->wait() to async index operations to prevent test flakiness.
Lines 631, 647, and 676 call async index operations without ->wait(). Since updateFilterableAttributes() and updateDocuments() return Task objects, they execute asynchronously and can race with subsequent search assertions, causing intermittent test failures.
🔧 Proposed fix
- $this->index->updateFilterableAttributes(['info.reviewNb']);
+ $this->index->updateFilterableAttributes(['info.reviewNb'])->wait();
@@
- $this->index->updateFilterableAttributes(['genre']);
+ $this->index->updateFilterableAttributes(['genre'])->wait();
@@
- $this->index->updateDocuments(self::DOCUMENTS);
+ $this->index->updateDocuments(self::DOCUMENTS)->wait();Also applies to: 647-647, 676-676
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Endpoints/SearchQueryTest.php` at line 631, The async index operations
(updateFilterableAttributes and updateDocuments called on $this->index) return
Task objects and must be awaited to avoid racing with subsequent assertions;
update each call to append ->wait() so the test waits for the index mutation to
complete (e.g., ensure calls to $this->index->updateFilterableAttributes(...)
and $this->index->updateDocuments(...) use ->wait() before performing
searches/assertions).
|
I love your pic @tw2066 👌 🍊 |
There was a problem hiding this comment.
Hey @tw2066, the CI is not passing.
I suggest you run the tests and linter checks locally and ensure it works before submitting for review 🙏
Please look into the CodeRabbitAI review comments and address them; you can resolve the irrelevant ones.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Endpoints/SearchQueryTest.php (1)
29-684: Add a directrawSearch(SearchQuery)test to ensure the SearchQuery parameter type path is covered.The test suite validates raw responses through
search(..., ['raw' => true])but does not callrawSearch()with aSearchQueryinstance directly. Since therawSearch()method signature acceptsarray|SearchQueryas its second parameter, a focused test explicitly passing aSearchQuerytorawSearch()would verify this type acceptance path is functional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Endpoints/SearchQueryTest.php` around lines 29 - 684, Add a new unit test method (e.g. testRawSearchWithSearchQuery) that constructs a SearchQuery instance, sets a couple of options (like setLimit(1) or setFacets(['genre'])), and calls rawSearch on the index with that SearchQuery (e.g. $this->index->rawSearch(null, $searchQuery) or appropriate signature); assert the returned raw array contains expected keys (hits, estimatedTotalHits, facetDistribution when applicable) to verify rawSearch accepts a SearchQuery object; reference the SearchQuery class and the rawSearch method when locating where to add the test alongside the other search tests in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Endpoints/SearchQueryTest.php`:
- Around line 200-231: The test testParametersCanBeAStar asserts presence of
_matchesPosition but never enables it on the SearchQuery; call
$searchQuery->setShowMatchesPosition(true) on the SearchQuery instance (before
the two $this->index->search(...) calls) so that matches positions are returned;
update the SearchQuery setup in testParametersCanBeAStar to include
setShowMatchesPosition(true) alongside setAttributesToRetrieve/crop/highlight to
fix the failing assertions.
---
Nitpick comments:
In `@tests/Endpoints/SearchQueryTest.php`:
- Around line 29-684: Add a new unit test method (e.g.
testRawSearchWithSearchQuery) that constructs a SearchQuery instance, sets a
couple of options (like setLimit(1) or setFacets(['genre'])), and calls
rawSearch on the index with that SearchQuery (e.g. $this->index->rawSearch(null,
$searchQuery) or appropriate signature); assert the returned raw array contains
expected keys (hits, estimatedTotalHits, facetDistribution when applicable) to
verify rawSearch accepts a SearchQuery object; reference the SearchQuery class
and the rawSearch method when locating where to add the test alongside the other
search tests in the file.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79953999-d9c0-4482-b772-86da2dc2b65f
📒 Files selected for processing (2)
src/Endpoints/Indexes.phptests/Endpoints/SearchQueryTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Endpoints/Indexes.php
|
|
||
| /** | ||
| * @internal | ||
| * @coversNothing |
There was a problem hiding this comment.
are these annotations really useful?
|
The relevant issues have been fixed |
| $this->index->updateDocuments(self::DOCUMENTS)->wait(); | ||
| } | ||
|
|
||
| public function testBasicSearchWithFinitePagination(): void |
There was a problem hiding this comment.
I don't think that it's necessary to duplicate tests from tests/Endpoints/SearchTest.php
IMHO we should just ensure that SearchQuery is converted to array and that it is passed to the http client.
Or just add a single test case to tests/Endpoints/SearchTest.php where you pass the SearchQuery object and that's enough
There was a problem hiding this comment.
There's no need to add test cases for this, the toArray method is obvious
|
Our repository uses rebases for merging PRs via the merge queue. Unfortunately:
@tw2066 can you look into resolving these issues? You may perform the rebase manually locally, or open a new PR with a clean history. Cheers, |
Pull Request
Update search and rawSearch methods to use SearchQuery
Summary by CodeRabbit
New Features
Tests