DEV: Make site setting type uploaded_image_list use upload IDs (PR #10401)

[nbianca]

It used to be a list of concatenated upload URLs which was prone to break.

GitHub

[eviltrout]

I might have missed it, but is there a mechanism to migrate the old format to the new format?

[ZogStriP]

db/migrate/20200810194943_change_selectable_avatars_site_setting.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+class ChangeSelectableAvatarsSiteSetting < ActiveRecord::Migration[6.0]
+  def up
+    selectable_avatars = execute("SELECT value FROM site_settings WHERE name = 'selectable_avatars'")
+    return if selectable_avatars.cmd_tuples == 0
+
+    upload_ids = []
+    selectable_avatars.first["value"].split("\n").each do |url|
+      begin
+        upload_ids << Upload.get_from_url(url).id

That’s risky to run in a migration. Any way you can avoid calling a model’s method?

[ZogStriP]

db/migrate/20200810194943_change_selectable_avatars_site_setting.rb

@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+class ChangeSelectableAvatarsSiteSetting < ActiveRecord::Migration[6.0]
+  def up
+    selectable_avatars = execute("SELECT value FROM site_settings WHERE name = 'selectable_avatars'")
+    return if selectable_avatars.cmd_tuples == 0
+
+    # Keep old site setting value as a backup
+    execute <<~SQL
+      UPDATE site_settings
+      SET name = 'selectable_avatars_urls'
+      WHERE name = 'selectable_avatars'
+    SQL
+
+    # Extract SHA1s from URLs and then use them for upload ID lookups.
+    urls = []
+    sha1s = []
+    selectable_avatars.first["value"].split("\n").each do |url|
+      match = url.match(/(\/original\/\dX[\/\.\w]*\/(\h+)[\.\w]*)/)
+      if match.present?
+        urls << match[1]
+        sha1s << match[2]
+      else
+        STDERR.puts "Could not extract a SHA1 from #{url}"
+      end
+    end
+
+    upload_ids = execute <<~SQL
+      SELECT id
+      FROM uploads
+      WHERE url IN (#{urls.map { |url| "'#{url}'" }.join(',')})

What happens if either urls or sha1s is empty?

[ZogStriP]

db/migrate/20200810194943_change_selectable_avatars_site_setting.rb

@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+class ChangeSelectableAvatarsSiteSetting < ActiveRecord::Migration[6.0]
+  def up
+    selectable_avatars = execute("SELECT value FROM site_settings WHERE name = 'selectable_avatars'")
+    return if selectable_avatars.cmd_tuples == 0
+
+    # Keep old site setting value as a backup
+    execute <<~SQL
+      UPDATE site_settings
+      SET name = 'selectable_avatars_urls'
+      WHERE name = 'selectable_avatars'
+    SQL
+
+    # Extract SHA1s from URLs and then use them for upload ID lookups.
+    urls = []
+    sha1s = []
+    selectable_avatars.first["value"].split("\n").each do |url|
+      match = url.match(/(\/original\/\dX[\/\.\w]*\/(\h+)[\.\w]*)/)
+      if match.present?
+        urls << match[1]
+        sha1s << match[2]
+      else
+        STDERR.puts "Could not extract a SHA1 from #{url}"
+      end
+    end
+
+    upload_ids = execute <<~SQL
+      SELECT id
+      FROM uploads
+      WHERE url IN (#{urls.map { |url| "'#{url}'" }.join(',')})
+         OR sha1 IN (#{sha1s.map { |sha1| "'#{sha1}'" }.join(',')})
+    SQL
+    upload_ids = upload_ids.map { |row| row["id"] }
+    return if upload_ids.size == 0
+
+    execute <<~SQL
+      INSERT INTO site_settings(name, data_type, value, created_at, updated_at)
+      SELECT 'selectable_avatars', data_type, '#{upload_ids.join("|")}', created_at, NOW()
+      FROM site_settings
+      WHERE name = 'selectable_avatars_urls'
+    SQL
+  end
+
+  def down
+    execute("DELETE FROM site_settings WHERE name = 'selectable_avatars'")

Nice

[nbianca]

It generated an invalid query SELECT id FROM uploads WHERE url IN () OR sha1 IN (), but I fixed it.

discourse_development=# SELECT id FROM uploads WHERE url IN () OR sha1 IN ();
ERROR:  syntax error at or near ")"
LINE 1: SELECT id FROM uploads WHERE url IN () OR sha1 IN ();
                                             ^

[ZogStriP]

app/controllers/admin/site_settings_controller.rb

@@ -16,6 +16,10 @@ def update
     value.strip! if value.is_a?(String)
     raise_access_hidden_setting(id)
 
+    if SiteSetting.type_supervisor.get_type(id) == :uploaded_image_list
+      value = value.split("|").map { |url| Upload.get_from_url(url) } || []

We need to be careful about this as this will generate at least 1 request per upload in the list.

[ZogStriP]

db/migrate/20200810194943_change_selectable_avatars_site_setting.rb

@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+class ChangeSelectableAvatarsSiteSetting < ActiveRecord::Migration[6.0]
+  def up
+    selectable_avatars = execute("SELECT value FROM site_settings WHERE name = 'selectable_avatars'")
+    return if selectable_avatars.cmd_tuples == 0
+
+    # Keep old site setting value as a backup
+    execute <<~SQL
+      UPDATE site_settings
+      SET name = 'selectable_avatars_urls'
+      WHERE name = 'selectable_avatars'
+    SQL
+
+    # Extract SHA1s from URLs and then use them for upload ID lookups
+    urls = []
+    sha1s = []
+    selectable_avatars.first["value"].split("\n").each do |url|
+      match = url.match(/(\/original\/\dX[\/\.\w]*\/(\h+)[\.\w]*)/)
+      if match.present?
+        urls << match[1]
+        sha1s << match[2]
+      else
+        STDERR.puts "Could not extract a SHA1 from #{url}"
+      end
+    end
+
+    # Ensure at least one URL or SHA1 exists so the query below can be valid
+    return if urls.size == 0 && sha1s.size == 0
+
+    uploads_query = []
+    uploads_query << "url IN (#{urls.map { |url| "'#{url}'" }.join(',')})" if urls.size > 0
+    uploads_query << "sha1 IN (#{sha1s.map { |sha1| "'#{sha1}'" }.join(',')})" if sha1s.size > 0
+    uploads_query = "SELECT id FROM uploads WHERE #{uploads_query.join(" OR ")}"
+
+    upload_ids = execute(uploads_query)
+    upload_ids = upload_ids.map { |row| row["id"] }

Very tiny cosmetic change: could be a one-liner

    upload_ids = execute(uploads_query).map { |row| row["id"] }

[ZogStriP]

db/migrate/20200810194943_change_selectable_avatars_site_setting.rb

@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+class ChangeSelectableAvatarsSiteSetting < ActiveRecord::Migration[6.0]
+  def up
+    selectable_avatars = execute("SELECT value FROM site_settings WHERE name = 'selectable_avatars'")
+    return if selectable_avatars.cmd_tuples == 0
+
+    # Keep old site setting value as a backup
+    execute <<~SQL
+      UPDATE site_settings
+      SET name = 'selectable_avatars_urls'
+      WHERE name = 'selectable_avatars'
+    SQL
+
+    # Extract SHA1s from URLs and then use them for upload ID lookups
+    urls = []
+    sha1s = []
+    selectable_avatars.first["value"].split("\n").each do |url|
+      match = url.match(/(\/original\/\dX[\/\.\w]*\/(\h+)[\.\w]*)/)
+      if match.present?
+        urls << match[1]
+        sha1s << match[2]
+      else
+        STDERR.puts "Could not extract a SHA1 from #{url}"
+      end
+    end
+
+    # Ensure at least one URL or SHA1 exists so the query below can be valid
+    return if urls.size == 0 && sha1s.size == 0
+
+    uploads_query = []
+    uploads_query << "url IN (#{urls.map { |url| "'#{url}'" }.join(',')})" if urls.size > 0

Is there any chances there could be a SQL injection here?

[nbianca]

What do you mean by “1 request per upload”? You are correct, it performs one database lookup (find_by) for each upload.

I can make it a bulk operation if I reimplement Upload#get_from_url (pretty much where and not find_by and match regex with all inputs).

[ZogStriP]

Exactly. If there are 10 uploads in the list, we’ll be doing at least 10 SQL queries every time we’re loading that site setting…

[nbianca]

I do not think. Just to be sure, I used ActiveRecord::Base.connection.quote as in other migrations.

[ZogStriP]

spec/models/upload_spec.rb

@@ -229,6 +229,59 @@
     end
   end
 
+  context ".get_from_url" do

  context ".get_from_urls" do

[ZogStriP]

spec/models/upload_spec.rb

@@ -229,6 +229,59 @@
     end
   end
 
+  context ".get_from_url" do
+    let(:upload) { Fabricate(:upload, sha1: "10f73034616a796dfd70177dc54b6def44c4ba6f") }
+    let(:upload2) { Fabricate(:upload, sha1: "2a7081e615f9075befd87a9a6d273935c0262cd5") }
+
+    it "works with multiple uploads" do
+      expect(Upload.get_from_urls([upload.url, upload2.url])).to contain_exactly(upload, upload2)
+    end
+
+    it "works when the file has been uploaded" do
+      expect(Upload.get_from_urls([upload.url])).to contain_exactly(upload)

Not sure this test is needed since it’s already been testing in the previous one?

[ZogStriP]

spec/models/upload_spec.rb

@@ -229,6 +229,59 @@
     end
   end
 
+  context ".get_from_url" do
+    let(:upload) { Fabricate(:upload, sha1: "10f73034616a796dfd70177dc54b6def44c4ba6f") }
+    let(:upload2) { Fabricate(:upload, sha1: "2a7081e615f9075befd87a9a6d273935c0262cd5") }
+
+    it "works with multiple uploads" do
+      expect(Upload.get_from_urls([upload.url, upload2.url])).to contain_exactly(upload, upload2)
+    end
+
+    it "works when the file has been uploaded" do
+      expect(Upload.get_from_urls([upload.url])).to contain_exactly(upload)
+    end
+
+    it "works for an extensionless URL" do
+      upload.update!(url: upload.url.sub('.png', ''))
+      expect(Upload.get_from_urls([upload.reload.url])).to contain_exactly(upload)

Feels like these two lines are doing too much. It would be clearer with more code.

      url = upload.url.sub('.png', '')
      upload.update!(url: url)
      upload.reload
      expect(Upload.get_from_urls([url])).to contain_exactly(upload)

[ZogStriP]

spec/models/upload_spec.rb

@@ -229,6 +229,59 @@
     end
   end
 
+  context ".get_from_url" do
+    let(:upload) { Fabricate(:upload, sha1: "10f73034616a796dfd70177dc54b6def44c4ba6f") }
+    let(:upload2) { Fabricate(:upload, sha1: "2a7081e615f9075befd87a9a6d273935c0262cd5") }
+
+    it "works with multiple uploads" do
+      expect(Upload.get_from_urls([upload.url, upload2.url])).to contain_exactly(upload, upload2)
+    end
+
+    it "works when the file has been uploaded" do
+      expect(Upload.get_from_urls([upload.url])).to contain_exactly(upload)
+    end
+
+    it "works for an extensionless URL" do
+      upload.update!(url: upload.url.sub('.png', ''))
+      expect(Upload.get_from_urls([upload.reload.url])).to contain_exactly(upload)
+    end
+
+    it "works with uploads with mismatched URLs" do
+      upload.update!(url: "/uploads/default/12345/971308e535305c51.png")
+      expect(Upload.get_from_urls([upload.url])).to contain_exactly(upload)
+      expect(Upload.get_from_urls(["/uploads/default/123131/971308e535305c51.png"])).to contain_exactly()

I think there is a better helper than contain_exactly() to check for empty arrays

[danielwaterworth]

app/models/concerns/has_url.rb

@@ -44,5 +44,37 @@ def get_from_url(url)
 
       result || self.find_by("url LIKE ?", "%#{data[1]}")
     end
+
+    def get_from_urls(upload_urls)
+      urls = []
+      sha1s = []
+
+      upload_urls.each do |url|
+        next if url.blank?
+
+        uri = begin
+          URI(UrlHelper.unencode(url))
+        rescue URI::Error
+        end
+
+        next if uri&.path.blank?
+        urls << uri.path
+
+        if data = extract_url(uri.path).presence
+          urls << data[1]
+          sha1s << data[2] if self.name == "Upload"
+        end
+      end
+
+      if urls.present? && sha1s.present?
+        self.where(url: urls).or(self.where(sha1: sha1s))
+      elsif urls.present?
+        self.where(url: urls)
+      elsif sha1s.present?
+        self.where(sha1: sha1s)
+      else
+        []
+      end

You could just do self.where(url: urls).or(self.where(sha1: sha1s)) in all cases.

[ZogStriP]

spec/models/upload_spec.rb

@@ -229,6 +229,59 @@
     end
   end
 
+  context ".get_from_url" do
+    let(:upload) { Fabricate(:upload, sha1: "10f73034616a796dfd70177dc54b6def44c4ba6f") }
+    let(:upload2) { Fabricate(:upload, sha1: "2a7081e615f9075befd87a9a6d273935c0262cd5") }
+
+    it "works with multiple uploads" do
+      expect(Upload.get_from_urls([upload.url, upload2.url])).to contain_exactly(upload, upload2)
+    end
+
+    it "works when the file has been uploaded" do
+      expect(Upload.get_from_urls([upload.url])).to contain_exactly(upload)
+    end
+
+    it "works for an extensionless URL" do
+      upload.update!(url: upload.url.sub('.png', ''))
+      expect(Upload.get_from_urls([upload.reload.url])).to contain_exactly(upload)
+    end
+
+    it "works with uploads with mismatched URLs" do
+      upload.update!(url: "/uploads/default/12345/971308e535305c51.png")
+      expect(Upload.get_from_urls([upload.url])).to contain_exactly(upload)
+      expect(Upload.get_from_urls(["/uploads/default/123131/971308e535305c51.png"])).to contain_exactly()
+    end
+
+    it "works with an upload with a URL containing a deep tree" do
+      upload.update!(url: Discourse.store.get_path_for("original", 16001, upload.sha1, ".#{upload.extension}"))
+      expect(Upload.get_from_urls([upload.url])).to contain_exactly(upload)
+    end
+
+    it "works when using a CDN" do
+      begin

Why is that begin/rescue block required?

[ZogStriP]

spec/models/upload_spec.rb

@@ -229,6 +229,59 @@
     end
   end
 
+  context ".get_from_url" do
+    let(:upload) { Fabricate(:upload, sha1: "10f73034616a796dfd70177dc54b6def44c4ba6f") }
+    let(:upload2) { Fabricate(:upload, sha1: "2a7081e615f9075befd87a9a6d273935c0262cd5") }
+
+    it "works with multiple uploads" do
+      expect(Upload.get_from_urls([upload.url, upload2.url])).to contain_exactly(upload, upload2)
+    end
+
+    it "works when the file has been uploaded" do
+      expect(Upload.get_from_urls([upload.url])).to contain_exactly(upload)
+    end
+
+    it "works for an extensionless URL" do
+      upload.update!(url: upload.url.sub('.png', ''))
+      expect(Upload.get_from_urls([upload.reload.url])).to contain_exactly(upload)
+    end
+
+    it "works with uploads with mismatched URLs" do
+      upload.update!(url: "/uploads/default/12345/971308e535305c51.png")
+      expect(Upload.get_from_urls([upload.url])).to contain_exactly(upload)
+      expect(Upload.get_from_urls(["/uploads/default/123131/971308e535305c51.png"])).to contain_exactly()
+    end
+
+    it "works with an upload with a URL containing a deep tree" do
+      upload.update!(url: Discourse.store.get_path_for("original", 16001, upload.sha1, ".#{upload.extension}"))
+      expect(Upload.get_from_urls([upload.url])).to contain_exactly(upload)
+    end
+
+    it "works when using a CDN" do
+      begin
+        original_asset_host = Rails.configuration.action_controller.asset_host
+        Rails.configuration.action_controller.asset_host = 'http://my.cdn.com'
+
+        expect(Upload.get_from_urls([
+          URI.join("http://my.cdn.com", upload.url).to_s
+        ])).to contain_exactly(upload)
+      ensure
+        Rails.configuration.action_controller.asset_host = original_asset_host
+      end
+    end
+
+    it "works with full URLs" do
+      expect(Upload.get_from_urls([
+        URI.join("http://discourse.some.com:3000/", upload.url).to_s
+      ])).to contain_exactly(upload)
+    end
+
+    it "works with invalid URIs" do

I would not call it “work” with invalid URIs

    it "handles invalid URIs" do

[ZogStriP]

app/controllers/admin/site_settings_controller.rb

@@ -16,6 +16,10 @@ def update
     value.strip! if value.is_a?(String)
     raise_access_hidden_setting(id)
 
+    if SiteSetting.type_supervisor.get_type(id) == :uploaded_image_list
+      value = Upload.get_from_urls(value.split("|")).to_a