It used to be a list of concatenated upload URLs which was prone to break.
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
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
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
it "handles invalid URIs" do