FEATURE: Uppy direct S3 multipart uploads in composer (PR #14051)

GitHub

Uppy changed from Plugin to BasePlugin

These todos will be resolved in another PR

I suspect we will need to tune these rate limits, I thought that these were just a good starting point.

These todos will be resolved in another PR

This one, to be precise FIX: Use file.id instead of file.name for media-optimization resolvers by martin-brennan · Pull Request #14110 · dis

Should this be the following instead?

              this._handleUploadError(file, err);

It doesn’t look like we are handling the error there.

We are, I’m trying to get across that for some of these functions, the promise error is handled and fires the upload-error event, while the other functions we have to handle it manually. It’s annoying, but if I handle it here too then the upload-error event still fires.

No; see line 365. this here refers to the prepareUploadsPart function which is inside uppy, not the composer which is what we want

    if !FileHelper.is_supported_image?(file_name) && file_size >= SiteSetting.max_attachment_size_kb.kilobytes 
      return (
        render_json_error(
          I18n.t("upload.attachments.too_large", 
          max_size_kb: SiteSetting.max_attachment_size_kb
        ), 
        status: 422
      )
    end

Not sure if the nested is required here.

Since the unique_identifier is from a param, this rate limit can easily be bypassed by passing a different unique identifier manually so I’m not sure if this rate limit will serve its purpose.

Feels odd we need to Array#map twice here. Perhaps just have validate_part_number return an integer?

    if !part_number.between?(1, 10000)

I think this condition itself is sufficient.

I’m wondering if there is a reason we need a rate limit for this action instead of just relying on the global request rate limit.

      Discourse.store.abort_multipart(

Actually do we have to check if the abort has succeeded before destroying the upload stub?

I think we should just make this action idempotent. If the external_upload_stub is not found, it is safe to mean that the record has been destroyed so we can just return a success_json.

Kind of feel like this can be extracted into a before_action that is ran on the related actions.

Not sure about mentioning part number here since it should have been caught by validate_part_number