FIX: Validate upload is still valid after calling the "before_upload_creation" event (PR #13091)

We’ll end up with a saved upload and a file that wasn’t uploaded if a “before_upload_creation” callback raises an exception.

GitHub

Looks good but we shouldn’t merge until after release.

Something about this API is still not feeling right to me.

Sometimes we raise an exception on upload creation. Sometimes we do not.

Later on we use save! to create the upload.

Feels to me like we should be consistent here. Any issues creating an upload should raise. Never return a broken / unfinished upload from the upload creation API.

Thoughts @romanrizzi ?

I understand your concerns and would also like to be as consistent as possible here.

Later on we use save! to create the upload.

We already know that the upload is valid because of the first save, so calling save! later never raises an exception. It looks like every place where we call this method checks if the record was persisted or contains errors to return to the client, so I think being consistent means using the non-bang version.

Sometimes we raise an exception on upload creation. Sometimes we do not.

Raising an exception was the only way I could find to return an error if the discourse-antivirus scan failed. We care about the upload being valid to be sure that we can safely stream the file to ClamAV, but we don’t want to save it yet. A possible solution would be moving the callback before the first save and let the callback check if the upload is valid. Something like:

on(:before_upload_creation) do |file, upload, is_image, for_export|
  if upload.valid?
    ....
   if found_virus?
      ...
      raise exception
    end
  end
end	 

I also thought about moving this check to an Upload validation callback but we won’t have access to the file at that point.

We have some conflicts, I guess we merge this for now and consider re-working the upload creator longer term.

The title of this pull request changed from “FIX: Use #valid? to validate an upload instead of #save.” to "FIX: Validate upload is still valid after calling the “before_upload_creation” event

@SamSaffron Apologies for bringing this up again, but you’re right about this being super inconsistent, and it doesn’t felt right to merge as it is.

I’ve been thinking about how plugins can perform additional validations on the file and the upload without relying on exceptions. I just pushed a new change that triggers the callback and checks if any errors were attached to the upload before saving. In conjunction with this change, I also modified the antivirus plugin callback in DEV: Add errors to the upload instead of raising an exception. by romanrizzi · Pull Request #11 · discourse/discourse-antivirus · GitHub