Add embed columns to preview cards #5775
Conversation
|
codeclimate is complaining too many lines in a method, but this change just adds newlines and does not add statements. It is fine to ignore. |
| @@ -1,3 +1,3 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| StrongMigrations.start_after = 20170924022025 | |||
| StrongMigrations.start_after = 20171120000000 | |||
| end | ||
| end | ||
|
|
||
| add_column :preview_cards, :embed_url, :string, default: '', null: false |
There was a problem hiding this comment.
I think that we should use Mastodon::MigrationHelpers#add_column_with_default that can add columns without locking.
| end | ||
|
|
||
| dir.down do | ||
| execute 'UPDATE preview_cards SET url=embed_url WHERE embed_url IS NOT NULL' |
There was a problem hiding this comment.
Since embed_url is not null, it seems that all urls will be changed.
| end | ||
|
|
||
| add_column :preview_cards, :embed_url, :string, default: '', null: false | ||
| rename_column :preview_cards, :html, :embed_html |
There was a problem hiding this comment.
What is the reason for changing column names? If you change the column name, saving the preview card will fail until restarting the process.
|
c692082600268e2bd12665dd5928e6a263900993 eliminates blocking when migrating in up direction. |
|
deed5c1a79cadc9d1d8dac4c480f9eaa7e0e8492: rebased to e84fecb. |
| cleanup_concurrent_column_rename :preview_cards, :html, :embed_html | ||
| end | ||
|
|
||
| status_ids = Status.joins(:preview_cards) |
There was a problem hiding this comment.
There is no need to join Status. I think that this query is enough.
PreviewCard.where(type: 'photo').joins('JOIN preview_cards_statuses ON preview_cards_statuses.preview_card_id = preview_cards.id').pluck(:status_id)
SELECT "status_id" FROM "preview_cards" JOIN preview_cards_statuses ON preview_cards_statuses.preview_card_id = preview_cards.id WHERE "preview_cards"."type" = 1
There was a problem hiding this comment.
You are right. However, I still prefer a query without any raw SQL code. It is not performance-critical and fine to have the little overhead.
| cleanup_concurrent_column_rename :preview_cards, :height, :embed_height | ||
| cleanup_concurrent_column_rename :preview_cards, :width, :embed_width | ||
| cleanup_concurrent_column_rename :preview_cards, :html, :embed_html | ||
| execute 'UPDATE preview_cards SET url=embed_url WHERE embed_url IS NOT NULL' |
There was a problem hiding this comment.
Since embed_url is not null, it seems that all urls will be changed. I think that should be limited to photo.
akihikodaki
force-pushed
the
embed
branch
3 times, most recently
from
88b6660
to
02f8519
Compare
|
I understand embed_url (I think) but why rename html, width and height? |
|
Because all of them are for embedding while other attributes are not but kind of summary. |
|
My explanation may be a little insufficient so here is a bit more: |
Okay, I understand your reasoning, but I still believe that adding a complicated long-running migration just to improve developer-visible column naming is unnecessary. Personally, embed_width and width makes no difference to me especially when there's only one of them in the end. You could just add a code comment instead. |
| safety_assured do | ||
| add_column_with_default :preview_cards, :embed_url, :string, default: '', allow_null: false | ||
|
|
||
| rename_column_concurrently :preview_cards, :html, :embed_html |
There was a problem hiding this comment.
I want to get rid of the column renames.
akihikodaki
force-pushed
the
embed
branch
2 times, most recently
from
97197d9
to
86ac0b0
Compare
|
86ac0b047abd0b0263b8ee19cd1bad90ab617182: removed column renames and rebased to 9302369. |
This resolves the problem described with #5763 (comment).
Note that the migration included in this change rebuilds the entire
preview_cardstable and takes long.I'm not sure it is tolerated, but it is unlikely. If not, I will introduce additional changes.