Skip to content

File upload completion returns raw 500 (NoSuchUpload) instead of a clear error #13830

Description

@stilla

Summary

POST /v1/files/{id}/uploaded can return an unhandled HTTP 500 instead of a structured client error.

Root cause

The request body's id field (FileUploadCompleted.id / S3FileUploadCompleted.id) is used directly as the S3 UploadId in complete_multipart_upload (polar/integrations/aws/s3/service.py). This is a different value from the File's own id used in the URL path: it must be the nested upload.id returned in the POST /v1/files/ response, not the top-level File id.

Neither S3FileUploadCompleted.id/.path nor S3FileUploadMultipart.id (in polar/integrations/aws/s3/schemas.py) have an OpenAPI Field(description=...) clarifying this distinction, so it isn't visible in Swagger UI or in the generated polar-python/polar-js SDKs (Speakeasy docs/examples just show a generic id: "<id>" placeholder). This makes it easy for an integrator to reuse the File's own id, which deterministically produces botocore.errorfactory.NoSuchUpload on every completion call.

Separately, S3Service.complete_multipart_upload has no try/except ClientError (unlike sibling methods get_object_or_raise/get_head_or_raise in the same file), so any S3-side rejection (NoSuchUpload, MalformedXML, InvalidPart, etc.) bubbles up as a raw, unhandled HTTP 500 instead of a structured 4xx PolarError.

Suggested fix

  1. Wrap the boto3 call in S3Service.complete_multipart_upload in try/except ClientError, converting it to a structured client-facing error (e.g. S3FileError), consistent with the other methods in that class.
  2. Add Field(description=...) to S3FileUploadCompleted.id/.path and S3FileUploadMultipart.id explicitly stating these must be the upload.id/upload.path values from the file creation response, not the File's own id. Consider renaming to upload_id to remove the ambiguity outright.
  3. Related: S3FileUploadCompletedPart.checksum_sha256_base64 lacks the StripValidator that was added to checksum_etag in Strip whitespace from client-supplied checksum_etag on S3 upload completion #13260 to fix a similar unhandled-ClientError-500 bug. The same whitespace-stripping should be applied there for consistency.

Repro

  1. POST /v1/files/ with service: "product_media" → 201, returns { id: <file_uuid>, upload: { id: <s3_upload_id>, path, parts }, ... }.
  2. PUT the file to the signed part URL → 200, valid ETag.
  3. POST /v1/files/{id}/uploaded with body { id: <file_uuid>, path, parts } (reusing the File's own id instead of upload.id) → 500 NoSuchUpload, every time.

Using upload.id (not the File's own id) in the body's id field is the correct/working call shape; regardless, the API should surface a clear 4xx error rather than a raw 500 when the ids don't match.

Sent by @allison-polar from File upload API 500 error investigation.

Plain

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Status
    No status

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions