From d45948e1bd4e5b159491e243eb1db8d11dc35a01 Mon Sep 17 00:00:00 2001 From: Phan An Date: Sun, 18 Nov 2018 22:50:03 +0100 Subject: [PATCH] Revise the smart playlist rule --- app/Models/Rule.php | 36 +- app/Services/SmartPlaylistService.php | 39 +- .../Services/SmartPlaylistServiceTest.php | 362 ++++-------------- ... and isNot) or (is and isGreaterThan).json | 44 +++ tests/blobs/rules/beginsWith.json | 15 + tests/blobs/rules/contains.json | 15 + tests/blobs/rules/doesNotContain.json | 15 + tests/blobs/rules/endsWith.json | 15 + tests/blobs/rules/inLast.json | 15 + tests/blobs/rules/is and isNot.json | 23 ++ tests/blobs/rules/is or is.json | 28 ++ tests/blobs/rules/is.json | 15 + tests/blobs/rules/isBetween.json | 16 + tests/blobs/rules/isLessThan.json | 15 + tests/blobs/rules/isNot.json | 15 + tests/blobs/rules/notInLast.json | 15 + tests/blobs/rules/requiresUser.json | 15 + 17 files changed, 367 insertions(+), 331 deletions(-) create mode 100644 tests/blobs/rules/(is and isNot) or (is and isGreaterThan).json create mode 100644 tests/blobs/rules/beginsWith.json create mode 100644 tests/blobs/rules/contains.json create mode 100644 tests/blobs/rules/doesNotContain.json create mode 100644 tests/blobs/rules/endsWith.json create mode 100644 tests/blobs/rules/inLast.json create mode 100644 tests/blobs/rules/is and isNot.json create mode 100644 tests/blobs/rules/is or is.json create mode 100644 tests/blobs/rules/is.json create mode 100644 tests/blobs/rules/isBetween.json create mode 100644 tests/blobs/rules/isLessThan.json create mode 100644 tests/blobs/rules/isNot.json create mode 100644 tests/blobs/rules/notInLast.json create mode 100644 tests/blobs/rules/requiresUser.json diff --git a/app/Models/Rule.php b/app/Models/Rule.php index 748d3c11..c79f7abd 100644 --- a/app/Models/Rule.php +++ b/app/Models/Rule.php @@ -8,14 +8,6 @@ use InvalidArgumentException; class Rule { - private const LOGIC_OR = 'or'; - private const LOGIC_AND = 'and'; - - private const VALID_LOGICS = [ - self::LOGIC_AND, - self::LOGIC_OR, - ]; - public const OPERATOR_IS = 'is'; public const OPERATOR_IS_NOT = 'isNot'; public const OPERATOR_CONTAINS = 'contains'; @@ -43,17 +35,14 @@ class Rule ]; private $operator; - private $logic; private $value; private $model; private $parameterFactory; private function __construct(array $config) { - $this->validateLogic($config['logic']); $this->validateOperator($config['operator']); - $this->logic = $config['logic']; $this->value = $config['value']; $this->model = $config['model']; $this->operator = $config['operator']; @@ -83,31 +72,18 @@ class Rule // If the model is something like 'artist.name' or 'interactions.play_count', we have a subquery to deal with. // We handle such a case with a recursive call which, in theory, should work with an unlimited level of nesting, // though in practice we only have one level max. - $subQueryLogic = self::LOGIC_AND ? 'whereHas' : 'orWhereHas'; - - return $query->$subQueryLogic($fragments[0], function (Builder $subQuery) use ($fragments): Builder { + return $query->whereHas($fragments[0], function (Builder $subQuery) use ($fragments): Builder { return $this->build($subQuery, $fragments[1]); }); } + /** + * Resolve the logic of a (sub)query base on the configured operator. + * Basically, if the operator is "between," we use "whereBetween." Otherwise, it's "where." Simple. + */ private function resolveLogic(): string { - if ($this->operator === self::OPERATOR_IS_BETWEEN) { - return $this->logic === self::LOGIC_AND ? 'whereBetween' : 'orWhereBetween'; - } - - return $this->logic === self::LOGIC_AND ? 'where' : 'orWhere'; - } - - private function validateLogic(string $logic): void - { - if (!in_array($logic, self::VALID_LOGICS, true)) { - throw new InvalidArgumentException( - sprintf( - '%s is not a valid value for logic. Valid values are: %s', $logic, implode(', ', self::VALID_LOGICS) - ) - ); - } + return $this->operator === self::OPERATOR_IS_BETWEEN ? 'whereBetween' : 'where'; } private function validateOperator(string $operator): void diff --git a/app/Services/SmartPlaylistService.php b/app/Services/SmartPlaylistService.php index 6cde56cf..49d7bf67 100644 --- a/app/Services/SmartPlaylistService.php +++ b/app/Services/SmartPlaylistService.php @@ -30,18 +30,22 @@ class SmartPlaylistService $rules = $this->addRequiresUserRules($playlist->rules, $playlist->user); - return $this->buildQueryForRules($rules)->get(); + return $this->buildQueryFromRules($rules)->get(); } - public function buildQueryForRules(array $rules): Builder + public function buildQueryFromRules(array $rules): Builder { - return tap(Song::query(), static function (Builder $query) use ($rules): Builder { - foreach ($rules as $config) { - $query = Rule::create($config)->build($query); - } + $query = Song::query(); - return $query; + collect($rules)->each(static function (array $ruleGroup) use ($query): void { + $query->orWhere(static function (Builder $subQuery) use ($ruleGroup): void { + foreach ($ruleGroup['rules'] as $config) { + Rule::create($config)->build($subQuery); + } + }); }); + + return $query; } /** @@ -51,26 +55,29 @@ class SmartPlaylistService * * @param string[] $rules */ - private function addRequiresUserRules(array $rules, User $user): array + public function addRequiresUserRules(array $rules, User $user): array { - $additionalRules = []; + foreach ($rules as &$ruleGroup) { + $additionalRules = []; - foreach ($rules as $rule) { - foreach (self::RULE_REQUIRES_USER_PREFIXES as $modelPrefix) { - if (starts_with($rule['model'], $modelPrefix)) { - $additionalRules[] = $this->createRequireUserRule($user, $modelPrefix); + foreach ($ruleGroup['rules'] as &$config) { + foreach (self::RULE_REQUIRES_USER_PREFIXES as $modelPrefix) { + if (starts_with($config['model'], $modelPrefix)) { + $additionalRules[] = $this->createRequireUserRule($user, $modelPrefix); + } } } + + // make sure all those additional rules are unique. + $ruleGroup['rules'] = array_merge($ruleGroup['rules'], collect($additionalRules)->unique('model')->all()); } - // make sure all those additional rules are unique. - return array_merge($rules, collect($additionalRules)->unique('model')->all()); + return $rules; } private function createRequireUserRule(User $user, string $modelPrefix): array { return [ - 'logic' => 'and', 'model' => $modelPrefix.'user_id', 'operator' => 'is', 'value' => [$user->id], diff --git a/tests/Integration/Services/SmartPlaylistServiceTest.php b/tests/Integration/Services/SmartPlaylistServiceTest.php index 48df8b06..b1d2263e 100644 --- a/tests/Integration/Services/SmartPlaylistServiceTest.php +++ b/tests/Integration/Services/SmartPlaylistServiceTest.php @@ -32,176 +32,78 @@ class SmartPlaylistServiceTest extends TestCase parent::tearDown(); } + private function readFixtureFile(string $fileName): array + { + return json_decode(file_get_contents(__DIR__ . '/../../blobs/rules/' . $fileName), true); + } + public function provideRules(): array { return [ [ - [ - [ - 'logic' => 'and', - 'model' => 'title', - 'operator' => 'beginsWith', - 'value' => ['Foo'], - ], - ], - 'select * from "songs" where "title" LIKE ?', + $this->readFixtureFile('is.json'), + 'select * from "songs" where ("title" = ?)', + ['Foo'], + ], + [ + $this->readFixtureFile('isNot.json'), + 'select * from "songs" where ("title" <> ?)', + ['Foo'], + ], + [ + $this->readFixtureFile('contains.json'), + 'select * from "songs" where ("title" LIKE ?)', + ['%Foo%'], + ], + [ + $this->readFixtureFile('doesNotContain.json'), + 'select * from "songs" where ("title" NOT LIKE ?)', + ['%Foo%'], + ], + [ + $this->readFixtureFile('beginsWith.json'), + 'select * from "songs" where ("title" LIKE ?)', ['Foo%'], ], [ - [ - [ - 'logic' => 'and', - 'model' => 'title', - 'operator' => 'endsWith', - 'value' => ['Foo'], - ], - ], - 'select * from "songs" where "title" LIKE ?', + $this->readFixtureFile('endsWith.json'), + 'select * from "songs" where ("title" LIKE ?)', ['%Foo'], ], [ - [ - [ - 'logic' => 'or', - 'model' => 'title', - 'operator' => 'is', - 'value' => ['Foo'], - ], - ], - 'select * from "songs" where "title" = ?', - ['Foo'], + $this->readFixtureFile('isBetween.json'), + 'select * from "songs" where ("bit_rate" between ? and ?)', + ['192', '256'], ], [ - [ - [ - 'logic' => 'or', - 'model' => 'title', - 'operator' => 'isNot', - 'value' => ['Foo'], - ], - ], - 'select * from "songs" where "title" <> ?', - ['Foo'], - ], - [ - [ - [ - 'logic' => 'and', - 'model' => 'title', - 'operator' => 'contains', - 'value' => ['Foo'], - ], - ], - 'select * from "songs" where "title" LIKE ?', - ['%Foo%'], - ], - [ - [ - [ - 'logic' => 'and', - 'model' => 'title', - 'operator' => 'notContain', - 'value' => ['Foo'], - ], - ], - 'select * from "songs" where "title" NOT LIKE ?', - ['%Foo%'], - ], - [ - [ - [ - 'logic' => 'and', - 'model' => 'length', - 'operator' => 'isGreaterThan', - 'value' => [100], - ], - ], - 'select * from "songs" where "length" > ?', - [100], - ], - [ - [ - [ - 'logic' => 'and', - 'model' => 'length', - 'operator' => 'isLessThan', - 'value' => [100], - ], - ], - 'select * from "songs" where "length" < ?', - [100], - ], - [ - [ - [ - 'logic' => 'and', - 'model' => 'length', - 'operator' => 'isBetween', - 'value' => [100, 200], - ], - ], - 'select * from "songs" where "length" between ? and ?', - [100, 200], - ], - [ - [ - [ - 'logic' => 'and', - 'model' => 'created_at', - 'operator' => 'inLast', - 'value' => [7], - ], - ], - 'select * from "songs" where "created_at" >= ?', + $this->readFixtureFile('inLast.json'), + 'select * from "songs" where ("created_at" >= ?)', ['2018-07-08 00:00:00'], ], [ - [ - [ - 'logic' => 'and', - 'model' => 'created_at', - 'operator' => 'notInLast', - 'value' => [7], - ], - ], - 'select * from "songs" where "created_at" < ?', + $this->readFixtureFile('notInLast.json'), + 'select * from "songs" where ("created_at" < ?)', ['2018-07-08 00:00:00'], ], [ - [ - [ - 'logic' => 'and', - 'model' => 'created_at', - 'operator' => 'notInLast', - 'value' => [7], - ], - [ - 'logic' => 'or', - 'model' => 'length', - 'operator' => 'isBetween', - 'value' => [100, 200], - ], - ], - 'select * from "songs" where "created_at" < ? or "length" between ? and ?', - ['2018-07-08 00:00:00', 100, 200], + $this->readFixtureFile('isLessThan.json'), + 'select * from "songs" where ("length" < ?)', + ['300'], ], [ - [ - [ - 'logic' => 'or', - 'model' => 'artist.name', - 'operator' => 'isNot', - 'value' => ['Foo'], - ], - [ - 'logic' => 'and', - 'model' => 'created_at', - 'operator' => 'notInLast', - 'value' => [7], - ], - ], - 'select * from "songs" where exists (select * from "artists" where "songs"."artist_id" = "artists"."id" or ("name" <> ?)) and "created_at" < ?', - ['Foo', '2018-07-08 00:00:00', 100, 200], + $this->readFixtureFile('is and isNot.json'), + 'select * from "songs" where ("title" = ? and exists (select * from "artists" where "songs"."artist_id" = "artists"."id" and "name" <> ?))', + ['Foo', 'Bar'], + ], + [ + $this->readFixtureFile('(is and isNot) or (is and isGreaterThan).json'), + 'select * from "songs" where ("title" = ? and exists (select * from "albums" where "songs"."album_id" = "albums"."id" and "name" <> ?)) or ("genre" = ? and "bit_rate" > ?)', + ['Foo', 'Bar', 'Metal', '128'], + ], + [ + $this->readFixtureFile('is or is.json'), + 'select * from "songs" where ("title" = ?) or (exists (select * from "artists" where "songs"."artist_id" = "artists"."id" and "name" = ?))', + ['Foo', 'Bar'], ], ]; } @@ -214,7 +116,7 @@ class SmartPlaylistServiceTest extends TestCase */ public function testBuildQueryForRules(array $rules, string $sql, array $bindings): void { - $query = $this->service->buildQueryForRules($rules); + $query = $this->service->buildQueryFromRules($rules); $this->assertSame($sql, $query->toSql()); $queryBinding = $query->getBindings(); @@ -226,146 +128,36 @@ class SmartPlaylistServiceTest extends TestCase } } + public function testAddRequiresUserRules(): void + { + $rules = $this->readFixtureFile('requiresUser.json'); + + /** @var User $user */ + $user = factory(User::class)->create(); + + $this->assertEquals([ + 'model' => 'interactions.user_id', + 'operator' => 'is', + 'value' => [$user->id], + ], $this->service->addRequiresUserRules($rules, $user)[0]['rules'][1]); + } + public function testAllOperatorsAreCovered(): void { - $operators = collect($this->provideRules())->map(static function (array $providedRules): string { - return $providedRules[0][0]['operator']; + $rules = collect($this->provideRules())->map(static function (array $providedRule): array { + return $providedRule[0]; }); - $this->assertSame(count(Rule::VALID_OPERATORS), $operators->unique()->count()); - } + $operators = []; - public function testRuleWithoutSubQueries(): void - { - /** @var User $user */ - $user = factory(User::class)->create(); - - factory(Song::class, 2)->create([ - 'title' => static function (): string { - return 'Unique Foo '.uniqid(); - }, - ]); - - factory(Song::class, 5)->create([ - 'title' => 'Has nothing to do with the rule', - ]); - - $playlist = factory(Playlist::class)->create([ - 'user_id' => $user->id, - 'rules' => [ - [ - 'logic' => 'and', - 'model' => 'title', - 'operator' => 'beginsWith', - 'value' => ['Unique Foo'], - ], - ], - ]); - - $songs = $this->service->getSongs($playlist); - $this->assertCount(2, $songs); - $songs->each(function (Song $song): void { - $this->assertStringStartsWith('Unique Foo', $song->title); - }); - } - - public function testRulesWithSubQuery(): void - { - /** @var User $user */ - $user = factory(User::class)->create(); - - /** @var Album[] $albums */ - $albums = factory(Album::class, 3)->create([ - 'artist_id' => static function (): int { - return factory(Artist::class)->create([ - 'name' => static function (): string { - return 'Foo Artist '.uniqid(); - }, - ])->id; - }, - ]); - - foreach ($albums as $album) { - factory(Song::class, 2)->create([ - 'album_id' => $album->id, - 'artist_id' => $album->artist->id, - ]); + foreach ($rules as $rule) { + foreach ($rule as $ruleGroup) { + foreach ($ruleGroup['rules'] as $config) { + $operators[] = $config['operator']; + } + } } - /** @var Album $inApplicableAlbum */ - $inApplicableAlbum = factory(Album::class)->create([ - 'artist_id' => factory(Artist::class)->create(['name' => 'Nothing to do with the rule']), - ]); - - factory(Song::class, 1)->create([ - 'album_id' => $inApplicableAlbum->id, - ]); - - $playlist = factory(Playlist::class)->create([ - 'user_id' => $user->id, - 'rules' => [ - [ - 'logic' => 'and', - 'model' => 'artist.name', - 'operator' => 'beginsWith', - 'value' => ['Foo Artist'], - ], - [ - 'logic' => 'or', - 'model' => 'title', - 'operator' => 'beginsWith', - 'value' => ['There should not be anything like this'], - ], - ], - ]); - - $songs = $this->service->getSongs($playlist); - $this->assertCount(6, $songs); - $songs->each(function (Song $song): void { - $this->assertStringStartsWith('Foo Artist', $song->artist->name); - }); - } - - public function testRulesWithUser(): void - { - /** @var User $bob */ - $bob = factory(User::class)->create(); - /** @var User $alice */ - $alice = factory(User::class)->create(); - - $bobSong = factory(Song::class)->create([ - 'title' => 'Song for Bob', - ]); - factory(Interaction::class)->create([ - 'user_id' => $bob->id, - 'song_id' => $bobSong->id, - 'play_count' => 10, - ]); - - $aliceSong = factory(Song::class)->create([ - 'title' => 'Song for Alice', - ]); - factory(Interaction::class)->create([ - 'user_id' => $alice->id, - 'song_id' => $aliceSong->id, - 'play_count' => 12, - ]); - - $playlist = factory(Playlist::class)->create([ - 'user_id' => $bob->id, - 'rules' => [ - [ - 'logic' => 'and', - 'model' => 'interactions.play_count', - 'operator' => 'isGreaterThan', - 'value' => [5], - ], - ], - ]); - - /** @var Song[]|Collection $songs */ - $songs = $this->service->getSongs($playlist); - $this->assertCount(1, $songs); - $this->assertSame('Song for Bob', $songs[0]->title); + $this->assertSame(count(Rule::VALID_OPERATORS), count(array_unique($operators))); } } diff --git a/tests/blobs/rules/(is and isNot) or (is and isGreaterThan).json b/tests/blobs/rules/(is and isNot) or (is and isGreaterThan).json new file mode 100644 index 00000000..b1eaeb06 --- /dev/null +++ b/tests/blobs/rules/(is and isNot) or (is and isGreaterThan).json @@ -0,0 +1,44 @@ +[ + { + "id": 1541356285385, + "rules": [ + { + "id": 1541356313181, + "model": "title", + "operator": "is", + "value": [ + "Foo" + ] + }, + { + "id": 1542573964152, + "model": "album.name", + "operator": "isNot", + "value": [ + "Bar" + ] + } + ] + }, + { + "id": 1542573971423, + "rules": [ + { + "id": 1542573972485, + "model": "genre", + "operator": "is", + "value": [ + "Metal" + ] + }, + { + "id": 1542573978609, + "model": "bit_rate", + "operator": "isGreaterThan", + "value": [ + "128" + ] + } + ] + } +] diff --git a/tests/blobs/rules/beginsWith.json b/tests/blobs/rules/beginsWith.json new file mode 100644 index 00000000..1531cf11 --- /dev/null +++ b/tests/blobs/rules/beginsWith.json @@ -0,0 +1,15 @@ +[ + { + "id": 1541356285385, + "rules": [ + { + "id": 1541356313181, + "model": "title", + "operator": "beginsWith", + "value": [ + "Foo" + ] + } + ] + } +] diff --git a/tests/blobs/rules/contains.json b/tests/blobs/rules/contains.json new file mode 100644 index 00000000..8c3cab51 --- /dev/null +++ b/tests/blobs/rules/contains.json @@ -0,0 +1,15 @@ +[ + { + "id": 1541356285385, + "rules": [ + { + "id": 1541356313181, + "model": "title", + "operator": "contains", + "value": [ + "Foo" + ] + } + ] + } +] diff --git a/tests/blobs/rules/doesNotContain.json b/tests/blobs/rules/doesNotContain.json new file mode 100644 index 00000000..cbb32b0c --- /dev/null +++ b/tests/blobs/rules/doesNotContain.json @@ -0,0 +1,15 @@ +[ + { + "id": 1541356285385, + "rules": [ + { + "id": 1541356313181, + "model": "title", + "operator": "notContain", + "value": [ + "Foo" + ] + } + ] + } +] diff --git a/tests/blobs/rules/endsWith.json b/tests/blobs/rules/endsWith.json new file mode 100644 index 00000000..1e7398a5 --- /dev/null +++ b/tests/blobs/rules/endsWith.json @@ -0,0 +1,15 @@ +[ + { + "id": 1541356285385, + "rules": [ + { + "id": 1541356313181, + "model": "title", + "operator": "endsWith", + "value": [ + "Foo" + ] + } + ] + } +] diff --git a/tests/blobs/rules/inLast.json b/tests/blobs/rules/inLast.json new file mode 100644 index 00000000..e5be40da --- /dev/null +++ b/tests/blobs/rules/inLast.json @@ -0,0 +1,15 @@ +[ + { + "id": 1541356285385, + "rules": [ + { + "id": 1541356313181, + "model": "created_at", + "operator": "inLast", + "value": [ + "7" + ] + } + ] + } +] diff --git a/tests/blobs/rules/is and isNot.json b/tests/blobs/rules/is and isNot.json new file mode 100644 index 00000000..2b00a57d --- /dev/null +++ b/tests/blobs/rules/is and isNot.json @@ -0,0 +1,23 @@ +[ + { + "id": 1541356285385, + "rules": [ + { + "id": 1541356313181, + "model": "title", + "operator": "is", + "value": [ + "Foo" + ] + }, + { + "id": 1542572962736, + "model": "artist.name", + "operator": "isNot", + "value": [ + "Bar" + ] + } + ] + } +] diff --git a/tests/blobs/rules/is or is.json b/tests/blobs/rules/is or is.json new file mode 100644 index 00000000..e2310f47 --- /dev/null +++ b/tests/blobs/rules/is or is.json @@ -0,0 +1,28 @@ +[ + { + "id": 1541356285385, + "rules": [ + { + "id": 1541356313181, + "model": "title", + "operator": "is", + "value": [ + "Foo" + ] + } + ] + }, + { + "id": 1542573097361, + "rules": [ + { + "id": 1542573098295, + "model": "artist.name", + "operator": "is", + "value": [ + "Bar" + ] + } + ] + } +] diff --git a/tests/blobs/rules/is.json b/tests/blobs/rules/is.json new file mode 100644 index 00000000..7c325f5f --- /dev/null +++ b/tests/blobs/rules/is.json @@ -0,0 +1,15 @@ +[ + { + "id": 1541356285385, + "rules": [ + { + "id": 1541356313181, + "model": "title", + "operator": "is", + "value": [ + "Foo" + ] + } + ] + } +] diff --git a/tests/blobs/rules/isBetween.json b/tests/blobs/rules/isBetween.json new file mode 100644 index 00000000..be638614 --- /dev/null +++ b/tests/blobs/rules/isBetween.json @@ -0,0 +1,16 @@ +[ + { + "id": 1541356285385, + "rules": [ + { + "id": 1541356313181, + "model": "bit_rate", + "operator": "isBetween", + "value": [ + "192", + "256" + ] + } + ] + } +] diff --git a/tests/blobs/rules/isLessThan.json b/tests/blobs/rules/isLessThan.json new file mode 100644 index 00000000..e0e90f96 --- /dev/null +++ b/tests/blobs/rules/isLessThan.json @@ -0,0 +1,15 @@ +[ + { + "id": 1541356285385, + "rules": [ + { + "id": 1541356313181, + "model": "length", + "operator": "isLessThan", + "value": [ + "300" + ] + } + ] + } +] diff --git a/tests/blobs/rules/isNot.json b/tests/blobs/rules/isNot.json new file mode 100644 index 00000000..2f0bc426 --- /dev/null +++ b/tests/blobs/rules/isNot.json @@ -0,0 +1,15 @@ +[ + { + "id": 1541356285385, + "rules": [ + { + "id": 1541356313181, + "model": "title", + "operator": "isNot", + "value": [ + "Foo" + ] + } + ] + } +] diff --git a/tests/blobs/rules/notInLast.json b/tests/blobs/rules/notInLast.json new file mode 100644 index 00000000..a8e35668 --- /dev/null +++ b/tests/blobs/rules/notInLast.json @@ -0,0 +1,15 @@ +[ + { + "id": 1541356285385, + "rules": [ + { + "id": 1541356313181, + "model": "created_at", + "operator": "notInLast", + "value": [ + "7" + ] + } + ] + } +] diff --git a/tests/blobs/rules/requiresUser.json b/tests/blobs/rules/requiresUser.json new file mode 100644 index 00000000..80c292d8 --- /dev/null +++ b/tests/blobs/rules/requiresUser.json @@ -0,0 +1,15 @@ +[ + { + "id": 1541356285385, + "rules": [ + { + "id": 1541356313181, + "model": "interactions.play_count", + "operator": "is", + "value": [ + "42" + ] + } + ] + } +]