From 9d79d95cb9b4c35d78f61c700c5c9f8ad50e21d6 Mon Sep 17 00:00:00 2001 From: Phan An Date: Wed, 6 Jul 2022 13:05:21 +0200 Subject: [PATCH] refactor: massively simplify SmartPlaylist logic --- .../SmartPlaylistRuleParameterFactory.php | 37 ----- app/Models/Song.php | 132 +----------------- app/Rules/ValidSmartPlaylistRulePayload.php | 2 +- app/Services/SmartPlaylistService.php | 103 ++------------ app/Values/SmartPlaylistRule.php | 58 ++++++-- 5 files changed, 65 insertions(+), 267 deletions(-) delete mode 100644 app/Factories/SmartPlaylistRuleParameterFactory.php diff --git a/app/Factories/SmartPlaylistRuleParameterFactory.php b/app/Factories/SmartPlaylistRuleParameterFactory.php deleted file mode 100644 index 55498354..00000000 --- a/app/Factories/SmartPlaylistRuleParameterFactory.php +++ /dev/null @@ -1,37 +0,0 @@ - $value - * - * @return array - */ - public function createParameters(string $model, string $operator, array $value): array - { - $ruleParameterMap = [ - SmartPlaylistRule::OPERATOR_BEGINS_WITH => [$model, 'LIKE', "$value[0]%"], - SmartPlaylistRule::OPERATOR_ENDS_WITH => [$model, 'LIKE', "%$value[0]"], - SmartPlaylistRule::OPERATOR_IS => [$model, '=', $value[0]], - SmartPlaylistRule::OPERATOR_IS_NOT => [$model, '<>', $value[0]], - SmartPlaylistRule::OPERATOR_CONTAINS => [$model, 'LIKE', "%$value[0]%"], - SmartPlaylistRule::OPERATOR_NOT_CONTAIN => [$model, 'NOT LIKE', "%$value[0]%"], - SmartPlaylistRule::OPERATOR_IS_LESS_THAN => [$model, '<', $value[0]], - SmartPlaylistRule::OPERATOR_IS_GREATER_THAN => [$model, '>', $value[0]], - SmartPlaylistRule::OPERATOR_IS_BETWEEN => [$model, $value], - SmartPlaylistRule::OPERATOR_NOT_IN_LAST => static fn (): array => [$model, '<', now()->subDays($value[0])], - SmartPlaylistRule::OPERATOR_IN_LAST => static fn (): array => [$model, '>=', now()->subDays($value[0])], - ]; - - Assert::keyExists($ruleParameterMap, $operator); - - return is_callable($ruleParameterMap[$operator]) - ? $ruleParameterMap[$operator]() - : $ruleParameterMap[$operator]; - } -} diff --git a/app/Models/Song.php b/app/Models/Song.php index 7b0febce..7d7b5a6a 100644 --- a/app/Models/Song.php +++ b/app/Models/Song.php @@ -2,8 +2,8 @@ namespace App\Models; -use App\Events\LibraryChanged; use Illuminate\Database\Eloquent\Builder; +use Illuminate\Database\Eloquent\Casts\Attribute; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\BelongsTo; @@ -48,12 +48,7 @@ class Song extends Model public $incrementing = false; protected $guarded = []; - /** - * Attributes to be hidden from JSON outputs. - * Here we specify to hide lyrics as well to save some bandwidth (actually, lots of it). - * Lyrics can then be queried on demand. - */ - protected $hidden = ['lyrics', 'updated_at', 'path', 'mtime']; + protected $hidden = ['updated_at', 'path', 'mtime']; protected $casts = [ 'length' => 'float', @@ -64,109 +59,6 @@ class Song extends Model protected $keyType = 'string'; - /** - * Update song info. - * - * @param array $ids - * @param array $data the data array, with these supported fields: - * - title - * - artistName - * - albumName - * - lyrics - * All of these are optional, in which case the info will not be changed - * (except for lyrics, which will be emptied) - * - * @return Collection|array - */ - public static function updateInfo(array $ids, array $data): Collection - { - /* - * A collection of the updated songs. - * - * @var Collection - */ - $updatedSongs = collect(); - - $ids = (array) $ids; - // If we're updating only one song, take into account the title, lyrics, and track number. - $single = count($ids) === 1; - - foreach ($ids as $id) { - /** @var Song|null $song */ - $song = self::with('album', 'album.artist')->find($id); - - if (!$song) { - continue; - } - - $updatedSongs->push($song->updateSingle( - $single ? trim($data['title']) : $song->title, - trim($data['albumName'] ?: $song->album->name), - trim($data['artistName']) ?: $song->artist->name, - $single ? trim($data['lyrics']) : $song->lyrics, - $single ? (int) $data['track'] : $song->track, - (int) $data['compilationState'] - )); - } - - // Our library may have been changed. Broadcast an event to tidy it up if need be. - if ($updatedSongs->count()) { - event(new LibraryChanged()); - } - - return $updatedSongs; - } - - public function updateSingle( - string $title, - string $albumName, - string $artistName, - string $lyrics, - int $track, - int $compilationState - ): self { - if ($artistName === Artist::VARIOUS_NAME) { - // If the artist name is "Various Artists", it's a compilation song no matter what. - $compilationState = 1; - // and since we can't determine the real contributing artist, it's "Unknown" - $artistName = Artist::UNKNOWN_NAME; - } - - $artist = Artist::getOrCreate($artistName); - - switch ($compilationState) { - case 1: // ALL, or forcing compilation status to be Yes - $isCompilation = true; - break; - - case 2: // Keep current compilation status - $isCompilation = $this->album->artist_id === Artist::VARIOUS_ID; - break; - - default: - $isCompilation = false; - break; - } - - $album = Album::getOrCreate($artist, $albumName, $isCompilation); - - $this->artist_id = $artist->id; - $this->album_id = $album->id; - $this->title = $title; - $this->lyrics = $lyrics; - $this->track = $track; - - $this->save(); - - // Clean up unnecessary data from the object - unset($this->album); - unset($this->artist); - // and make sure the lyrics is shown - $this->makeVisible('lyrics'); - - return $this; - } - public function artist(): BelongsTo { return $this->belongsTo(Artist::class); @@ -198,22 +90,12 @@ class Song extends Model return $query->where('path', 'LIKE', "$path%"); } - /** - * Sometimes the tags extracted from getID3 are HTML entity encoded. - * This makes sure they are always sane. - */ - public function setTitleAttribute(string $value): void + protected function title(): Attribute { - $this->attributes['title'] = html_entity_decode($value); - } - - /** - * Some songs don't have a title. - * Fall back to the file name (without extension) for such. - */ - public function getTitleAttribute(?string $value): string - { - return $value ?: pathinfo($this->path, PATHINFO_FILENAME); + return new Attribute( + get: fn (?string $value) => $value ?: pathinfo($this->path, PATHINFO_FILENAME), + set: static fn (string $value) => html_entity_decode($value) + ); } public static function withMeta(User $scopedUser, ?Builder $query = null): Builder diff --git a/app/Rules/ValidSmartPlaylistRulePayload.php b/app/Rules/ValidSmartPlaylistRulePayload.php index 0d01a32b..caff9e7a 100644 --- a/app/Rules/ValidSmartPlaylistRulePayload.php +++ b/app/Rules/ValidSmartPlaylistRulePayload.php @@ -19,7 +19,7 @@ class ValidSmartPlaylistRulePayload implements Rule } return true; - } catch (Throwable $e) { + } catch (Throwable) { return false; } } diff --git a/app/Services/SmartPlaylistService.php b/app/Services/SmartPlaylistService.php index a1ea586e..335a261d 100644 --- a/app/Services/SmartPlaylistService.php +++ b/app/Services/SmartPlaylistService.php @@ -3,119 +3,38 @@ namespace App\Services; use App\Exceptions\NonSmartPlaylistException; -use App\Factories\SmartPlaylistRuleParameterFactory; use App\Models\Playlist; use App\Models\Song; -use App\Models\User; use App\Values\SmartPlaylistRule; use App\Values\SmartPlaylistRuleGroup; +use Illuminate\Contracts\Auth\Guard; use Illuminate\Database\Eloquent\Builder; use Illuminate\Support\Collection; class SmartPlaylistService { - private const USER_REQUIRING_RULE_PREFIXES = ['interactions.']; - - private SmartPlaylistRuleParameterFactory $parameterFactory; - - public function __construct(SmartPlaylistRuleParameterFactory $parameterFactory) + public function __construct(private Guard $auth) { - $this->parameterFactory = $parameterFactory; } - /** @return Collection|array */ + /** @return Collection|array */ public function getSongs(Playlist $playlist): Collection { throw_unless($playlist->is_smart, NonSmartPlaylistException::create($playlist)); - $ruleGroups = $this->addRequiresUserRules($playlist->rule_groups, $playlist->user); + $query = Song::withMeta($this->auth->user()); - return $this->buildQueryFromRules($ruleGroups, $playlist->user) - ->orderBy('songs.title') - ->get(); - } + $playlist->rule_groups->each(static function (SmartPlaylistRuleGroup $group, int $index) use ($query): void { + $clause = $index === 0 ? 'where' : 'orWhere'; - public function buildQueryFromRules(Collection $ruleGroups, User $user): Builder - { - $query = Song::withMeta($user); - - $ruleGroups->each(function (SmartPlaylistRuleGroup $group) use ($query): void { - $query->orWhere(function (Builder $subQuery) use ($group): void { - $group->rules->each(function (SmartPlaylistRule $rule) use ($subQuery): void { - $this->buildQueryForRule($subQuery, $rule); + $query->$clause(static function (Builder $subQuery) use ($group): void { + $group->rules->each(static function (SmartPlaylistRule $rule) use ($subQuery): void { + $subWhere = $rule->operator === SmartPlaylistRule::OPERATOR_IS_BETWEEN ? 'whereBetween' : 'where'; + $subQuery->$subWhere(...$rule->toCriteriaParameters()); }); }); }); - return $query; - } - - /** - * Some rules need to be driven by an additional "user" factor, for example play count, liked, or last played - * (basically everything related to interactions). - * For those, we create an additional "user_id" rule. - * - * @return Collection|array - */ - public function addRequiresUserRules(Collection $ruleGroups, User $user): Collection - { - return $ruleGroups->map(function (SmartPlaylistRuleGroup $group) use ($user): SmartPlaylistRuleGroup { - $clonedGroup = clone $group; - $additionalRules = collect(); - - $group->rules->each(function (SmartPlaylistRule $rule) use ($additionalRules, $user): void { - foreach (self::USER_REQUIRING_RULE_PREFIXES as $modelPrefix) { - if (starts_with($rule->model, $modelPrefix)) { - $additionalRules->add($this->createRequiresUserRule($user, $modelPrefix)); - } - } - }); - - // Make sure all those additional rules are unique. - $clonedGroup->rules = $clonedGroup->rules->merge($additionalRules->unique('model')->collect()); - - return $clonedGroup; - }); - } - - private function createRequiresUserRule(User $user, string $modelPrefix): SmartPlaylistRule - { - return SmartPlaylistRule::create([ - 'model' => $modelPrefix . 'user_id', - 'operator' => 'is', - 'value' => [$user->id], - ]); - } - - public function buildQueryForRule(Builder $query, SmartPlaylistRule $rule, ?string $model = null): Builder - { - if (!$model) { - $model = $rule->model; - } - - $fragments = explode('.', $model, 2); - - if (count($fragments) === 1) { - return $query->{$this->resolveWhereLogic($rule)}( - ...$this->parameterFactory->createParameters($model, $rule->operator, $rule->value) - ); - } - - // 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. - return $query->whereHas( - $fragments[0], - fn (Builder $subQuery) => $this->buildQueryForRule($subQuery, $rule, $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 resolveWhereLogic(SmartPlaylistRule $rule): string - { - return $rule->operator === SmartPlaylistRule::OPERATOR_IS_BETWEEN ? 'whereBetween' : 'where'; + return $query->orderBy('songs.title')->get(); } } diff --git a/app/Values/SmartPlaylistRule.php b/app/Values/SmartPlaylistRule.php index 316a76f4..05711267 100644 --- a/app/Values/SmartPlaylistRule.php +++ b/app/Values/SmartPlaylistRule.php @@ -33,17 +33,17 @@ final class SmartPlaylistRule implements Arrayable self::OPERATOR_NOT_IN_LAST, ]; - public const MODEL_TITLE = 'title'; - public const MODEL_ALBUM_NAME = 'album.name'; - public const MODEL_ARTIST_NAME = 'artist.name'; - public const MODEL_PLAY_COUNT = 'interactions.play_count'; - public const MODEL_LAST_PLAYED = 'interactions.updated_at'; - public const MODEL_USER_ID = 'interactions.user_id'; - public const MODEL_LENGTH = 'length'; - public const MODEL_DATE_ADDED = 'created_at'; - public const MODEL_DATE_MODIFIED = 'updated_at'; + private const MODEL_TITLE = 'title'; + private const MODEL_ALBUM_NAME = 'album.name'; + private const MODEL_ARTIST_NAME = 'artist.name'; + private const MODEL_PLAY_COUNT = 'interactions.play_count'; + private const MODEL_LAST_PLAYED = 'interactions.updated_at'; + private const MODEL_USER_ID = 'interactions.user_id'; + private const MODEL_LENGTH = 'length'; + private const MODEL_DATE_ADDED = 'created_at'; + private const MODEL_DATE_MODIFIED = 'updated_at'; - public const VALID_MODELS = [ + private const VALID_MODELS = [ self::MODEL_TITLE, self::MODEL_ALBUM_NAME, self::MODEL_ARTIST_NAME, @@ -54,6 +54,15 @@ final class SmartPlaylistRule implements Arrayable self::MODEL_DATE_MODIFIED, ]; + private const MODEL_COLUMN_MAP = [ + self::MODEL_TITLE => 'songs.title', + self::MODEL_ALBUM_NAME => 'albums.name', + self::MODEL_ARTIST_NAME => 'artists.name', + self::MODEL_LENGTH => 'songs.length', + self::MODEL_DATE_ADDED => 'songs.created_at', + self::MODEL_DATE_MODIFIED => 'songs.updated_at', + ]; + public ?int $id; public string $operator; public array $value; @@ -96,8 +105,7 @@ final class SmartPlaylistRule implements Arrayable ]; } - /** @param array|self $rule */ - public function equals($rule): bool + public function equals(array|self $rule): bool { if (is_array($rule)) { $rule = self::create($rule); @@ -107,4 +115,30 @@ final class SmartPlaylistRule implements Arrayable && !array_diff($this->value, $rule->value) && $this->model === $rule->model; } + + /** @return array */ + public function toCriteriaParameters(): array + { + $column = array_key_exists($this->model, self::MODEL_COLUMN_MAP) + ? self::MODEL_COLUMN_MAP[$this->model] + : $this->model; + + $resolvers = [ + self::OPERATOR_BEGINS_WITH => [$column, 'LIKE', "{$this->value[0]}%"], + self::OPERATOR_ENDS_WITH => [$column, 'LIKE', "%{$this->value[0]}"], + self::OPERATOR_IS => [$column, '=', $this->value[0]], + self::OPERATOR_IS_NOT => [$column, '<>', $this->value[0]], + self::OPERATOR_CONTAINS => [$column, 'LIKE', "%{$this->value[0]}%"], + self::OPERATOR_NOT_CONTAIN => [$column, 'NOT LIKE', "%{$this->value[0]}%"], + self::OPERATOR_IS_LESS_THAN => [$column, '<', $this->value[0]], + self::OPERATOR_IS_GREATER_THAN => [$column, '>', $this->value[0]], + self::OPERATOR_IS_BETWEEN => [$column, $this->value], + self::OPERATOR_NOT_IN_LAST => fn (): array => [$column, '<', now()->subDays($this->value[0])], + self::OPERATOR_IN_LAST => fn (): array => [$column, '>=', now()->subDays($this->value[0])], + ]; + + Assert::keyExists($resolvers, $this->operator); + + return is_callable($resolvers[$this->operator]) ? $resolvers[$this->operator]() : $resolvers[$this->operator]; + } }