From 30f4878ec3ae937466d4c43ea7e41a87a854f109 Mon Sep 17 00:00:00 2001 From: Phan An Date: Sun, 10 Oct 2021 20:05:51 +0200 Subject: [PATCH] feat(smart-playlist): validate smart playlist request (#1366) --- app/Casts/SmartPlaylistRulesCast.php | 2 +- .../Controllers/API/PlaylistController.php | 4 +- .../Requests/API/PlaylistStoreRequest.php | 6 + app/Http/Requests/API/PlaylistSyncRequest.php | 4 + .../Requests/API/PlaylistUpdateRequest.php | 17 ++ app/Rules/ValidSmartPlaylistRulePayload.php | 31 ++++ app/Values/SmartPlaylistRule.php | 36 +++- app/Values/SmartPlaylistRuleGroup.php | 24 +-- tests/Feature/PlaylistTest.php | 31 +++- .../Services/SmartPlaylistServiceTest.php | 38 ++-- .../ValidSmartPlaylistRulePayloadTest.php | 164 ++++++++++++++++++ ... and isNot) or (is and isGreaterThan).json | 8 +- tests/blobs/rules/isBetween.json | 6 +- 13 files changed, 321 insertions(+), 50 deletions(-) create mode 100644 app/Http/Requests/API/PlaylistUpdateRequest.php create mode 100644 app/Rules/ValidSmartPlaylistRulePayload.php create mode 100644 tests/Unit/Rules/ValidSmartPlaylistRulePayloadTest.php diff --git a/app/Casts/SmartPlaylistRulesCast.php b/app/Casts/SmartPlaylistRulesCast.php index 69a3abc5..0a1c6dfb 100644 --- a/app/Casts/SmartPlaylistRulesCast.php +++ b/app/Casts/SmartPlaylistRulesCast.php @@ -12,7 +12,7 @@ class SmartPlaylistRulesCast implements CastsAttributes public function get($model, string $key, $value, array $attributes): Collection { return collect(json_decode($value, true) ?: [])->map(static function (array $group): ?SmartPlaylistRuleGroup { - return SmartPlaylistRuleGroup::create($group); + return SmartPlaylistRuleGroup::tryCreate($group); }); } diff --git a/app/Http/Controllers/API/PlaylistController.php b/app/Http/Controllers/API/PlaylistController.php index 642dc5c3..2f905a46 100644 --- a/app/Http/Controllers/API/PlaylistController.php +++ b/app/Http/Controllers/API/PlaylistController.php @@ -4,13 +4,13 @@ namespace App\Http\Controllers\API; use App\Http\Requests\API\PlaylistStoreRequest; use App\Http\Requests\API\PlaylistSyncRequest; +use App\Http\Requests\API\PlaylistUpdateRequest; use App\Models\Playlist; use App\Models\User; use App\Repositories\PlaylistRepository; use App\Services\PlaylistService; use App\Services\SmartPlaylistService; use Illuminate\Contracts\Auth\Authenticatable; -use Illuminate\Http\Request; class PlaylistController extends Controller { @@ -52,7 +52,7 @@ class PlaylistController extends Controller return response()->json($playlist); } - public function update(Request $request, Playlist $playlist) + public function update(PlaylistUpdateRequest $request, Playlist $playlist) { $this->authorize('owner', $playlist); diff --git a/app/Http/Requests/API/PlaylistStoreRequest.php b/app/Http/Requests/API/PlaylistStoreRequest.php index c7ad6275..327d9fe8 100644 --- a/app/Http/Requests/API/PlaylistStoreRequest.php +++ b/app/Http/Requests/API/PlaylistStoreRequest.php @@ -2,6 +2,10 @@ namespace App\Http\Requests\API; +use App\Models\Song; +use App\Rules\ValidSmartPlaylistRulePayload; +use Illuminate\Validation\Rule; + /** * @property array $songs * @property string $name @@ -15,6 +19,8 @@ class PlaylistStoreRequest extends Request return [ 'name' => 'required', 'songs' => 'array', + 'songs.*' => [Rule::exists(Song::class, 'id')], + 'rules' => ['array', 'nullable', new ValidSmartPlaylistRulePayload()], ]; } } diff --git a/app/Http/Requests/API/PlaylistSyncRequest.php b/app/Http/Requests/API/PlaylistSyncRequest.php index c59eb75b..d13b7be4 100644 --- a/app/Http/Requests/API/PlaylistSyncRequest.php +++ b/app/Http/Requests/API/PlaylistSyncRequest.php @@ -2,6 +2,9 @@ namespace App\Http\Requests\API; +use App\Models\Song; +use Illuminate\Validation\Rule; + /** * @property array $songs */ @@ -12,6 +15,7 @@ class PlaylistSyncRequest extends Request { return [ 'songs' => 'present|array', + 'songs.*' => [Rule::exists(Song::class, 'id')], ]; } } diff --git a/app/Http/Requests/API/PlaylistUpdateRequest.php b/app/Http/Requests/API/PlaylistUpdateRequest.php new file mode 100644 index 00000000..c97db53e --- /dev/null +++ b/app/Http/Requests/API/PlaylistUpdateRequest.php @@ -0,0 +1,17 @@ + */ + public function rules(): array + { + return [ + 'name' => 'required', + 'rules' => ['array', 'nullable', new ValidSmartPlaylistRulePayload()], + ]; + } +} diff --git a/app/Rules/ValidSmartPlaylistRulePayload.php b/app/Rules/ValidSmartPlaylistRulePayload.php new file mode 100644 index 00000000..0d01a32b --- /dev/null +++ b/app/Rules/ValidSmartPlaylistRulePayload.php @@ -0,0 +1,31 @@ +id = $config['id'] ?? null; $this->value = $config['value']; @@ -48,9 +69,20 @@ final class SmartPlaylistRule implements Arrayable $this->operator = $config['operator']; } + public static function assertConfig(array $config, bool $allowUserIdModel = true): void + { + Assert::oneOf($config['operator'], self::VALID_OPERATORS); + Assert::oneOf( + $config['model'], + $allowUserIdModel ? array_prepend(self::VALID_MODELS, self::MODEL_USER_ID) : self::VALID_MODELS + ); + Assert::isArray($config['value']); + Assert::countBetween($config['value'], 1, 2); + } + public static function create(array $config): self { - return new static($config); + return new self($config); } /** @return array */ diff --git a/app/Values/SmartPlaylistRuleGroup.php b/app/Values/SmartPlaylistRuleGroup.php index 39a927bf..6c3113ea 100644 --- a/app/Values/SmartPlaylistRuleGroup.php +++ b/app/Values/SmartPlaylistRuleGroup.php @@ -13,23 +13,27 @@ final class SmartPlaylistRuleGroup implements Arrayable /** @var Collection|array */ public Collection $rules; - public static function create(array $jsonArray): ?self + public static function tryCreate(array $jsonArray): ?self { - $group = new self(); - try { - $group->id = $jsonArray['id'] ?? null; - - $group->rules = collect(array_map(static function (array $rawRuleConfig) { - return SmartPlaylistRule::create($rawRuleConfig); - }, $jsonArray['rules'])); - - return $group; + return self::create($jsonArray); } catch (Throwable $exception) { return null; } } + public static function create(array $jsonArray): self + { + $group = new self(); + $group->id = $jsonArray['id'] ?? null; + + $group->rules = collect(array_map(static function (array $rawRuleConfig) { + return SmartPlaylistRule::create($rawRuleConfig); + }, $jsonArray['rules'])); + + return $group; + } + /** @return array */ public function toArray(): array { diff --git a/tests/Feature/PlaylistTest.php b/tests/Feature/PlaylistTest.php index 8cebc6d3..45c35de7 100644 --- a/tests/Feature/PlaylistTest.php +++ b/tests/Feature/PlaylistTest.php @@ -74,20 +74,22 @@ class PlaylistTest extends TestCase public function testCreatingSmartPlaylistIgnoresSongs(): void { - /** @var User $user */ - $user = User::factory()->create(); - $this->postAsUser('api/playlist', [ 'name' => 'Smart Foo Bar', 'rules' => [ - SmartPlaylistRule::create([ - 'model' => 'artist.name', - 'operator' => SmartPlaylistRule::OPERATOR_IS, - 'value' => ['Bob Dylan'], - ])->toArray(), + [ + 'id' => 12345, + 'rules' => [ + SmartPlaylistRule::create([ + 'model' => 'artist.name', + 'operator' => SmartPlaylistRule::OPERATOR_IS, + 'value' => ['Bob Dylan'], + ])->toArray(), + ], + ], ], 'songs' => Song::orderBy('id')->take(3)->get()->pluck('id')->all(), - ], $user); + ]); /** @var Playlist $playlist */ $playlist = Playlist::orderBy('id', 'desc')->first(); @@ -96,6 +98,17 @@ class PlaylistTest extends TestCase self::assertEmpty($playlist->songs); } + public function testCreatingPlaylistWithNonExistentSongsFails(): void + { + $response = $this->postAsUser('api/playlist', [ + 'name' => 'Foo Bar', + 'rules' => [], + 'songs' => ['foo'], + ]); + + $response->assertUnprocessable(); + } + public function testUpdatePlaylistName(): void { /** @var User $user */ diff --git a/tests/Integration/Services/SmartPlaylistServiceTest.php b/tests/Integration/Services/SmartPlaylistServiceTest.php index af509134..03c6820b 100644 --- a/tests/Integration/Services/SmartPlaylistServiceTest.php +++ b/tests/Integration/Services/SmartPlaylistServiceTest.php @@ -32,67 +32,67 @@ class SmartPlaylistServiceTest extends TestCase public function provideRuleConfigs(): array { return [ - [ + 'is' => [ $this->readFixtureFile('is.json'), 'select * from "songs" where ("title" = ?)', ['Foo'], ], - [ + 'is not' => [ $this->readFixtureFile('isNot.json'), 'select * from "songs" where ("title" <> ?)', ['Foo'], ], - [ + 'contains' => [ $this->readFixtureFile('contains.json'), 'select * from "songs" where ("title" LIKE ?)', ['%Foo%'], ], - [ + 'does not contain' => [ $this->readFixtureFile('doesNotContain.json'), 'select * from "songs" where ("title" NOT LIKE ?)', ['%Foo%'], ], - [ + 'begins with' => [ $this->readFixtureFile('beginsWith.json'), 'select * from "songs" where ("title" LIKE ?)', ['Foo%'], ], - [ + 'ends with' => [ $this->readFixtureFile('endsWith.json'), 'select * from "songs" where ("title" LIKE ?)', ['%Foo'], ], - [ + 'is between' => [ $this->readFixtureFile('isBetween.json'), - 'select * from "songs" where ("bit_rate" between ? and ?)', - ['192', '256'], + 'select * from "songs" where ("created_at" between ? and ?)', + ['2021.10.01', '2021.11.01'], ], - [ + 'in last' => [ $this->readFixtureFile('inLast.json'), 'select * from "songs" where ("created_at" >= ?)', ['2018-07-08 00:00:00'], ], - [ + 'not in last' => [ $this->readFixtureFile('notInLast.json'), 'select * from "songs" where ("created_at" < ?)', ['2018-07-08 00:00:00'], ], - [ + 'is less than' => [ $this->readFixtureFile('isLessThan.json'), 'select * from "songs" where ("length" < ?)', ['300'], ], - [ + 'is and is not' => [ $this->readFixtureFile('is and isNot.json'), 'select * from "songs" where ("title" = ? and exists (select * from "artists" where "songs"."artist_id" = "artists"."id" and "name" <> ?))', // @phpcs-ignore-line ['Foo', 'Bar'], ], - [ + '(is and is not) or (is and is greater than)' => [ $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" > ?)', // @phpcs-ignore-line - ['Foo', 'Bar', 'Metal', '128'], + 'select * from "songs" where ("title" = ? and exists (select * from "albums" where "songs"."album_id" = "albums"."id" and "name" <> ?)) or ("title" = ? and "created_at" > ?)', // @phpcs-ignore-line + ['Foo', 'Bar', 'Baz', '2021.10.01'], ], - [ + 'is or is' => [ $this->readFixtureFile('is or is.json'), 'select * from "songs" where ("title" = ?) or (exists (select * from "artists" where "songs"."artist_id" = "artists"."id" and "name" = ?))', // @phpcs-ignore-line ['Foo', 'Bar'], @@ -104,7 +104,7 @@ class SmartPlaylistServiceTest extends TestCase public function testBuildQueryForRules(array $rawRules, string $sql, array $bindings): void { $ruleGroups = collect($rawRules)->map(static function (array $group): SmartPlaylistRuleGroup { - return SmartPlaylistRuleGroup::create($group); + return SmartPlaylistRuleGroup::tryCreate($group); }); $query = $this->service->buildQueryFromRules($ruleGroups); @@ -123,7 +123,7 @@ class SmartPlaylistServiceTest extends TestCase { $ruleGroups = collect($this->readFixtureFile('requiresUser.json'))->map( static function (array $group): SmartPlaylistRuleGroup { - return SmartPlaylistRuleGroup::create($group); + return SmartPlaylistRuleGroup::tryCreate($group); } ); diff --git a/tests/Unit/Rules/ValidSmartPlaylistRulePayloadTest.php b/tests/Unit/Rules/ValidSmartPlaylistRulePayloadTest.php new file mode 100644 index 00000000..45a4326d --- /dev/null +++ b/tests/Unit/Rules/ValidSmartPlaylistRulePayloadTest.php @@ -0,0 +1,164 @@ + */ + public function provideInvalidPayloads(): array + { + return [ + 'invalid format' => ['foo'], + 'invalid model' => [ + [ + [ + 'rules' => [ + [ + 'model' => 'foo', + 'operator' => 'like', + 'value' => ['bar'], + ], + ], + ], + ], + ], + 'invalid operator' => [ + [ + [ + 'rules' => [ + [ + 'model' => 'artist.name', + 'operator' => '