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

Make custom emoji domains case insensitive #9351 #9474

Merged
merged 4 commits into from Dec 11, 2018

Conversation

Projects
None yet
2 participants
@Esteth
Copy link
Contributor

Esteth commented Dec 8, 2018

Make custom emoji domains case insensitive to fix #9531

Postgres supports CITEXT, but migrating the database from TEXT to CITEXT could require a whole table lock or an interim migration with dual writes.

Instead, it should be fine to migrate existing rows to lowercase one-by-one, input only lowercase input in future, and do comparisons against lowercase input.

This is my first pull request for mastodon and I haven't worked in Ruby in a long time - please let me know if I'm missing something!

@@ -0,0 +1,5 @@
class DowncaseCustomEmojiDomains < ActiveRecord::Migration[5.2]
def change
CustomEmoji.update_all('domain = lower(domain)')

This comment has been minimized.

@Gargron

Gargron Dec 10, 2018

Member

Use in_batches.update_all to prevent table lock ups

This comment has been minimized.

@Esteth

Esteth Dec 10, 2018

Author Contributor

Awesome, I didn't know this existed!

@@ -0,0 +1,5 @@
class DowncaseCustomEmojiDomains < ActiveRecord::Migration[5.2]

This comment has been minimized.

@Gargron

Gargron Dec 10, 2018

Member

Add disable_ddl_transaction! to use no locking around the updates.

This comment has been minimized.

@Esteth

Esteth Dec 10, 2018

Author Contributor

Thanks, I'm learning a lot about ActiveRecord migrations :)

@@ -336,9 +336,9 @@
create_table "mutes", force: :cascade do |t|
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.boolean "hide_notifications", default: true, null: false

This comment has been minimized.

@Gargron

Gargron Dec 10, 2018

Member

Not sure what this is about, the version string update is OK but this seems unnecessary, you can manually switch it around.

This comment has been minimized.

@Esteth

Esteth Dec 10, 2018

Author Contributor

Oops, this is the result of a bad merge. My bad for not seeing it!

Reverted

Don't use transactions, operate in batches.
Also revert spurious schema change.
@Esteth
Copy link
Contributor Author

Esteth left a comment

Thanks Gargron - I've forgotten so much about rails!

@@ -0,0 +1,5 @@
class DowncaseCustomEmojiDomains < ActiveRecord::Migration[5.2]

This comment has been minimized.

@Esteth

Esteth Dec 10, 2018

Author Contributor

Thanks, I'm learning a lot about ActiveRecord migrations :)

@@ -0,0 +1,5 @@
class DowncaseCustomEmojiDomains < ActiveRecord::Migration[5.2]
def change
CustomEmoji.update_all('domain = lower(domain)')

This comment has been minimized.

@Esteth

Esteth Dec 10, 2018

Author Contributor

Awesome, I didn't know this existed!

@@ -336,9 +336,9 @@
create_table "mutes", force: :cascade do |t|
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.boolean "hide_notifications", default: true, null: false

This comment has been minimized.

@Esteth

Esteth Dec 10, 2018

Author Contributor

Oops, this is the result of a bad merge. My bad for not seeing it!

Reverted

@Gargron Gargron merged commit 7d00e4e into tootsuite:master Dec 11, 2018

11 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: check-i18n Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-webui Your tests passed on CircleCI!
Details
codeclimate All good!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment