-
-
Notifications
You must be signed in to change notification settings - Fork 116
feat(api): add support for ULIDs to every applicable endpoint #3168
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
feat(api): add support for ULIDs to every applicable endpoint #3168
Conversation
Why not just use |
I like that more than the current approach in the branch. Alternatively, we could also just fold everything into existing * 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 |
|
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: |
|
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). |
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: 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 Also, if we do decide having a |
|
Maybe the approach we could take is:
'u' => ['required', new UserIdentifier()],
$user = (new FindUserByIdentifier())->execute($input['u']);With this pattern, we're mostly decoupled from both the model and the web API domain. |
|
Seems reasonable. |
|
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. |
|
Docs PR (RetroAchievements/api-docs#76) has been updated with the latest changes. Will be merged on deploy. |
Implements RetroAchievements/api-docs#76.
Generally, every endpoint in the web API that returns a username now also returns a
ULIDalongside the field to represent that user.Additionally, anything that lets a consumer query by
ufor username now also allowsifor ULID. If an endpoint already is using aniparam for something else,dis used instead for ULID.The following endpoints are affected: