From: Tom Hughes Date: Wed, 17 Jan 2024 18:32:08 +0000 (+0000) Subject: Merge remote-tracking branch 'upstream/pull/4482' X-Git-Tag: live~822 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/4eba549b5c0c2d01d0476ceaff894db6723177ee?hp=2d358c9df2752669b591678acfd804745527f09a Merge remote-tracking branch 'upstream/pull/4482' --- diff --git a/.rubocop.yml b/.rubocop.yml index 1e18afd83..ddfb63cae 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -68,6 +68,9 @@ Rails/SkipsModelValidations: - 'db/migrate/*.rb' - 'app/controllers/users_controller.rb' +Style/ArgumentsForwarding: + Enabled: false + Style/Documentation: Enabled: false diff --git a/Gemfile.lock b/Gemfile.lock index 0135ebb87..96dea6362 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -96,8 +96,8 @@ GEM autoprefixer-rails (10.4.16.0) execjs (~> 2) aws-eventstream (1.3.0) - aws-partitions (1.877.0) - aws-sdk-core (3.190.1) + aws-partitions (1.880.0) + aws-sdk-core (3.190.2) aws-eventstream (~> 1, >= 1.3.0) aws-partitions (~> 1, >= 1.651.0) aws-sigv4 (~> 1.8) @@ -126,7 +126,7 @@ GEM bigdecimal (3.1.5) binding_of_caller (1.0.0) debug_inspector (>= 0.0.1) - bootsnap (1.17.0) + bootsnap (1.17.1) msgpack (~> 1.2) bootstrap (5.3.2) autoprefixer-rails (>= 9.1.0) @@ -153,7 +153,7 @@ GEM rack-test (>= 0.6.3) regexp_parser (>= 1.5, < 3.0) xpath (~> 3.2) - concurrent-ruby (1.2.2) + concurrent-ruby (1.2.3) config (5.1.0) deep_merge (~> 1.2, >= 1.2.1) dry-validation (~> 1.0, >= 1.0.0) @@ -289,7 +289,7 @@ GEM image_processing (1.12.2) mini_magick (>= 4.9.5, < 5) ruby-vips (>= 2.0.17, < 3) - image_size (3.3.0) + image_size (3.4.0) in_threads (1.6.0) io-console (0.7.1) irb (1.11.1) @@ -333,7 +333,7 @@ GEM mini_portile2 (2.8.5) mini_racer (0.8.0) libv8-node (~> 18.16.0.0) - minitest (5.20.0) + minitest (5.21.1) msgpack (1.7.2) multi_json (1.15.0) multi_xml (0.6.0) @@ -401,7 +401,7 @@ GEM omniauth (~> 2.0) openstreetmap-deadlock_retry (1.3.1) parallel (1.24.0) - parser (3.3.0.2) + parser (3.3.0.4) ast (~> 2.4.1) racc pg (1.5.4) @@ -485,11 +485,11 @@ GEM rouge (4.2.0) rtlcss (0.2.1) mini_racer (>= 0.6.3) - rubocop (1.59.0) + rubocop (1.60.0) json (~> 2.3) language_server-protocol (>= 3.17.0) parallel (~> 1.10) - parser (>= 3.2.2.4) + parser (>= 3.3.0.2) rainbow (>= 2.2.2, < 4.0) regexp_parser (>= 1.8, < 3.0) rexml (>= 3.2.5, < 4.0) diff --git a/app/models/user.rb b/app/models/user.rb index bc95f20c1..b04f8e2b9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -34,12 +34,13 @@ # # Indexes # -# users_auth_idx (auth_provider,auth_uid) UNIQUE -# users_display_name_idx (display_name) UNIQUE -# users_display_name_lower_idx (lower((display_name)::text)) -# users_email_idx (email) UNIQUE -# users_email_lower_idx (lower((email)::text)) -# users_home_idx (home_tile) +# users_auth_idx (auth_provider,auth_uid) UNIQUE +# users_display_name_canonical_idx (lower(NORMALIZE(display_name, NFKC))) +# users_display_name_idx (display_name) UNIQUE +# users_display_name_lower_idx (lower((display_name)::text)) +# users_email_idx (email) UNIQUE +# users_email_lower_idx (lower((email)::text)) +# users_home_idx (home_tile) # class User < ApplicationRecord @@ -95,7 +96,7 @@ class User < ApplicationRecord validates :display_name, :presence => true, :length => 3..255, :exclusion => %w[new terms save confirm confirm-email go_public reset-password forgot-password suspended] validates :display_name, :if => proc { |u| u.display_name_changed? }, - :uniqueness => { :case_sensitive => false } + :normalized_uniqueness => { :case_sensitive => false } validates :display_name, :if => proc { |u| u.display_name_changed? }, :characters => { :url_safe => true }, :whitespace => { :leading => false, :trailing => false } @@ -132,7 +133,7 @@ class User < ApplicationRecord user = find_by("email = ? OR display_name = ?", options[:username].strip, options[:username]) if user.nil? - users = where("LOWER(email) = LOWER(?) OR LOWER(display_name) = LOWER(?)", options[:username].strip, options[:username]) + users = where("LOWER(email) = LOWER(?) OR LOWER(NORMALIZE(display_name, NFKC)) = LOWER(NORMALIZE(?, NFKC))", options[:username].strip, options[:username]) user = users.first if users.count == 1 end diff --git a/app/validators/normalized_uniqueness_validator.rb b/app/validators/normalized_uniqueness_validator.rb new file mode 100644 index 000000000..eb3600c7b --- /dev/null +++ b/app/validators/normalized_uniqueness_validator.rb @@ -0,0 +1,18 @@ +class NormalizedUniquenessValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + relation = if options.fetch(:case_sensitive, true) + record.class.where("NORMALIZE(#{attribute}, NFKC) = NORMALIZE(?, NFKC)", value) + else + record.class.where("LOWER(NORMALIZE(#{attribute}, NFKC)) = LOWER(NORMALIZE(?, NFKC))", value) + end + + relation = relation.where.not(record.class.primary_key => [record.id_in_database]) if record.persisted? + + if relation.exists? + error_options = options.except(:case_sensitive) + error_options[:value] = value + + record.errors.add(attribute, :taken, **error_options) + end + end +end diff --git a/db/migrate/20231213182102_add_canonical_user_index.rb b/db/migrate/20231213182102_add_canonical_user_index.rb new file mode 100644 index 000000000..905fb3248 --- /dev/null +++ b/db/migrate/20231213182102_add_canonical_user_index.rb @@ -0,0 +1,7 @@ +class AddCanonicalUserIndex < ActiveRecord::Migration[7.1] + disable_ddl_transaction! + + def change + add_index :users, "LOWER(NORMALIZE(display_name, NFKC))", :name => "users_display_name_canonical_idx", :algorithm => :concurrently + end +end diff --git a/db/structure.sql b/db/structure.sql index 0563417cd..c9fdebbc8 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -2922,6 +2922,13 @@ CREATE INDEX user_tokens_user_id_idx ON public.user_tokens USING btree (user_id) CREATE UNIQUE INDEX users_auth_idx ON public.users USING btree (auth_provider, auth_uid); +-- +-- Name: users_display_name_canonical_idx; Type: INDEX; Schema: public; Owner: - +-- + +CREATE INDEX users_display_name_canonical_idx ON public.users USING btree (lower(NORMALIZE(display_name, NFKC))); + + -- -- Name: users_display_name_idx; Type: INDEX; Schema: public; Owner: - -- @@ -3581,6 +3588,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('23'), ('22'), ('21'), +('20231213182102'), ('20231206141457'), ('20231117170422'), ('20231101222146'), diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 4e2675a2e..d21512f2a 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -27,10 +27,13 @@ class UserTest < ActiveSupport::TestCase end def test_unique_display_name - existing_user = create(:user) - new_user = build(:user, :display_name => existing_user.display_name) - assert_not new_user.save - assert_includes new_user.errors[:display_name], "has already been taken" + create(:user, :display_name => "H\u{e9}nryIV") + + %W[H\u{e9}nryIV he\u{301}nryiv H\u{c9}nry\u2163 he\u{301}nry\u2173].each do |name| + new_user = build(:user, :display_name => name) + assert_not new_user.save + assert_includes new_user.errors[:display_name], "has already been taken" + end end def test_email_valid