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

Search account domain in lowercase #13016

Merged
merged 3 commits into from
Feb 1, 2020

Conversation

abcang
Copy link
Contributor

@abcang abcang commented Jan 31, 2020

I noticed that the validation query before saving the remote account did not work with the index.

SELECT  1 AS one FROM "accounts" WHERE "accounts"."username" = 'abcang' AND "accounts"."domain" = 'pawoo.net' LIMIT 1
Limit  (cost=0.00..12043.92 rows=1 width=0) (actual time=336.947..336.948 rows=1 loops=1)
  ->  Seq Scan on accounts  (cost=0.00..12043.92 rows=1 width=0) (actual time=336.945..336.945 rows=1 loops=1)
        Filter: (((username)::text = 'abcang'::text) AND ((domain)::text = 'pawoo.net'::text))
        Rows Removed by Filter: 34062
Planning time: 0.191 ms
Execution time: 337.147 ms

Since the index of the account is as follows, it is necessary to compare it with lower("accounts"." username") and lower("accounts"."domain") to use index.

t.index "lower((username)::text), lower((domain)::text)", name: "index_accounts_on_username_and_domain_lower", unique: true

As a result of the fix, the index is now used correctly.

SELECT  1 AS one FROM "accounts" WHERE LOWER("accounts"."username") = 'abcang' AND LOWER("accounts"."domain") = 'pawoo.net' LIMIT 1
Limit  (cost=0.41..8.43 rows=1 width=0) (actual time=0.047..0.047 rows=1 loops=1)
  ->  Index Scan using index_accounts_on_username_and_domain_lower on accounts  (cost=0.41..8.43 rows=1 width=0) (actual time=0.046..0.046 rows=1 loops=1)
        Index Cond: ((lower((username)::text) = 'abcang'::text) AND (lower((domain)::text) = 'pawoo.net'::text))
Planning time: 5.180 ms
Execution time: 0.073 ms

Also, the remote account is validated with case_sensitive: true, but the actual unique index is not case-sensitive, so this was also fixed.

else
Account.where(Account.arel_table[:domain].lower.eq domain.to_s.downcase)
end
Account.where(Account.arel_table[:domain].lower.eq(domain.nil? ? nil : domain.to_s.downcase))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if the comparison target is nil, comparison with lower("accounts"."domain") is required to use the index.

Fabricate(:account, domain: 'にゃん', username: 'username')
account = Fabricate.build(:account, domain: 'xn--r9j5b5b', username: 'Username')
account.valid?
expect(account).not_to model_have_error_on_field(:username)
expect(account).to model_have_error_on_field(:username)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, this test was wrong. When saving the account without validating the username, an error occurred due to unique constraint of DB.

Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

Seems ok with current practices, but technically, the local part of acct URIs is supposed to be case-sensitive.

@Gargron
Copy link
Member

Gargron commented Feb 1, 2020

technically, the local part of acct URIs is supposed to be case-sensitive.

I don't think that's true? alice is not supposed to be available when Alice exists, in fact I remember us having some issues and a migration to fix them around this.

@Gargron Gargron merged commit 61a7390 into mastodon:master Feb 1, 2020
@abcang abcang deleted the search_domain_in_lowercase branch February 1, 2020 14:42
@ClearlyClaire
Copy link
Contributor

ClearlyClaire commented Feb 1, 2020

@Gargron the specs say that the local part of acct: URIs are supposed to be case-sensitive. It's just that in Mastodon we explicitly decided to go against that, to accommodate with people changing the casing of already-existing accounts manually, iirc

EDIT: but anyway, we already make that assumption in various places, so this PR shouldn't break anything that wasn't already broken

abcang added a commit to CrossGate-Pawoo/mastodon that referenced this pull request Aug 25, 2020
* Search account domain in lowercase

* fix rubocop error

* fix spec/models/account_spec.rb
abcang added a commit to CrossGate-Pawoo/mastodon that referenced this pull request Aug 25, 2020
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.

3 participants