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

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

GitHub

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

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

What happens if either urls or sha1s is empty?

Nice :+1:

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 ();
                                             ^

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

Very tiny cosmetic change: could be a one-liner

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

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

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).

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…

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

  context ".get_from_urls" do

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

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)

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

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

Why is that begin/rescue block required?

I would not call it “work” with invalid URIs :wink:

    it "handles invalid URIs" do

:heart_eyes: