Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faster multiplayer room participation listing #11263

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion app/Http/Controllers/Users/MultiplayerController.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ public function index($userId, $typeGroup)
'cursor' => cursor_from_params($rawParams),
'limit' => get_int($rawParams['limit'] ?? null),
'mode' => 'participated',
'sort' => 'ended',
'type_group' => $typeGroup,
'user' => $user,
];
Expand Down
8 changes: 7 additions & 1 deletion app/Libraries/DbCursorHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ class DbCursorHelper
private $sort;
private $sortName;

public function __construct($sorts, $defaultSort, $requestedSort = null)
// $prefix is only because itemToCursor will use the non-prefixed column while everything else uses the prefixed one for join queries.
public function __construct($sorts, $defaultSort, $requestedSort = null, private string $prefix = '')
{
$this->sortName = $requestedSort;
$this->sort = $sorts[$requestedSort] ?? null;
Expand Down Expand Up @@ -41,6 +42,11 @@ public function itemToCursor($item)
return $ret;
}

public function getPrefix()
{
return $this->prefix;
}

public function getSort()
{
return $this->sort;
Expand Down
42 changes: 38 additions & 4 deletions app/Models/Multiplayer/Room.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

use App\Casts\PresentString;
use App\Exceptions\InvariantException;
use App\Libraries\DbCursorHelper;
use App\Models\Beatmap;
use App\Models\Chat\Channel;
use App\Models\Model;
Expand Down Expand Up @@ -50,6 +51,13 @@ class Room extends Model
{
use Memoizes, SoftDeletes, WithDbCursorHelper;

const PARTICIPATED_SORT = [
'ended' => [
['column' => 'ends_at', 'order' => 'DESC', 'type' => 'time'],
['column' => 'room_id', 'order' => 'DESC', 'type' => 'int'],
],
];

const SORTS = [
'ended' => [
['column' => 'ends_at', 'order' => 'DESC', 'type' => 'time'],
Expand Down Expand Up @@ -149,6 +157,7 @@ public static function search(array $rawParams, ?int $maxLimit = null)
], ['null_missing' => true]);

$maxLimit ??= 250;
$mode = $params['mode'];
$user = $params['user'];
$seasonId = $params['season_id'];
$sort = $params['sort'];
Expand Down Expand Up @@ -176,15 +185,15 @@ public static function search(array $rawParams, ?int $maxLimit = null)
$query->where('category', $category);
}

switch ($params['mode']) {
switch ($mode) {
case 'all':
break;
case 'ended':
$query->ended();
$sort ??= 'ended';
break;
case 'participated':
$query->hasParticipated($user);
$query->hasParticipatedForListing($user);
break;
case 'owned':
$query->startedBy($user);
Expand All @@ -193,7 +202,9 @@ public static function search(array $rawParams, ?int $maxLimit = null)
$query->active();
}

$cursorHelper = static::makeDbCursorHelper($sort);
$cursorHelper = $mode === 'participated'
? new DbCursorHelper(static::PARTICIPATED_SORT, 'ended', 'ended', 'multiplayer_rooms_high.')
: static::makeDbCursorHelper($sort);
$query->cursorSort($cursorHelper, cursor_from_params($rawParams));

$limit = clamp($params['limit'] ?? $maxLimit, 1, $maxLimit);
Expand Down Expand Up @@ -263,14 +274,28 @@ public function scopeFeatured(Builder $query): Builder
return $query->whereIn('category', ['featured_artist', 'spotlight']);
}

public function scopeHasParticipated($query, User $user)
public function scopeHasParticipated(Builder $query, User $user)
{
return $query->whereHas(
'userHighScores',
fn ($q) => $q->where('user_id', $user->getKey()),
);
}

public function scopeHasParticipatedForListing(Builder $query, User $user)
{
$tempModel = new UserScoreAggregate();

return $query
->selectRaw("{$this->getTable()}.*, {$tempModel->qualifyColumn('room_id')}")
->join(
$tempModel->getTable(),
$tempModel->qualifyColumn('room_id'),
'=',
$this->qualifyColumn('id'),
)->where($tempModel->qualifyColumn('user_id'), $user->getKey());
}

public function scopeStartedBy($query, User $user)
{
return $query->where('user_id', $user->user_id);
Expand Down Expand Up @@ -500,6 +525,15 @@ public function recentParticipants(): array
->all();
}

public function save(array $options = [])
{
if ($this->exists && $this->isDirty('ends_at')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make more sense for this column to be called ended_at similar to scores table, and have it set when the score is set, rather than when the room is saved?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's duplicating the room's ends_at time and ordered to match the display of a room's end time, not when the user played
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that, but I'm proposing not doing that. I think it makes more sense to list based on the user's last play rather than the time the room was closed? Will make much more sense for the profile page usage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The display will need to be adjusted a bit since the part with the room state and closing time are meant to make sense together, and it would be weird if all the times started showing other of order.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dunno, I think it would still make sense to me 😅

$this->userHighScores()->update(['ends_at' => $this->ends_at]);
}

return parent::save($options);
}

public function startGame(User $host, array $rawParams)
{
priv_check_user($host, 'MultiplayerRoomCreate')->ensureCan();
Expand Down
2 changes: 2 additions & 0 deletions app/Models/Multiplayer/UserScoreAggregate.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* @property int $attempts
* @property int $completed
* @property \Carbon\Carbon $created_at
* @property \Carbon\Carbon|null $ends_at
* @property int $id
* @property int|null $last_score_id
* @property bool $in_room
Expand Down Expand Up @@ -61,6 +62,7 @@ public static function new(User $user, Room $room): self
$obj = static::lookupOrDefault($user, $room);

if (!$obj->exists) {
$obj->ends_at = $room->ends_at;
$obj->save(); // force a save now to avoid being trolled later.
$obj->recalculate();
}
Expand Down
22 changes: 11 additions & 11 deletions app/Models/Traits/WithDbCursorHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,17 @@ public static function makeDbCursorHelper(?string $sort = null)
return new DbCursorHelper(static::SORTS, static::DEFAULT_SORT, $sort);
}

private static function cursorSortExecOrder($query, array $sort)
private static function cursorSortExecOrder($query, array $sort, string $prefix)
{
foreach ($sort as $i => $sortItem) {
$orderMethod = $i === 0 ? 'reorderBy' : 'orderBy';
$query->$orderMethod($sortItem['column'], $sortItem['order']);
$query->$orderMethod("{$prefix}{$sortItem['column']}", $sortItem['order']);
}

return $query;
}

private static function cursorSortExecWhere($query, ?array $preparedCursor)
private static function cursorSortExecWhere($query, ?array $preparedCursor, string $prefix)
{
if (empty($preparedCursor)) {
return $query;
Expand All @@ -35,13 +35,13 @@ private static function cursorSortExecWhere($query, ?array $preparedCursor)
$dir = $current['order'] === 'DESC' ? '<' : '>';

if (count($preparedCursor) === 0) {
$query->where($current['column'], $dir, $current['value']);
$query->where("{$prefix}{$current['column']}", $dir, $current['value']);
} else {
$query->where($current['column'], "{$dir}=", $current['value'])
->where(function ($q) use ($current, $dir, $preparedCursor) {
return $q->where($current['column'], $dir, $current['value'])
->orWhere(function ($qq) use ($preparedCursor) {
return static::cursorSortExecWhere($qq, $preparedCursor);
$query->where("{$prefix}{$current['column']}", "{$dir}=", $current['value'])
->where(function ($q) use ($current, $dir, $prefix, $preparedCursor) {
return $q->where("{$prefix}{$current['column']}", $dir, $current['value'])
->orWhere(function ($qq) use ($prefix, $preparedCursor) {
return static::cursorSortExecWhere($qq, $preparedCursor, $prefix);
});
});
}
Expand All @@ -64,12 +64,12 @@ public function scopeCursorSort($query, $sortOrCursorHelper, $cursorOrStatic)
? $sortOrCursorHelper
: static::makeDbCursorHelper(get_string($sortOrCursorHelper));

$query = static::cursorSortExecOrder($query, $cursorHelper->getSort());
$query = static::cursorSortExecOrder($query, $cursorHelper->getSort(), $cursorHelper->getPrefix());

$cursor = $cursorOrStatic instanceof static
? $cursorHelper->itemToCursor($cursorOrStatic)
: $cursorOrStatic;

return static::cursorSortExecWhere($query, $cursorHelper->prepare($cursor));
return static::cursorSortExecWhere($query, $cursorHelper->prepare($cursor), $cursorHelper->getPrefix());
}
}
2 changes: 1 addition & 1 deletion app/Singletons/OsuAuthorize.php
Original file line number Diff line number Diff line change
Expand Up @@ -1085,7 +1085,7 @@ public function checkChatChannelJoin(?User $user, Channel $channel): string
// This check is only for when joining the channel directly; joining via the Room
// will always add the user to the channel.
if ($channel->isMultiplayer()) {
$room = Room::hasParticipated($user)->find($channel->room_id);
$room = Room::where('id', $channel->room_id)->hasParticipated($user)->first();
if ($room !== null) {
return 'ok';
}
Expand Down
11 changes: 11 additions & 0 deletions database/factories/Chat/ChannelFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use App\Exceptions\InvariantException;
use App\Models\Chat\Channel;
use App\Models\LegacyMatch\LegacyMatch;
use App\Models\Multiplayer\Room;
use App\Models\User;
use Database\Factories\Factory;

Expand All @@ -30,6 +31,16 @@ public function moderated(): static
return $this->state(['moderated' => true]);
}

public function multiplayer(): static
{
$room = Room::factory()->create();

return $this->state([
'name' => "#lazermp_{$room->getKey()}",
'type' => Channel::TYPES['multiplayer'],
]);
}

public function pm(User ...$users): static
{
if (empty($users)) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php

// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the GNU Affero General Public License v3.0.
// See the LICENCE file in the repository root for full licence text.

declare(strict_types=1);

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

return new class extends Migration
{
/**
* Run the migrations.
*/
public function up(): void
{
Schema::table('multiplayer_rooms_high', function (Blueprint $table) {
$table->timestampTz('ends_at')->nullable();
$table->index([DB::raw('ends_at DESC'), DB::raw('room_id DESC'), 'user_id'], 'participated_rooms');
});
}

/**
* Reverse the migrations.
*/
public function down(): void
{
Schema::table('multiplayer_rooms_high', function (Blueprint $table) {
$table->dropColumn('ends_at');
$table->dropIndex('participated_rooms');
});
}
};
42 changes: 42 additions & 0 deletions tests/Controllers/Chat/ChannelsControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use App\Libraries\UserChannelList;
use App\Models\Chat\Channel;
use App\Models\Chat\Message;
use App\Models\Multiplayer\Room;
use App\Models\Multiplayer\ScoreLink;
use App\Models\Multiplayer\UserScoreAggregate;
use App\Models\User;
Expand Down Expand Up @@ -150,6 +151,39 @@ public function testChannelJoin($type, $success)
}
}

/**
* @dataProvider channelJoinMultiplayerDataProvider
*/
public function testChannelJoinMultiplayer(bool $participated)
{
$channel = Channel::factory()->multiplayer()->create();
$status = $participated ? 200 : 403;

if ($participated) {
$room = Room::findOrFail($channel->room_id);
$room->userHighScores()->save(new UserScoreAggregate(['user_id' => $this->user->getKey()]));
}

$this->actAsScopedUser($this->user, ['*']);

$this->getAssertableChannelList($this->user)
->assertMissing(['channel_id' => $channel->getKey()]);

$request = $this->json('PUT', route('api.chat.channels.join', [
'channel' => $channel->getKey(),
'user' => $this->user->getKey(),
]))->assertStatus($status);

if ($participated) {
$request->assertJsonFragment(['channel_id' => $channel->getKey()]);
$this->getAssertableChannelList($this->user)
->assertFragment(['channel_id' => $channel->getKey()]);
} else {
$this->getAssertableChannelList($this->user)
->assertMissing(['channel_id' => $channel->getKey()]);
}
}

//region PUT /chat/channels/[channel_id]/users/[user_id] - Join Channel (public)
public function testChannelJoinPublicWhenGuest() // fail
{
Expand Down Expand Up @@ -378,6 +412,14 @@ public function testChannelLeavePublicWhenGuest() // fail

//endregion

public static function channelJoinMultiplayerDataProvider()
{
return [
[true],
[false],
];
}

public static function dataProvider()
{
return [
Expand Down