Pull Request Test Coverage Report for Build 14313605139

Details

• 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.002%) to 93.609%

Totals
Change from base Build 13813583754:	0.002%
Covered Lines:	3530
Relevant Lines:	3771

💛 - Coveralls

Hi! For my understanding, I have a couple conceptual questions about this particular change:

1. In our application, the streams we pass to the upload() calls are actual open files, so random access is possible. Could we add an option to have requests rewind the stream and reread it from the start, so that the content can be read twice without having to preload it into memory? Or would that require code changes in requests itself?
2. Is it possible to make the same change as this one to chunked upload sessions, so that chunk uploads can also follow 307 redirects successfully?

Hi,

1. It would be possible, but we would have to disable auto redirects in requests library and handle all redirects manually. I'm not sure if we want to make such a big change in our SDK
2. Chunks of a file are loaded into memory already in case retry of a chunk is necessary. Did you test it and it didn't work this way?

1. In our application, the streams we pass to the upload() calls are actual open files, so random access is possible. Could we add an option to have requests rewind the stream and reread it from the start, so that the content can be read twice without having to preload it into memory? Or would that require code changes in requests itself?

1. It would be possible, but we would have to disable auto redirects in requests library and handle all redirects manually. I'm not sure if we want to make such a big change in our SDK

That makes sense. Actually, after reconsidering, I'm not sure I agree here. requests already knows how to rewind the open file object by default when re-POST-ing after a redirect (psf/requests#3655), as @nicolasferreiralozano pointed out in the original issue #887. So I don't think it's necessary for box-python-sdk to handle redirects manually in order to do the rewind? Perhaps the use of requests-toolbelt to perform the streaming from the open file object makes it harder, though.

2. Is it possible to make the same change as this one to chunked upload sessions, so that chunk uploads can also follow 307 redirects successfully?

2. Chunks of a file are loaded into memory already in case retry of a chunk is necessary. Did you test it and it didn't work this way?

Sorry, my mistake - please feel free to ignore my point 2. Our application actually bypasses the UploadSession.upload_part_bytes() method, which behaves as you describe, and instead reaches into the BoxSession to call PUT at a lower level. After a closer reading, it appears to me that our app does this precisely to avoid having to pre-load the chunk into memory. We will have to undo that behavior ourselves.

1. When setting stream_file_content to False SDK rollbacks to the default requests library behaviour. I learnt from docs that it keeps body in the memory, but I haven't verified it in any way. So if you say that requests rewinds the stream by its own then I guess setting stream_file_content to False should be the solution for you. So is the change fine to be merged ?

1. When setting stream_file_content to False SDK rollbacks to the default requests library behaviour. I learnt from docs that it keeps body in the memory, but I haven't verified it in any way. So if you say that requests rewinds the stream by its own then I guess setting stream_file_content to False should be the solution for you. So is the change fine to be merged ?

It's possible that setting stream_file_content=False will indeed rewind the stream even with the SDK's particular configurations, but I feel like in that case, calling the parameter stream_file_content is more confusing than helpful.

Unfortunately, I have not had time to try to plug in this change into our app to see if it works. Hence, I would not support or oppose merging this change as is, but I defer to your judgement.