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


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 (
          max_size_kb: SiteSetting.max_attachment_size_kb
        status: 422

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.


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