RAWeb icon indicating copy to clipboard operation
RAWeb copied to clipboard

feat(api): add support for ULIDs to every applicable endpoint

Open wescopeland opened this issue 10 months ago • 8 comments

Implements https://github.com/RetroAchievements/api-docs/pull/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 avatar Feb 01 '25 23:02 wescopeland

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)?

Jamiras avatar Feb 02 '25 16:02 Jamiras

@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.

wescopeland avatar Feb 02 '25 22:02 wescopeland

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();

Jamiras avatar Feb 03 '25 23:02 Jamiras

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).

wescopeland avatar Feb 05 '25 00:02 wescopeland

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);
    }
}

Jamiras avatar Feb 05 '25 14:02 Jamiras

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.

wescopeland avatar Feb 05 '25 21:02 wescopeland

Seems reasonable.

Jamiras avatar Feb 06 '25 00:02 Jamiras

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 avatar Feb 08 '25 22:02 wescopeland

Docs PR (https://github.com/RetroAchievements/api-docs/pull/76) has been updated with the latest changes. Will be merged on deploy.

wescopeland avatar Feb 28 '25 22:02 wescopeland