Skip to content

Conversation

@wescopeland
Copy link
Member

Implements RetroAchievements/api-docs#76.

Generally, every endpoint in the web API that returns a username now also returns a ULID alongside the field to represent that user.

Additionally, anything that lets a consumer query by u for username now also allows i for ULID. If an endpoint already is using an i param for something else, d is used instead for ULID.

The following endpoints are affected:

  • API_GetAchievementOfTheWeek
  • API_GetAchievementUnlocks
  • API_GetAchievementsEarnedBetween
  • API_GetAchievementsEarnedOnDay
  • API_GetActiveClaims
  • API_GetClaims
  • API_GetComments
  • API_GetGameExtended
  • API_GetGameInfoAndUserProgress
  • API_GetGameLeaderboards
  • API_GetGameRankAndScore
  • API_GetLeaderboardEntries
  • API_GetRecentGameAwards
  • API_GetTicketData
  • API_GetTopTenUsers
  • API_GetUserAwards
  • API_GetUserClaims
  • API_GetUserCompletedGames
  • API_GetUserCompletionProgress
  • API_GetUserGameLeaderboards
  • API_GetUserGameRankAndScore
  • API_GetUserPoints
  • API_GetUserProgress
  • API_GetUserRecentAchievements
  • API_GetUserRecentlyPlayedGames
  • API_GetUserSetRequests
  • API_GetUserSummary
  • API_GetUserWantToPlayList
  • API_GetUsersFollowingMe
  • API_GetUsersIFollow

@wescopeland wescopeland requested a review from a team February 1, 2025 23:40
@Jamiras
Copy link
Member

Jamiras commented Feb 2, 2025

Additionally, anything that lets a consumer query by u for username now also allows i for ULID. If an endpoint already is using an i param for something else, d is used instead for ULID.

Why not just use ulid as the parameter name (or some other letter that's not used by any script - I think v, w, and x are available)?

@wescopeland
Copy link
Member Author

@Jamiras

Why not just use ulid as the parameter name

I like that more than the current approach in the branch. Alternatively, we could also just fold everything into existing u params, ie:

 *  API_GetAchievementsEarnedBetween - returns achievements earned by a user between two timestamps
 *    u : username or user ULID
$input = Validator::validate(Arr::wrap(request()->query()), [
    'u' => ['required', 'string', 'min:2', 'max:26'],
]);

// Try ULID first if length matches, otherwise treat as username.
$user = strlen($input['u']) === 26
    ? User::whereUlid($input['u'])->first()
    : User::whereName($input['u'])->first();

If you're not opposed to this approach, I'll make this change to work with the single u query param. I want to verify if this is agreeable first, as whatever approach we take will touch a lot of endpoint code.

@Jamiras
Copy link
Member

Jamiras commented Feb 3, 2025

My only concern with overloading the parameter is knowing which value to query. The DB field is 32 chars, but the new username validator only allows 20. The longest in the DB is also 20. As long as that holds true, we can safely assume anything that's 26 characters is a ULID.

I'm wondering if it would be reasonable to turn this into a helper function, since it'll be used by a lot of scripts:

$input = Validator::validate(Arr::wrap(request()->query()), [
    'u' => ['required', 'string', 'min:2', 'max:26'],
]);

// Try ULID first if length matches, otherwise treat as username.
$user = strlen($input['u']) === 26
    ? User::whereUlid($input['u'])->first()
    : User::whereName($input['u'])->first();

@wescopeland
Copy link
Member Author

We can probably do something like this:

/**
 * @return Builder<User>
 */
public static function whereUlidOrName(?string $identifier): Builder
{
    if ($identifier === null) {
        return static::query()->whereRaw('1 = 0');
    }

    return static::query()
        ->where(function ($query) use ($identifier) {
            $ulidLength = 26;

            if (mb_strlen($identifier) === $ulidLength) {
                $query->whereUlid($identifier);
            } else {
                $query->where('display_name', $identifier)
                    ->orWhere('User', $identifier);
            }
        });
}

If this is agreeable, I'll update all the endpoints (and their validators).

@Jamiras
Copy link
Member

Jamiras commented Feb 5, 2025

If this is agreeable, I'll update all the endpoints (and their validators).

I was kind of hoping to roll the validator in there too to avoid duplicating the definition of a user parameter 20-some times.

Something like:

function lookupApiTargetUser(): ?User
{
    $input = Validator::validate(Arr::wrap(request()->query()), [
        'u' => ['required', 'string', 'min:2', 'max:26'],
    ]);
    
    // Try ULID first if length matches, otherwise treat as username.
    return strlen($input['u']) === 26
        ? User::whereUlid($input['u'])->first()
        : User::whereName($input['u'])->first();
}

But I don't know where it would be reasonable to put it. It's not like we have an "API_helpers.php". And I don't know if it's acceptable to call Validator::validate multiple times (as most of the endpoints have other parameters to validate).

It doesn't feel right putting a whereUlidOrName on the User model, when it's an API-specific helper.

Also, if we do decide having a whereUlidOrName is desirable, I believe it can be simplified (based on the original non-helper implementation):

public static function whereUlidOrName(?string $identifier): Builder
{
    $ulidLength = 26;
    
    if (mb_strlen($identifier) === $ulidLength) {
        return $this->whereUlid($identifier);
    } else {
        return $this->whereName($identifier);
    }
}

@wescopeland
Copy link
Member Author

wescopeland commented Feb 5, 2025

Maybe the approach we could take is:

  • Create a custom validation rule we can stick on any param (similar to CtypeAlnum()), ie:
'u' => ['required', new UserIdentifier()],
  • Create an action class in the Http namespace for lookups:
$user = (new FindUserByIdentifier())->execute($input['u']);

With this pattern, we're mostly decoupled from both the model and the web API domain.

@Jamiras
Copy link
Member

Jamiras commented Feb 6, 2025

Seems reasonable.

@wescopeland
Copy link
Member Author

I've gone through each endpoint in latest and applied the pattern discussed above:

'u' => ['required', new UserIdentifier()],
$user = (new FindUserByIdentifier())->execute($input['u']);

Once this PR is approved, before merging I will update the accompanying docs PR.

@wescopeland
Copy link
Member Author

Docs PR (RetroAchievements/api-docs#76) has been updated with the latest changes. Will be merged on deploy.

@wescopeland wescopeland merged commit 6457563 into RetroAchievements:master Feb 28, 2025
9 checks passed
@wescopeland wescopeland deleted the web-api-ulids branch February 28, 2025 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants