From d05ca291446f00b24942215e993ef15c2c9a37ea Mon Sep 17 00:00:00 2001 From: recursivetree <60423027+recursivetree@users.noreply.github.com> Date: Sun, 24 Dec 2023 05:08:46 +0100 Subject: [PATCH] Rewrite Filterable trait used for Squad filters (#658) * reimplement basics * fix or groups at root level * add is not condition * add remaining filter+comments * fix tests * add new test case * styleci * fix crash with manual squads * fix test --- .gitignore | 5 + src/Exceptions/InvalidFilterException.php | 28 ++++ src/Models/Filterable.php | 187 ++++++++-------------- src/Models/QueryGroupBuilder.php | 125 +++++++++++++++ tests/Squads/AllianceRuleTest.php | 40 +++++ tests/Squads/NoRulesTest.php | 12 +- 6 files changed, 271 insertions(+), 126 deletions(-) create mode 100644 src/Exceptions/InvalidFilterException.php create mode 100644 src/Models/QueryGroupBuilder.php diff --git a/.gitignore b/.gitignore index 8b5b2bb7e..c90a0e72e 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,8 @@ .env.php .env .idea/ + +# testing +vendor/ +composer.lock +.phpunit.cache/test-results diff --git a/src/Exceptions/InvalidFilterException.php b/src/Exceptions/InvalidFilterException.php new file mode 100644 index 000000000..79f3e6fe5 --- /dev/null +++ b/src/Exceptions/InvalidFilterException.php @@ -0,0 +1,28 @@ +getFilters(), 'and') && ! property_exists($this->getFilters(), 'or')) return true; - // init a new object based on parameter - $class = get_class($member); + $query = new QueryGroupBuilder($member->newQuery(), true); - return (new $class)::where($member->getKeyName(), $member->id) - ->where(function ($query) { - - // verb will determine what kind of method we have to use (simple andWhere or orWhere) - $verb = property_exists($this->getFilters(), 'and') ? 'whereHas' : 'orWhereHas'; - - // rules will determine all objects and ruleset in the current object root - $rules = property_exists($this->getFilters(), 'and') ? $this->getFilters()->and : $this->getFilters()->or; - - // sort rules by path - $sorted_rules = $this->sortFiltersByRelations($rules); - - // TODO: find a way to handle this using recursive loop and determine common patterns - $sorted_rules->each(function ($rules_group, $path) use ($query, $verb) { - - if (is_int($path)) { - - $parent_verb = $verb == 'whereHas' ? 'where' : 'orWhere'; - - $query->$parent_verb(function ($q2) use ($rules_group, $parent_verb) { - - // all pairs will be group in distinct collection due to previous group by - // as a result, we have to iterate over each members - $rules_group->each(function ($rules) use ($parent_verb, $q2) { - - // determine the match kind for the current pair - // sort all rules from this pair in order to ensure relationship consistency - $group_verb = property_exists($rules, 'and') ? 'whereHas' : 'orWhereHas'; - $rules_group = $this->sortFiltersByRelations(property_exists($rules, 'and') ? - $rules->and : $rules->or); - - $rules_group->each(function ($rules, $path) use ($parent_verb, $group_verb, $q2) { - - // prepare query from current pair group - $q2->$parent_verb(function ($q3) use ($rules, $path, $group_verb) { - $q3->$group_verb($path, function ($q4) use ($rules, $group_verb) { - - // prevent dummy query by encapsulating rules outside relations - $q4->where(function ($q5) use ($rules, $group_verb) { - $this->applyRules($q5, $group_verb, $rules); - }); - }); - }); - }); - }); - }); - - } else { - - $rules = $rules_group; - - // using group by, we've pair all relationships by their top level relation - // $query->whereHas('characters(.*)', function ($sub_query) { ... } - $query->$verb($path, function ($q2) use ($rules, $verb) { - - // override the complete rule group with a global where. - // doing it so will prevent SQL query like - // (users.id = tokens.user_id OR character_id = ? OR character_id = ?) - // when multiple rules are applied on same path. - $q2->where(function ($q3) use ($rules, $verb) { - $this->applyRules($q3, $verb, $rules); - }); - }); - } - }); - })->exists(); - } - - /** - * @param array $rules - * @return \Illuminate\Support\Collection - */ - private function sortFiltersByRelations(array $rules) - { - return collect($rules)->sortBy('path')->groupBy(function ($rule) { - if (! property_exists($rule, 'path')) - return false; - - $relation_members = explode('.', $rule->path); + // make sure we only allow results of the entity we are checking count + $query->where(function (Builder $inner_query) use ($member) { + $inner_query->where($member->getKeyName(), $member->getKey()); + }); - return $relation_members[0]; + // wrap this in an inner query to ensure it is '(correct_entity_check) AND (rule1 AND/OR rule2)' + $query->where(function ($inner_query) { + $this->applyGroup($inner_query, $this->getFilters()); }); + + return $query->getUnderlyingQuery()->exists(); } /** - * @param \Illuminate\Database\Eloquent\Builder $query - * @param string $group_verb - * @param array|\Illuminate\Support\Collection $rules - * @return \Illuminate\Database\Eloquent\Builder + * Applies a filter group to $query. + * + * @param Builder $query the query to add the filter group to + * @param stdClass $group the filter group configuration + * + * @throws InvalidFilterException */ - private function applyRules(Builder $query, string $group_verb, $rules) + private function applyGroup(Builder $query, stdClass $group): void { - $query_operator = $group_verb == 'whereHas' ? 'where' : 'orWhere'; - - if (is_array($rules)) - $rules = collect($rules); + $query_group = new QueryGroupBuilder($query, property_exists($group, 'and')); - $rules->sortBy('path')->groupBy('path')->each(function ($relation_rules, $relation) use ($query, $query_operator) { - if (strpos($relation, '.') !== false) { - $relation = substr($relation, strpos($relation, '.') + 1); + $rules = $query_group->isAndGroup() ? $group->and : $group->or; - $query->whereHas($relation, function ($q2) use ($query_operator, $relation_rules) { - $q2->where(function ($q3) use ($relation_rules, $query_operator) { - foreach ($relation_rules as $index => $rule) { - if ($rule->operator == 'contains') { - $json_operator = $query_operator == 'where' ? 'whereJsonContains' : 'orWhereJsonContains'; - $q3->$json_operator($rule->field, $rule->criteria); - } else { - $q3->$query_operator($rule->field, $rule->operator, $rule->criteria); - } - } - }); - }); + foreach ($rules as $rule){ + // check if this is a nested group or not + if(property_exists($rule, 'path')){ + $this->applyRule($query_group, $rule); } else { - $query->where(function ($q2) use ($relation_rules, $query_operator) { - foreach ($relation_rules as $index => $rule) { - if ($rule->operator == 'contains') { - $json_operator = $query_operator == 'where' ? 'whereJsonContains' : 'orWhereJsonContains'; - $q2->$json_operator($rule->field, $rule->criteria); - } else { - $q2->$query_operator($rule->field, $rule->operator, $rule->criteria); - } - } + // this is a nested group + $query_group->where(function ($group_query) use ($rule) { + $this->applyGroup($group_query, $rule); }); } - }); + } + } - return $query; + /** + * Applies a rule to a query group. + * + * @param QueryGroupBuilder $query the query to add the rule to + * @param stdClass $rule the rule configuration + * + * @throws InvalidFilterException + */ + private function applyRule(QueryGroupBuilder $query, stdClass $rule): void { + // 'is' operator + if($rule->operator === '=' || $rule->operator === '<' || $rule->operator === '>'){ + // normal comparison operations need to relation to exist + $query->whereHas($rule->path, function (Builder $inner_query) use ($rule) { + $inner_query->where($rule->field, $rule->operator, $rule->criteria); + }); + } elseif ($rule->operator === '<>' || $rule->operator === '!=') { + // not equal is special cased since a missing relation is the same as not equal + $query->whereDoesntHave($rule->path, function (Builder $inner_query) use ($rule) { + $inner_query->where($rule->field, $rule->criteria); + }); + } elseif($rule->operator === 'contains'){ + // contains is maybe a misleading name, since it actually checks if json contains a value + $query->whereHas($rule->path, function (Builder $inner_query) use ($rule) { + $inner_query->whereJsonContains($rule->field, $rule->criteria); + }); + } else { + throw new InvalidFilterException(sprintf('Unknown rule operator: \'%s\'', $rule->operator)); + } } } diff --git a/src/Models/QueryGroupBuilder.php b/src/Models/QueryGroupBuilder.php new file mode 100644 index 000000000..e74a8b5e7 --- /dev/null +++ b/src/Models/QueryGroupBuilder.php @@ -0,0 +1,125 @@ +query = $query; + $this->is_and_group = $is_and_group; + } + + /** + * @return bool Returns true when the where clauses are linked by AND + */ + public function isAndGroup(): bool { + return $this->is_and_group; + } + + /** + * @return Builder The underlying query builder used for this group + */ + public function getUnderlyingQuery(): Builder + { + return $this->query; + } + + /** + * Either adds a 'where' or 'orWhere' to the query, depending on if it is an AND linked group or not. + * + * @param Closure $callback a callback to add constraints + * @return $this + * + * @see Builder::where + * @see Builder::orWhere + */ + public function where(Closure $callback): QueryGroupBuilder { + if($this->is_and_group){ + $this->query->where($callback); + } else { + $this->query->orWhere($callback); + } + + return $this; + } + + /** + * Either adds a 'whereHas' or 'orWhereHas' to the query, depending on if it is an AND linked group or not. + * + * @param string $relation the relation to check for existence + * @param Closure $callback a callback to add more constraints + * @return $this + * + * @see Builder::whereHas + * @see Builder::orWhereHas + */ + public function whereHas(string $relation, Closure $callback): QueryGroupBuilder { + if($this->is_and_group){ + $this->query->whereHas($relation, $callback); + } else { + $this->query->orWhereHas($relation, $callback); + } + + return $this; + } + + /** + * Either adds a 'whereDoesntHave' or 'orWhereDoesntHave' to the query, depending on if it is an AND linked group or not. + * + * @param string $relation the relation to check for absence + * @param Closure $callback a callback to add more constraints + * @return $this + * + * @see Builder::whereDoesntHave + * @see Builder::orWhereDoesntHave + */ + public function whereDoesntHave(string $relation, Closure $callback): QueryGroupBuilder { + if($this->is_and_group){ + $this->query->whereDoesntHave($relation, $callback); + } else { + $this->query->orWhereDoesntHave($relation, $callback); + } + + return $this; + } +} diff --git a/tests/Squads/AllianceRuleTest.php b/tests/Squads/AllianceRuleTest.php index cd60cacb0..5a93e477c 100644 --- a/tests/Squads/AllianceRuleTest.php +++ b/tests/Squads/AllianceRuleTest.php @@ -155,4 +155,44 @@ public function testUserHasCharacterInAlliance() $this->assertFalse($squad->isEligible($user)); } } + + /** + * This test checks whether a character from a corp outside an alliance is eligible for a squad with a alliance is not filter. + * In SeAT 4, this was not working properly + */ + public function testCharacterHasNoAllianceWithAllianceIsNotFilter(){ + $squad = new Squad([ + 'name' => 'Testing Squad', + 'description' => 'Some description', + 'type' => 'auto', + 'filters' => json_encode([ + 'and' => [ + [ + 'name' => 'alliance', + 'path' => 'characters.affiliation', + 'field' => 'alliance_id', + 'operator' => '<>', + 'criteria' => 99000000, + 'text' => 'Random Alliance', + ], + ], + ]), + ]); + + $user = User::first(); + + $user->characters->each(function ($character){ + $character->affiliation->update([ + 'alliance_id' => 99000000, + ]); + }); + $this->assertFalse($squad->isEligible($user)); + + $user->characters->each(function ($character){ + $character->affiliation->update([ + 'alliance_id' => null, + ]); + }); + $this->assertTrue($squad->isEligible($user)); + } } diff --git a/tests/Squads/NoRulesTest.php b/tests/Squads/NoRulesTest.php index ce73cd3d1..ce29211aa 100644 --- a/tests/Squads/NoRulesTest.php +++ b/tests/Squads/NoRulesTest.php @@ -27,6 +27,7 @@ use Orchestra\Testbench\TestCase; use Seat\Eveapi\Models\Character\CharacterInfo; use Seat\Eveapi\Models\RefreshToken; +use Seat\Web\Exceptions\InvalidFilterException; use Seat\Web\Models\Squads\Squad; use Seat\Web\Models\User; use Seat\Web\WebServiceProvider; @@ -72,10 +73,10 @@ protected function setUp(): void Event::fake(); - CharacterInfo::factory(50) + CharacterInfo::factory(5) ->create(); - User::factory(10) + User::factory(1) ->create() ->each(function ($user) { CharacterInfo::whereDoesntHave('refresh_token')->get() @@ -99,11 +100,10 @@ public function testUserWithNoRules() ]); // pickup users - $users = User::all(); + $user = User::first(); // ensure no users are eligible - foreach ($users as $user) { - $this->assertTrue($squad->isEligible($user)); - } + $this->assertTrue($squad->isEligible($user), 'Squad without filter is open to anyone'); + } }