Browse Source

Make PreviewCard records reuseable between statuses (#4642)

* Make PreviewCard records reuseable between statuses

**Warning!** Migration truncates preview_cards tablec

* Allow a wider thumbnail for link preview, display it in horizontal layout (#4648)

* Delete preview cards files before truncating

* Rename old table instead of truncating it

* Add mastodon:maintenance:remove_deprecated_preview_cards

* Ignore deprecated_preview_cards in schema definition

* Fix null behaviour
Eugen Rochko 2 years ago
parent
commit
7dc5035031

+ 1
- 1
app/controllers/api/v1/statuses_controller.rb View File

@@ -29,7 +29,7 @@ class Api::V1::StatusesController < Api::BaseController
29 29
   end
30 30
 
31 31
   def card
32
-    @card = PreviewCard.find_by(status: @status)
32
+    @card = @status.preview_cards.first
33 33
 
34 34
     if @card.nil?
35 35
       render_empty

+ 7
- 2
app/javascript/mastodon/features/status/components/card.js View File

@@ -1,6 +1,7 @@
1 1
 import React from 'react';
2 2
 import ImmutablePropTypes from 'react-immutable-proptypes';
3 3
 import punycode from 'punycode';
4
+import classnames from 'classnames';
4 5
 
5 6
 const IDNA_PREFIX = 'xn--';
6 7
 
@@ -32,7 +33,7 @@ export default class Card extends React.PureComponent {
32 33
     if (card.get('image')) {
33 34
       image = (
34 35
         <div className='status-card__image'>
35
-          <img src={card.get('image')} alt={card.get('title')} className='status-card__image-image' />
36
+          <img src={card.get('image')} alt={card.get('title')} className='status-card__image-image' width={card.get('width')} height={card.get('height')} />
36 37
         </div>
37 38
       );
38 39
     }
@@ -41,8 +42,12 @@ export default class Card extends React.PureComponent {
41 42
       provider = decodeIDNA(getHostname(card.get('url')));
42 43
     }
43 44
 
45
+    const className = classnames('status-card', {
46
+      'horizontal': card.get('width') > card.get('height'),
47
+    });
48
+
44 49
     return (
45
-      <a href={card.get('url')} className='status-card' target='_blank' rel='noopener'>
50
+      <a href={card.get('url')} className={className} target='_blank' rel='noopener'>
46 51
         {image}
47 52
 
48 53
         <div className='status-card__content'>

+ 12
- 0
app/javascript/styles/components.scss View File

@@ -2057,6 +2057,18 @@ button.icon-button.active i.fa-retweet {
2057 2057
   background: lighten($ui-base-color, 8%);
2058 2058
 }
2059 2059
 
2060
+.status-card.horizontal {
2061
+  display: block;
2062
+
2063
+  .status-card__image {
2064
+    width: 100%;
2065
+  }
2066
+
2067
+  .status-card__image-image {
2068
+    border-radius: 4px 4px 0 0;
2069
+  }
2070
+}
2071
+
2060 2072
 .status-card__image-image {
2061 2073
   border-radius: 4px 0 0 4px;
2062 2074
   display: block;

+ 3
- 0
app/models/media_attachment.rb View File

@@ -142,9 +142,11 @@ class MediaAttachment < ApplicationRecord
142 142
 
143 143
   def populate_meta
144 144
     meta = {}
145
+
145 146
     file.queued_for_write.each do |style, file|
146 147
       begin
147 148
         geo = Paperclip::Geometry.from_file file
149
+
148 150
         meta[style] = {
149 151
           width: geo.width.to_i,
150 152
           height: geo.height.to_i,
@@ -155,6 +157,7 @@ class MediaAttachment < ApplicationRecord
155 157
         meta[style] = {}
156 158
       end
157 159
     end
160
+
158 161
     meta
159 162
   end
160 163
 

+ 23
- 8
app/models/preview_card.rb View File

@@ -4,16 +4,13 @@
4 4
 # Table name: preview_cards
5 5
 #
6 6
 #  id                 :integer          not null, primary key
7
-#  status_id          :integer
8 7
 #  url                :string           default(""), not null
9
-#  title              :string
10
-#  description        :string
8
+#  title              :string           default(""), not null
9
+#  description        :string           default(""), not null
11 10
 #  image_file_name    :string
12 11
 #  image_content_type :string
13 12
 #  image_file_size    :integer
14 13
 #  image_updated_at   :datetime
15
-#  created_at         :datetime         not null
16
-#  updated_at         :datetime         not null
17 14
 #  type               :integer          default("link"), not null
18 15
 #  html               :text             default(""), not null
19 16
 #  author_name        :string           default(""), not null
@@ -22,6 +19,8 @@
22 19
 #  provider_url       :string           default(""), not null
23 20
 #  width              :integer          default(0), not null
24 21
 #  height             :integer          default(0), not null
22
+#  created_at         :datetime         not null
23
+#  updated_at         :datetime         not null
25 24
 #
26 25
 
27 26
 class PreviewCard < ApplicationRecord
@@ -31,21 +30,37 @@ class PreviewCard < ApplicationRecord
31 30
 
32 31
   enum type: [:link, :photo, :video, :rich]
33 32
 
34
-  belongs_to :status
33
+  has_and_belongs_to_many :statuses
35 34
 
36
-  has_attached_file :image, styles: { original: '120x120#' }, convert_options: { all: '-quality 80 -strip' }
35
+  has_attached_file :image, styles: { original: '280x120>' }, convert_options: { all: '-quality 80 -strip' }
37 36
 
38 37
   include Attachmentable
39 38
   include Remotable
40 39
 
41
-  validates :url, presence: true
40
+  validates :url, presence: true, uniqueness: true
42 41
   validates_attachment_content_type :image, content_type: IMAGE_MIME_TYPES
43 42
   validates_attachment_size :image, less_than: 1.megabytes
44 43
 
44
+  before_save :extract_dimensions, if: :link?
45
+
45 46
   def save_with_optional_image!
46 47
     save!
47 48
   rescue ActiveRecord::RecordInvalid
48 49
     self.image = nil
49 50
     save!
50 51
   end
52
+
53
+  private
54
+
55
+  def extract_dimensions
56
+    file = image.queued_for_write[:original]
57
+
58
+    return if file.nil?
59
+
60
+    geo         = Paperclip::Geometry.from_file(file)
61
+    self.width  = geo.width.to_i
62
+    self.height = geo.height.to_i
63
+  rescue Paperclip::Errors::NotIdentifiedByImageMagickError
64
+    nil
65
+  end
51 66
 end

+ 2
- 1
app/models/status.rb View File

@@ -47,10 +47,11 @@ class Status < ApplicationRecord
47 47
   has_many :replies, foreign_key: 'in_reply_to_id', class_name: 'Status', inverse_of: :thread
48 48
   has_many :mentions, dependent: :destroy
49 49
   has_many :media_attachments, dependent: :destroy
50
+
50 51
   has_and_belongs_to_many :tags
52
+  has_and_belongs_to_many :preview_cards
51 53
 
52 54
   has_one :notification, as: :activity, dependent: :destroy
53
-  has_one :preview_card, dependent: :destroy
54 55
   has_one :stream_entry, as: :activity, inverse_of: :status
55 56
 
56 57
   validates :uri, uniqueness: true, unless: :local?

+ 60
- 40
app/services/fetch_link_card_service.rb View File

@@ -4,29 +4,45 @@ class FetchLinkCardService < BaseService
4 4
   URL_PATTERN = %r{https?://\S+}
5 5
 
6 6
   def call(status)
7
-    # Get first http/https URL that isn't local
8
-    url = parse_urls(status)
7
+    @status = status
8
+    @url    = parse_urls
9 9
 
10
-    return if url.nil?
10
+    return if @url.nil? || @status.preview_cards.any?
11 11
 
12
-    url  = url.to_s
13
-    card = PreviewCard.where(status: status).first_or_initialize(status: status, url: url)
14
-    res  = Request.new(:head, url).perform
12
+    @url = @url.to_s
15 13
 
16
-    return if res.code != 200 || res.mime_type != 'text/html'
14
+    RedisLock.acquire(lock_options) do |lock|
15
+      if lock.acquired?
16
+        @card = PreviewCard.find_by(url: @url)
17
+        process_url if @card.nil?
18
+      end
19
+    end
17 20
 
18
-    attempt_opengraph(card, url) unless attempt_oembed(card, url)
21
+    attach_card unless @card.nil?
19 22
   rescue HTTP::ConnectionError, OpenSSL::SSL::SSLError
20 23
     nil
21 24
   end
22 25
 
23 26
   private
24 27
 
25
-  def parse_urls(status)
26
-    if status.local?
27
-      urls = status.text.match(URL_PATTERN).to_a.map { |uri| Addressable::URI.parse(uri).normalize }
28
+  def process_url
29
+    @card = PreviewCard.new(url: @url)
30
+    res   = Request.new(:head, @url).perform
31
+
32
+    return if res.code != 200 || res.mime_type != 'text/html'
33
+
34
+    attempt_oembed || attempt_opengraph
35
+  end
36
+
37
+  def attach_card
38
+    @status.preview_cards << @card
39
+  end
40
+
41
+  def parse_urls
42
+    if @status.local?
43
+      urls = @status.text.match(URL_PATTERN).to_a.map { |uri| Addressable::URI.parse(uri).normalize }
28 44
     else
29
-      html  = Nokogiri::HTML(status.text)
45
+      html  = Nokogiri::HTML(@status.text)
30 46
       links = html.css('a')
31 47
       urls  = links.map { |a| Addressable::URI.parse(a['href']).normalize unless skip_link?(a) }.compact
32 48
     end
@@ -44,41 +60,41 @@ class FetchLinkCardService < BaseService
44 60
     a['rel']&.include?('tag') || a['class']&.include?('u-url')
45 61
   end
46 62
 
47
-  def attempt_oembed(card, url)
48
-    response = OEmbed::Providers.get(url)
63
+  def attempt_oembed
64
+    response = OEmbed::Providers.get(@url)
49 65
 
50
-    card.type          = response.type
51
-    card.title         = response.respond_to?(:title)         ? response.title         : ''
52
-    card.author_name   = response.respond_to?(:author_name)   ? response.author_name   : ''
53
-    card.author_url    = response.respond_to?(:author_url)    ? response.author_url    : ''
54
-    card.provider_name = response.respond_to?(:provider_name) ? response.provider_name : ''
55
-    card.provider_url  = response.respond_to?(:provider_url)  ? response.provider_url  : ''
56
-    card.width         = 0
57
-    card.height        = 0
66
+    @card.type          = response.type
67
+    @card.title         = response.respond_to?(:title)         ? response.title         : ''
68
+    @card.author_name   = response.respond_to?(:author_name)   ? response.author_name   : ''
69
+    @card.author_url    = response.respond_to?(:author_url)    ? response.author_url    : ''
70
+    @card.provider_name = response.respond_to?(:provider_name) ? response.provider_name : ''
71
+    @card.provider_url  = response.respond_to?(:provider_url)  ? response.provider_url  : ''
72
+    @card.width         = 0
73
+    @card.height        = 0
58 74
 
59
-    case card.type
75
+    case @card.type
60 76
     when 'link'
61
-      card.image = URI.parse(response.thumbnail_url) if response.respond_to?(:thumbnail_url)
77
+      @card.image = URI.parse(response.thumbnail_url) if response.respond_to?(:thumbnail_url)
62 78
     when 'photo'
63
-      card.url    = response.url
64
-      card.width  = response.width.presence  || 0
65
-      card.height = response.height.presence || 0
79
+      @card.url    = response.url
80
+      @card.width  = response.width.presence  || 0
81
+      @card.height = response.height.presence || 0
66 82
     when 'video'
67
-      card.width  = response.width.presence  || 0
68
-      card.height = response.height.presence || 0
69
-      card.html   = Formatter.instance.sanitize(response.html, Sanitize::Config::MASTODON_OEMBED)
83
+      @card.width  = response.width.presence  || 0
84
+      @card.height = response.height.presence || 0
85
+      @card.html   = Formatter.instance.sanitize(response.html, Sanitize::Config::MASTODON_OEMBED)
70 86
     when 'rich'
71 87
       # Most providers rely on <script> tags, which is a no-no
72 88
       return false
73 89
     end
74 90
 
75
-    card.save_with_optional_image!
91
+    @card.save_with_optional_image!
76 92
   rescue OEmbed::NotFound
77 93
     false
78 94
   end
79 95
 
80
-  def attempt_opengraph(card, url)
81
-    response = Request.new(:get, url).perform
96
+  def attempt_opengraph
97
+    response = Request.new(:get, @url).perform
82 98
 
83 99
     return if response.code != 200 || response.mime_type != 'text/html'
84 100
 
@@ -88,19 +104,23 @@ class FetchLinkCardService < BaseService
88 104
     detector.strip_tags = true
89 105
 
90 106
     guess = detector.detect(html, response.charset)
91
-    page = Nokogiri::HTML(html, nil, guess&.fetch(:encoding))
107
+    page  = Nokogiri::HTML(html, nil, guess&.fetch(:encoding))
92 108
 
93
-    card.type             = :link
94
-    card.title            = meta_property(page, 'og:title') || page.at_xpath('//title')&.content
95
-    card.description      = meta_property(page, 'og:description') || meta_property(page, 'description')
96
-    card.image_remote_url = meta_property(page, 'og:image') if meta_property(page, 'og:image')
109
+    @card.type             = :link
110
+    @card.title            = meta_property(page, 'og:title') || page.at_xpath('//title')&.content || ''
111
+    @card.description      = meta_property(page, 'og:description') || meta_property(page, 'description') || ''
112
+    @card.image_remote_url = meta_property(page, 'og:image') if meta_property(page, 'og:image')
97 113
 
98
-    return if card.title.blank?
114
+    return if @card.title.blank?
99 115
 
100
-    card.save_with_optional_image!
116
+    @card.save_with_optional_image!
101 117
   end
102 118
 
103 119
   def meta_property(html, property)
104 120
     html.at_xpath("//meta[@property=\"#{property}\"]")&.attribute('content')&.value || html.at_xpath("//meta[@name=\"#{property}\"]")&.attribute('content')&.value
105 121
   end
122
+
123
+  def lock_options
124
+    { redis: Redis.current, key: "fetch:#{@url}" }
125
+  end
106 126
 end

+ 2
- 0
config/environment.rb View File

@@ -3,3 +3,5 @@ require_relative 'application'
3 3
 
4 4
 # Initialize the Rails application.
5 5
 Rails.application.initialize!
6
+
7
+ActiveRecord::SchemaDumper.ignore_tables = ['deprecated_preview_cards']

+ 30
- 0
db/migrate/20170901141119_truncate_preview_cards.rb View File

@@ -0,0 +1,30 @@
1
+class TruncatePreviewCards < ActiveRecord::Migration[5.1]
2
+  def up
3
+    rename_table :preview_cards, :deprecated_preview_cards
4
+
5
+    create_table :preview_cards do |t|
6
+      t.string     :url, default: '', null: false, index: { unique: true }
7
+      t.string     :title, default: '', null: false
8
+      t.string     :description, default: '', null: false
9
+      t.attachment :image
10
+      t.integer    :type, default: 0, null: false
11
+      t.text       :html, default: '', null: false
12
+      t.string     :author_name, default: '', null: false
13
+      t.string     :author_url, default: '', null: false
14
+      t.string     :provider_name, default: '', null: false
15
+      t.string     :provider_url, default: '', null: false
16
+      t.integer    :width, default: 0, null: false
17
+      t.integer    :height, default: 0, null: false
18
+      t.timestamps
19
+    end
20
+  end
21
+
22
+  def down
23
+    if ActiveRecord::Base.connection.table_exists? 'deprecated_preview_cards'
24
+      drop_table :preview_cards
25
+      rename_table :deprecated_preview_cards, :preview_cards
26
+    else
27
+      raise ActiveRecord::IrreversibleMigration, 'Previous preview cards table has already been removed'
28
+    end
29
+  end
30
+end

+ 7
- 0
db/migrate/20170901142658_create_join_table_preview_cards_statuses.rb View File

@@ -0,0 +1,7 @@
1
+class CreateJoinTablePreviewCardsStatuses < ActiveRecord::Migration[5.1]
2
+  def change
3
+    create_join_table :preview_cards, :statuses do |t|
4
+      t.index [:status_id, :preview_card_id]
5
+    end
6
+  end
7
+end

+ 13
- 9
db/schema.rb View File

@@ -10,7 +10,7 @@
10 10
 #
11 11
 # It's strongly recommended that you check this file into your version control system.
12 12
 
13
-ActiveRecord::Schema.define(version: 20170829215220) do
13
+ActiveRecord::Schema.define(version: 20170901142658) do
14 14
 
15 15
   # These are extensions that must be enabled in order to support this database
16 16
   enable_extension "plpgsql"
@@ -224,17 +224,14 @@ ActiveRecord::Schema.define(version: 20170829215220) do
224 224
     t.index ["uid"], name: "index_oauth_applications_on_uid", unique: true
225 225
   end
226 226
 
227
-  create_table "preview_cards", id: :serial, force: :cascade do |t|
228
-    t.bigint "status_id"
227
+  create_table "preview_cards", force: :cascade do |t|
229 228
     t.string "url", default: "", null: false
230
-    t.string "title"
231
-    t.string "description"
229
+    t.string "title", default: "", null: false
230
+    t.string "description", default: "", null: false
232 231
     t.string "image_file_name"
233 232
     t.string "image_content_type"
234 233
     t.integer "image_file_size"
235 234
     t.datetime "image_updated_at"
236
-    t.datetime "created_at", null: false
237
-    t.datetime "updated_at", null: false
238 235
     t.integer "type", default: 0, null: false
239 236
     t.text "html", default: "", null: false
240 237
     t.string "author_name", default: "", null: false
@@ -243,7 +240,15 @@ ActiveRecord::Schema.define(version: 20170829215220) do
243 240
     t.string "provider_url", default: "", null: false
244 241
     t.integer "width", default: 0, null: false
245 242
     t.integer "height", default: 0, null: false
246
-    t.index ["status_id"], name: "index_preview_cards_on_status_id", unique: true
243
+    t.datetime "created_at", null: false
244
+    t.datetime "updated_at", null: false
245
+    t.index ["url"], name: "index_preview_cards_on_url", unique: true
246
+  end
247
+
248
+  create_table "preview_cards_statuses", id: false, force: :cascade do |t|
249
+    t.bigint "preview_card_id", null: false
250
+    t.bigint "status_id", null: false
251
+    t.index ["status_id", "preview_card_id"], name: "index_preview_cards_statuses_on_status_id_and_preview_card_id"
247 252
   end
248 253
 
249 254
   create_table "reports", id: :serial, force: :cascade do |t|
@@ -432,7 +437,6 @@ ActiveRecord::Schema.define(version: 20170829215220) do
432 437
   add_foreign_key "oauth_access_tokens", "oauth_applications", column: "application_id", on_delete: :cascade
433 438
   add_foreign_key "oauth_access_tokens", "users", column: "resource_owner_id", on_delete: :cascade
434 439
   add_foreign_key "oauth_applications", "users", column: "owner_id", on_delete: :cascade
435
-  add_foreign_key "preview_cards", "statuses", on_delete: :cascade
436 440
   add_foreign_key "reports", "accounts", column: "action_taken_by_account_id", on_delete: :nullify
437 441
   add_foreign_key "reports", "accounts", column: "target_account_id", on_delete: :cascade
438 442
   add_foreign_key "reports", "accounts", on_delete: :cascade

+ 23
- 0
lib/tasks/mastodon.rake View File

@@ -270,5 +270,28 @@ namespace :mastodon do
270 270
       ActiveRecord::Base.connection.execute('UPDATE media_attachments SET account_id = NULL FROM media_attachments ma LEFT JOIN accounts a ON a.id = ma.account_id WHERE media_attachments.id = ma.id AND ma.account_id IS NOT NULL AND a.id IS NULL')
271 271
       ActiveRecord::Base.connection.execute('UPDATE reports SET action_taken_by_account_id = NULL FROM reports r LEFT JOIN accounts a ON a.id = r.action_taken_by_account_id WHERE reports.id = r.id AND r.action_taken_by_account_id IS NOT NULL AND a.id IS NULL')
272 272
     end
273
+
274
+    desc 'Remove deprecated preview cards'
275
+    task remove_deprecated_preview_cards: :environment do
276
+      return unless ActiveRecord::Base.connection.table_exists? 'deprecated_preview_cards'
277
+
278
+      class DeprecatedPreviewCard < PreviewCard
279
+        self.table_name = 'deprecated_preview_cards'
280
+      end
281
+
282
+      puts 'Delete records and associated files from deprecated preview cards? [y/N]: '
283
+      confirm = STDIN.gets.chomp
284
+
285
+      if confirm.casecmp?('y')
286
+        DeprecatedPreviewCard.in_batches.destroy_all
287
+
288
+        puts 'Drop deprecated preview cards table? [y/N]: '
289
+        confirm = STDIN.gets.chomp
290
+
291
+        if confirm.casecmp?('y')
292
+          ActiveRecord::Migration.drop_table :deprecated_preview_cards
293
+        end
294
+      end
295
+    end
273 296
   end
274 297
 end

+ 3
- 3
spec/services/fetch_link_card_service_spec.rb View File

@@ -31,7 +31,7 @@ RSpec.describe FetchLinkCardService do
31 31
 
32 32
       it 'works with SJIS' do
33 33
         expect(a_request(:get, 'http://example.com/sjis')).to have_been_made.at_least_once
34
-        expect(status.preview_card.title).to eq("SJISのページ")
34
+        expect(status.preview_cards.first.title).to eq("SJISのページ")
35 35
       end
36 36
     end
37 37
 
@@ -40,7 +40,7 @@ RSpec.describe FetchLinkCardService do
40 40
 
41 41
       it 'works with SJIS even with wrong charset header' do
42 42
         expect(a_request(:get, 'http://example.com/sjis_with_wrong_charset')).to have_been_made.at_least_once
43
-        expect(status.preview_card.title).to eq("SJISのページ")
43
+        expect(status.preview_cards.first.title).to eq("SJISのページ")
44 44
       end
45 45
     end
46 46
 
@@ -49,7 +49,7 @@ RSpec.describe FetchLinkCardService do
49 49
 
50 50
       it 'works with koi8-r' do
51 51
         expect(a_request(:get, 'http://example.com/koi8-r')).to have_been_made.at_least_once
52
-        expect(status.preview_card.title).to eq("Московя начинаетъ только въ XVI ст. привлекать внимане иностранцевъ.")
52
+        expect(status.preview_cards.first.title).to eq("Московя начинаетъ только въ XVI ст. привлекать внимане иностранцевъ.")
53 53
       end
54 54
     end
55 55
   end