Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uploading a large media file for draft form fails #178

Closed
yanokwa opened this issue Nov 24, 2020 · 9 comments · Fixed by getodk/central-frontend#412
Closed

Uploading a large media file for draft form fails #178

yanokwa opened this issue Nov 24, 2020 · 9 comments · Fixed by getodk/central-frontend#412

Comments

@yanokwa
Copy link
Member

yanokwa commented Nov 24, 2020

I've created a draft form with three media files (mp4s). Two of them uploaded just great. The third fails with a "Something went wrong: the server returned an invalid error.". When that third file is edited and reduced to <100MB, it uploads fine.

nginx shows this error:

nginx | 2020/11/24 01:46:22 [error] 1262634#1262634: *263811 client intended to send too large body: 112333958 bytes, client: 99.145.234.152, server: , request: "POST /v1/projects/248/forms/tree_inventory/draft/attachments/do.mp4 HTTP/1.1", host: "example.getodk.cloud", referrer: "https://example.getodk.cloud/"

@yanokwa
Copy link
Member Author

yanokwa commented Nov 24, 2020

100 MB is probably a fine limit, but maybe we can get a more helpful error message?

@matthew-white
Copy link
Member

Is there a way for Backend to return an error in this case? (I'm guessing not?) If not, then we could try to add something to Frontend. At the moment Frontend only knows how to process errors from Backend. However, Frontend also has access to the file size, so rather than waiting for an error on upload and trying to parse that response, Frontend could show an error message as soon as the file is dropped onto the page. How about:

The file "xyz" exceeds the size limit of 100 MB and cannot be uploaded.

@yanokwa
Copy link
Member Author

yanokwa commented Nov 24, 2020

I don't think we should do the upload and the fail because it's not very time and bandwidth friendly. Warning before the upload is nicer. Two concerns come to mind...

  1. What happens if someone does the upload via the API? The backend has to respond then, right?
  2. The disconnect between that limit "100 MB" and the actual hard coded limit in the nginx file

@matthew-white
Copy link
Member

matthew-white commented Nov 24, 2020

What happens if someone does the upload via the API? The backend has to respond then, right?

All API requests will pass through nginx, so right now an API user will receive the same response as Frontend. I think it'd be ideal for there to be a JSON response in this case that's in the same format as a Backend Problem, but I'm also not sure what the options are around the API and nginx. @issa-tseng, do you have a sense for what's possible in this case?

@larshelge
Copy link

larshelge commented Dec 28, 2020

Getting the file size in Javascript is fairly straight-forward with the File API (example), which can be used for client-side validation.

Max file upload size is controlled in nginx with the client_max_body_size directive. It returns the 413 (Request Entity Too Large) HTTP status code when exceeded which I think is appropriate as an API response.

@issa-tseng
Copy link
Member

@matthew-white let's see if i'm hearing you right—you want nginx's response here to look more like backend's?

@matthew-white
Copy link
Member

I'm not sure! I'd be interested to hear what you think. The request will never reach Backend, right? So we only have the nginx response? If that's the case, I'm not sure whether or not it makes sense to try to have nginx return an error in the same format as Backend. What do you think makes the most sense for API users? Should we document that this type of error is not a Backend Problem?

@lognaturel
Copy link
Member

It seems fine to me for API users to see the raw nginx response.

In the case of an upload through frontend, I do agree that it would be really helpful for frontend to detect the issue and error out before an upload. Would it make sense to pull the max size out to .env for example and plumb it through to nginx and frontend? We've so far heard of two users who have run into problems with that max so there are probably more out there.

@matthew-white
Copy link
Member

Assuming the nginx response stays the same, I think the easiest thing would be for Frontend to look out for a 413 and show a specific alert message in that case. That would involve just a small change to our existing error-to-alert system (possibly a one-liner!). I definitely see how it'd be useful to have client-side validation, but it's actually not something we generally do, due to the additional complexity. I think that's especially true in this case if it'd require plumbing through an environment variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants