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

"Offset does not match" error when offset matches #30

Closed
ericswpark opened this issue Jan 7, 2023 · 5 comments
Closed

"Offset does not match" error when offset matches #30

ericswpark opened this issue Jan 7, 2023 · 5 comments

Comments

@ericswpark
Copy link
Contributor

ericswpark commented Jan 7, 2023

I'm getting the following response from drf-chunked-upload:

{'detail': 'Offsets do not match', 'offset': 55000000}

The weird thing is, I am sending a chunk with the content range header start set to the offset provided:

{'Authorization': 'Token [redacted]', 'Content-Range': 'bytes 55000000-55999999/1733612877'}

I'm guessing an off-by-one mistake here on my part, but the code clearly compares the "start" regex group of the Content-Range header and the offset stored in the incomplete chunked upload:

self.is_valid_chunked_upload(chunked_upload)
if chunked_upload.offset != start:
raise ChunkedUploadError(status=status.HTTP_400_BAD_REQUEST,
detail='Offsets do not match',
offset=chunked_upload.offset)

Any idea why this would be occurring?

@jkeifer
Copy link
Owner

jkeifer commented Jan 7, 2023

Off-by-one errors are far too easy with the chunks in my experience, but I can't see where you'd be off here. That is, my reading of the code is the same as yours and I don't see why, based on what you've provided, that you'd be getting this error.

My first thought is to see if you can modify your running version of the code to make that error message a bit more informative. Maybe do:

 if chunked_upload.offset != start: 
     raise ChunkedUploadError(
          status=status.HTTP_400_BAD_REQUEST, 
          detail='Offsets do not match', 
          expected_offset=chunked_upload.offset,
          provided_offset=start,
     )

That might give us a hint at what is going wrong here.

Otherwise, I'd probably want to try to write a test replicating your request above (set the offest on an upload to 55000000 and then try to upload a chunk with the specified content range). Hopefully that would also trigger the issue, or if not that it would help indicate what else might be wrong.

One problem: I am completely inundated with several things at the moment and I will not have any possibility of spending meaningful time working on this repo for at least the next month. If you're able to provide an additional info or work through a test case to see if this can be replicated I will try to make time to respond, but I can't really commit to more than that right now.

@ericswpark
Copy link
Contributor Author

@jkeifer figured out the problem.

My client requests the list of incomplete chunked uploads using a GET request to the API endpoint. The problem is, Django returns a cached version of the API endpoint, with the old offset (from before the upload was interrupted).

I'm trying to see if manually overriding the _get function and adding a method decorator to disable caching for that specific endpoint would help. See commit: shipperstack/shipper@9e9e0d3

Also see linked issue: shipperstack/shipper#160

Should this fix be integrated into this project since many Django projects probably enable caching, or should this just be implemented on a per-project basis?

@ericswpark
Copy link
Contributor Author

Yup, the method decorator for disabling caching worked 100% and it's fixed on my end. But I think other users of this library may also run into the caching issue, so I think it might be better to add this decorator on the library-level. @jkeifer thoughts? If you think users should override based on their caching configuration I will close this issue. Thanks!

@jkeifer
Copy link
Owner

jkeifer commented Feb 17, 2023

I think this should be added to the library, as it appears to also ensure downstream caching mechanisms will not cache either (per the added headers).

jkeifer added a commit that referenced this issue Aug 13, 2023
jkeifer added a commit that referenced this issue Aug 13, 2023
jkeifer added a commit that referenced this issue Aug 13, 2023
jkeifer added a commit that referenced this issue Aug 13, 2023
@jkeifer
Copy link
Owner

jkeifer commented Aug 13, 2023

Closed by #35.

@jkeifer jkeifer closed this as completed Aug 13, 2023
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

No branches or pull requests

2 participants