fix: return empty paginator when scout text value is empty#216
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.46)PHP Parse error: syntax error, unexpected token "->" in /vendor/phpunit/phpunit/src/Runner/Version.php on line 38 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Concerns/PerformsRestOperations.php`:
- Around line 56-65: The LengthAwarePaginator instantiation uses
$resource->defaultLimit directly which can be null; change it to use a fallback
consistent with Paginable::paginate() (e.g., $resource->defaultLimit ?? 50) so
per-page is never null. Update the block in PerformsRestOperations (the code
that calls $this->afterSearch($request) and returns $this->buildResponse(...,
new LengthAwarePaginator(...))) to replace $resource->defaultLimit with the
fallback expression and ensure the paginator receives an integer perPage.
- Around line 42-43: Docblock in the PerformsRestOperations trait currently ends
mid-sentence ("Returns an empty paginated result if search text is provided but
empty,") — complete or rephrase it to a full sentence in the
PerformsRestOperations.php docblock (e.g., "Returns an empty paginated result if
search text is provided but empty.") or reformat to remove the trailing comma;
update the docblock surrounding the trait/method that describes returning an
empty paginated result so it is grammatically complete and clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ebf219b-4f9e-42f8-94d5-7d5433a9fc2d
📒 Files selected for processing (1)
src/Concerns/PerformsRestOperations.php
| if ($textProvided && !filled($request->input('search.text.value'))) { | ||
| $this->afterSearch($request); | ||
| return $this->buildResponse( | ||
| $resource, | ||
| new LengthAwarePaginator( | ||
| [], | ||
| 0, | ||
| $request->input('search.limit', $resource->defaultLimit) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Missing fallback for defaultLimit — potential null perPage value.
In Paginable::paginate() (context snippet 3), the default limit is computed as $this->defaultLimit ?? 50, ensuring a fallback when the property is unset. Here, $resource->defaultLimit is used directly without the ?? 50 fallback.
If neither search.limit is provided nor $resource->defaultLimit is set, perPage could be null, which may cause unexpected behavior in the paginator.
🔧 Suggested fix to align with Paginable pattern
if ($textProvided && !filled($request->input('search.text.value'))) {
$this->afterSearch($request);
return $this->buildResponse(
$resource,
new LengthAwarePaginator(
[],
0,
- $request->input('search.limit', $resource->defaultLimit)
+ $request->input('search.limit', $resource->defaultLimit ?? 50)
)
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Concerns/PerformsRestOperations.php` around lines 56 - 65, The
LengthAwarePaginator instantiation uses $resource->defaultLimit directly which
can be null; change it to use a fallback consistent with Paginable::paginate()
(e.g., $resource->defaultLimit ?? 50) so per-page is never null. Update the
block in PerformsRestOperations (the code that calls
$this->afterSearch($request) and returns $this->buildResponse(..., new
LengthAwarePaginator(...))) to replace $resource->defaultLimit with the fallback
expression and ensure the paginator receives an integer perPage.
1 participant
Problem
When
{"search": {"text": {"value": ""}}}or{"value": null}is sent to the search endpoint, Elasticsearch returns a 400 error:This happens because
$request->has('search.text')returnstrueeven with an empty value, causingScoutBuilderto be selected and an invalidmatch: { query: "" }to be sent to Elasticsearch.Root cause
The dispatcher in
PerformsRestOperations::search()was only checking for the presence ofsearch.text, not whether its value was actually usable:Fix
When
search.textis provided but its value is empty/null, we return an empty paginated result immediately without hitting Elasticsearch or the database:A
buildResponse()private method was also extracted to avoid duplicating the response structure.Behaviour
Summary by CodeRabbit
Bug Fixes
Refactor