✨ scout queries #11
✨ scout queries #11
Conversation
WalkthroughThis update introduces support for applying access control logic to Laravel Scout search queries within the application. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Auth
participant LaravelScoutBuilder as Scout\Builder
participant AccessServiceProvider
participant Control
participant Perimeter
User->>Scout\Builder: Initiate search query
Scout\Builder->>AccessServiceProvider: controlled() macro
AccessServiceProvider->>Control: Create control instance for model
Control->>Control: scoutQueried(Scout\Builder, Auth::user())
Control->>Perimeter: applyScoutQueryCallback(Scout\Builder, user)
Perimeter-->>Control: Modified Scout\Builder
Control-->>Scout\Builder: Return controlled query
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
🧹 Nitpick comments (7)
src/AccessServiceProvider.php (1)
49-58: Good implementation of conditional Scout integration.The
bootScoutBuildermethod correctly checks for Scout's existence before adding functionality, making this integration truly optional.Consider adding a PHPDoc comment to describe the purpose of this method:
+/** + * Register the Scout Builder macro if Laravel Scout is installed. + * + * @return void + */ protected function bootScoutBuilder(): void { if (class_exists(Builder::class)) { Builder::macro('controlled', function (Builder $builder) { $control = $builder->model->newControl(); return $control->queried($builder, Auth::user()); }); } }tests/Support/Access/Controls/ModelControl.php (1)
29-31: Consider importing the Scout Builder class.The Scout query filters are correctly implemented, but the code uses fully qualified class names instead of importing the class.
Consider adding an import for the Scout Builder class at the top of the file:
use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Model; +use Laravel\Scout\Builder as ScoutBuilder; use Lomkit\Access\Controls\Control;Then use the imported class name in the callbacks:
- ->scoutQuery(function (\Laravel\Scout\Builder $query, Model $user) { + ->scoutQuery(function (ScoutBuilder $query, Model $user) { return $query->where('is_shared', true); })Also applies to: 40-42, 51-53, 62-64
src/Controls/Control.php (3)
15-15: Make the TODO comment more descriptive.The current TODO comment "change readme image" is vague and doesn't provide enough context about what needs to be changed or why.
- // @TODO: change readme image + // @TODO: Update readme image to include the new Scout query functionality
135-154: Consider reducing code duplication with the query control logic.The
applyScoutQueryControlmethod is almost identical toapplyQueryControl, differing only in type signatures and callback calls. This creates maintenance overhead as any logic changes would need to be duplicated in both methods.Consider extracting the common logic into a template method or using a more generic approach that could handle both query types. For example:
protected function applyQueryControlGeneric($query, Model $user, string $type = 'eloquent'): mixed { $noResultCallback = function ($query) use ($type) { return $type === 'eloquent' ? $this->noResultQuery($query) : $this->noResultScoutQuery($query); }; foreach ($this->perimeters() as $perimeter) { if ($perimeter->applyAllowedCallback($user)) { $method = $type === 'eloquent' ? 'applyQueryCallback' : 'applyScoutQueryCallback'; $query = $perimeter->$method($query, $user); $noResultCallback = function ($query) {return $query; }; if (!$perimeter->overlays()) { return $query; } } } return $noResultCallback($query); }Then, the specific methods could use this generic implementation:
protected function applyQueryControl(Builder $query, Model $user): Builder { return $this->applyQueryControlGeneric($query, $user, 'eloquent'); } protected function applyScoutQueryControl(\Laravel\Scout\Builder $query, Model $user): \Laravel\Scout\Builder { return $this->applyQueryControlGeneric($query, $user, 'scout'); }
168-171: Consider a more robust approach for ensuring no Scout results.Using a field name like
__NOT_A_VALID_FIELD__to ensure no results might be fragile if a future database schema includes a field with that name.Consider a more robust approach, such as:
- return $query->where('__NOT_A_VALID_FIELD__', 0); + return $query->whereRaw('1=0'); // Guaranteed to return no resultsOr documenting the intention more clearly:
- return $query->where('__NOT_A_VALID_FIELD__', 0); + // Use an intentionally invalid field name to ensure no results are returned + // This approach is used because Scout doesn't support whereRaw + return $query->where('__lomkit_access_no_results__', 0);tests/Feature/ControlsScoutQueryTest.php (2)
85-106: Consider removing unused test data.In this test, you create models with
is_clientset to true, but these models aren't relevant to the test assertion which only checks for theis_sharedcondition.Model::factory() - ->state(['is_client' => true]) - ->count(50) - ->create(); -Model::factory() ->state(['is_shared' => true]) ->count(50) ->create(); -Model::factory() - ->state(['is_own' => true]) - ->count(50) - ->create();
1-107: Consider adding tests for actual search results.The current tests only verify that the correct conditions are added to the Scout query builder, but they don't verify that the actual search results are filtered correctly.
Consider adding additional assertions that execute the search and verify the results:
public function test_control_scout_queried_actually_filters_results(): void { Auth::user()->update(['should_client' => true]); // Create models with different is_client values $clientModels = Model::factory() ->state(['is_client' => true]) ->count(3) ->create(); $nonClientModels = Model::factory() ->state(['is_client' => false]) ->count(3) ->create(); // Execute the controlled search $results = Model::search() ->controlled() // Assuming there's a 'controlled' macro that applies the current user's control ->get(); // Assert that only client models are returned $this->assertCount(3, $results); foreach ($results as $result) { $this->assertTrue($result->is_client); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
composer.json(1 hunks)src/AccessServiceProvider.php(2 hunks)src/Controls/Control.php(4 hunks)src/Perimeters/Perimeter.php(3 hunks)tests/Feature/ControlsScoutQueryTest.php(1 hunks)tests/Support/Access/Controls/ModelControl.php(4 hunks)tests/Support/Models/Model.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
tests/Support/Models/Model.php (1)
tests/Support/Database/Factories/ModelFactory.php (1)
ModelFactory(8-28)
tests/Support/Access/Controls/ModelControl.php (2)
src/Perimeters/Perimeter.php (2)
scoutQuery(112-117)query(105-110)tests/Support/Models/Model.php (1)
Model(11-25)
src/Controls/Control.php (4)
src/Perimeters/Perimeter.php (4)
query(105-110)applyAllowedCallback(65-68)applyScoutQueryCallback(40-43)overlays(134-137)tests/Support/Models/Model.php (1)
Model(11-25)tests/Support/Access/Controls/ModelControl.php (1)
perimeters(15-69)src/Perimeters/OverlayPerimeter.php (1)
overlays(14-17)
src/Perimeters/Perimeter.php (1)
tests/Support/Models/Model.php (1)
Model(11-25)
tests/Feature/ControlsScoutQueryTest.php (3)
tests/Support/Models/Model.php (1)
Model(11-25)tests/Support/Access/Controls/ModelControl.php (1)
ModelControl(13-70)src/Controls/Control.php (1)
scoutQueried(101-104)
🪛 GitHub Actions: tests
tests/Support/Models/Model.php
[error] 11-11: PHP Fatal error: Trait "Laravel\Scout\Searchable" not found.
🔇 Additional comments (11)
composer.json (1)
19-19:❓ Verification inconclusive
Good addition of Laravel Scout as a dev dependency.
This addition properly integrates Laravel Scout functionality while keeping it optional for production use, aligning with the PR objectives to support Scout queries.
🏁 Script executed:
#!/bin/bash # Check Laravel Scout version compatibility with supported Laravel versions # Display the supported Laravel versions in composer.json echo "Supported Laravel versions:" cat composer.json | grep "laravel/framework" # Check Laravel Scout package compatibility information curl -s https://packagist.org/packages/laravel/scout.json | jq '.package.versions["10.0.0"].require."illuminate/database"'Length of output: 285
Attention: Verify Composer File Location & Version Constraint
The addition of Laravel Scout (
"laravel/scout": "^10") appears to align with the PR’s objectives for supporting Scout queries. However, our automated checks revealed two points that need clarification:
- The script was unable to locate a
composer.jsonfile at the repository root. Please confirm thatcomposer.jsonis correctly placed and updated in the expected location.- The Packagist query for version
10.0.0did not return any value for"illuminate/database", which might be due to using an incorrect version key or a change in the package’s dependency definitions. Verify that the version constraint and its associated dependencies are correctly defined for your intended usage.Once these points are confirmed, the integration looks solid.
tests/Support/Access/Controls/ModelControl.php (1)
29-31:❓ Verification inconclusive
Verify that the Scout query implementation is consistent with Eloquent.
Scout's query filtering works differently than Eloquent's. For Eloquent filters, you're using
orWherebut for Scout filters you're usingwhere. This might lead to inconsistent behavior.
🏁 Script executed:
#!/bin/bash # Check if the Scout builder's where method works the same as Eloquent's orWhere # Find the Control class that processes these queries to see how they're handled rg -A 10 "function scoutQueried" --type phpLength of output: 767
Action: Validate Consistency of Scout Query Filters
- The snippet in
tests/Support/Access/Controls/ModelControl.phpuses a Scout query closure that applies a simplewherefilter:->scoutQuery(function (\Laravel\Scout\Builder $query, Model $user) { return $query->where('is_shared', true); })- In contrast, Eloquent filter logic elsewhere uses
orWhere, which means the filtering behavior might differ.- The
scoutQueriedmethod insrc/Controls/Control.phpdelegates query modifications viaapplyScoutQueryControl(). This confirms that the Scout query is being processed as designed.- Since Laravel Scout’s filtering mechanism inherently supports only the
wheremethod (without anorWherealternative), please verify that this difference is intentional. If an OR-type condition is needed for consistency, consider either revising the filtering logic or adding documentation to clarify the design choice.src/Perimeters/Perimeter.php (4)
11-11: Good addition of the new property for Scout query callback.The new protected
$scoutQueryCallbackproperty aligns well with the existing code style and design patterns in the class.
20-20: Initialize default Scout query callback correctly.The default implementation properly returns the query unchanged, which is a sensible default that matches the pattern used for other callbacks.
40-43: Well-structured method for applying the Scout query callback.This method follows the same pattern as the existing
applyQueryCallbackmethod, maintaining consistency in the codebase. The return type is correctly specified as\Laravel\Scout\Builder.
112-117: Good implementation of the fluent setter for Scout query callback.The
scoutQuerymethod maintains the same fluent interface pattern used elsewhere in the class, allowing for method chaining and maintaining a consistent API.src/Controls/Control.php (1)
93-104: Good implementation of the Scout query control method.The
scoutQueriedmethod follows the same pattern as the existingqueriedmethod, providing a consistent API extension for Scout queries. The method is well-documented with appropriate PHPDoc comments.tests/Feature/ControlsScoutQueryTest.php (4)
6-18: Good test for the no-passing perimeter scenario.This test properly verifies that when no perimeter passes, the system applies a condition that should yield no results. The assertion correctly checks the
wheresproperty of the Scout query.
20-36: Well-structured test for client perimeter.This test clearly sets up the necessary conditions, creates the appropriate model instances, and verifies that the client perimeter's Scout query callback is correctly applied.
38-59: Good test for overlayed perimeters.This test properly verifies the behavior of an overlayed shared perimeter combined with a client perimeter, ensuring both conditions are applied to the Scout query.
61-83: Comprehensive test for shared and own perimeters.This test validates that when both shared and own perimeters apply, the Scout query includes the appropriate conditions for both perimeters.
Docstrings generation was requested by @GautierDele. * #11 (comment) The following files were modified: * `src/AccessServiceProvider.php` * `src/Controls/Control.php` * `src/Perimeters/Perimeter.php` * `tests/Support/Access/Controls/ModelControl.php` * `tests/Support/Models/Model.php`
|
Note Generated docstrings for this pull request at #12 |
* 📝 Add docstrings to `feature/scout-queries` Docstrings generation was requested by @GautierDele. * #11 (comment) The following files were modified: * `src/AccessServiceProvider.php` * `src/Controls/Control.php` * `src/Perimeters/Perimeter.php` * `tests/Support/Access/Controls/ModelControl.php` * `tests/Support/Models/Model.php` * Apply fixes from StyleCI * ♻️ removed unecessary comments * Apply fixes from StyleCI --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: StyleCI Bot <bot@styleci.io> Co-authored-by: Gautier DELEGLISE <gautier.deleglise@gmail.com>
Summary by CodeRabbit