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

fix(user): enforce uniqueness on display names #2528

Merged

Conversation

wescopeland
Copy link
Member

@wescopeland wescopeland commented Jun 23, 2024

SQL to Run Before Migration/Deployment:

-- reassign all display_name values to match User, which is already unique
UPDATE `UserAccounts`
SET `display_name` = `User`;

This PR adds a uniqueness constraint on UserAccounts.display_name. I don't believe it's desirable for two users to have the same display_name.

Also, the user profile route now supports navigation by display_name in addition to User.

@wescopeland wescopeland added the deployment/sql Includes SQL that needs to be run before/after deployment label Jun 23, 2024
@wescopeland wescopeland requested a review from a team June 23, 2024 19:56
@Jamiras
Copy link
Member

Jamiras commented Jul 20, 2024

From your migration SQL:

-- Set duplicate display names to NULL
UPDATE UserAccounts
SET display_name = NULL
WHERE display_name IN (

Wouldn't it be better to set display_name=User, which is already unique?

Probably doesn't really matter. The only users with display_names set are the ones that had non-ASCII characters: https://discord.com/channels/476211979464343552/1002697125081124986/1167420029113999370

app/Helpers/util/string.php Outdated Show resolved Hide resolved
resources/views/pages-legacy/userInfo.blade.php Outdated Show resolved Hide resolved
@Jamiras
Copy link
Member

Jamiras commented Jul 20, 2024

I've also updated the SQL query in the OP to match your suggestion:

UPDATE `UserAccounts`
SET `display_name` = `User`;

Don't we still want to maintain those unicode values that already exist? (where not null, and resolve uniqueness)

@wescopeland
Copy link
Member Author

wescopeland commented Jul 20, 2024

If we do, I still think we should throw out the ones that I categorized as "crazy" earlier:

DELETE FROM UserAccounts
WHERE display_name NOT REGEXP '^[[:alpha:][:digit:][:space:]_.-]+$';

then:

UPDATE `UserAccounts`
SET `display_name` = 'User'
WHERE `display_name` IS NULL;

@Jamiras
Copy link
Member

Jamiras commented Jul 25, 2024

@luchaos - Since you went through the effort of transferring the non-ASCII usernames to the display_name field, what are your thoughts on sanitizing them?

There are currently six accounts that are incompatible with a unique key on display_name:

> select User, display_name, LastLogin from UserAccounts where display_name in (select display_name from UserAccounts where display_name is not null group by display_name having count(*) > 1) order by display_name;

Five of which haven't even been used as far as I can tell.

@Jamiras Jamiras requested a review from luchaos July 25, 2024 18:25
@wescopeland
Copy link
Member Author

All display_name values in prod have been set to null: https://discord.com/channels/476211979464343552/718091467893243937/1272243405333856347

@wescopeland wescopeland removed the deployment/sql Includes SQL that needs to be run before/after deployment label Aug 31, 2024
@wescopeland
Copy link
Member Author

wescopeland commented Aug 31, 2024

Going to go ahead and merge this to get it off our plate.

I'm not sure if we want to run this SQL query in any environment:

UPDATE `UserAccounts`
SET `display_name` = `User`;

MariaDB treats NULL as unique, and we currently don't have any code to write this value on new account creation (and I'm not sure that's desirable). My thinking right now is we continue to leave all display_name values as NULL until we support username changes.

@wescopeland wescopeland merged commit ddad15a into RetroAchievements:master Aug 31, 2024
5 checks passed
@wescopeland wescopeland deleted the unique-display-names branch August 31, 2024 20:07
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