Chunked uploader #350

carycheng · 2018-10-09T23:47:27Z

No description provided.

boxcla · 2018-10-09T23:47:29Z

Hi @carycheng, thanks for the pull request. Before we can merge it, we need you to sign our Contributor License Agreement. You can do so electronically here: http://opensource.box.com/cla

Once you have signed, just add a comment to this pull request saying, "CLA signed". Thanks!

coveralls · 2018-10-11T20:21:11Z

Pull Request Test Coverage Report for Build 1265

65 of 65 (100.0%) changed or added relevant lines in 7 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.08%) to 96.964%

Totals
Change from base Build 1260:	0.08%
Covered Lines:	2395
Relevant Lines:	2470

💛 - Coveralls

mattwiller · 2018-10-12T17:12:19Z

boxsdk/object/file.py

+        Create a new chunked upload session for uploading a new version of the file.
+
+        :param file_size:
+            The size of the file that will be uploaded.


Might be worth noting that this is in bytes

mattwiller · 2018-10-12T17:12:53Z

boxsdk/object/file.py

+        :type file_size:
+            `int`
+        :param file_name:
+            The name of the file version that will be uploaded.


This isn't totally correct; this parameter renames the entire file on upload.

mattwiller · 2018-10-12T17:16:19Z

boxsdk/object/folder.py

+        response = self._session.post(
+            url,
+            data=json.dumps(body_params),
+        ).json()


This is a bit hard to read spread across multiple lines; does it fit all on one line instead?

mattwiller · 2018-10-12T17:21:30Z

boxsdk/object/upload_session.py

+            data=content_stream
+        ).json()
+
+    def commit(self, parts, content_sha1):


This should also allow passing arbitrary file attributes and an ETag; see https://developer.box.com/v2.0/reference/#commit-upload

Also, parts can be optional — if the user doesn't pass it in you can just call the API to get the list of parts the server has and use those (if you just want to trust the server).

mattwiller · 2018-10-12T17:35:50Z

boxsdk/object/upload_session.py

+
+        range_end = min(offset + self.part_size - 1, total_size - 1)  # pylint:disable=no-member
+
+        return self._session.put(


It might be good to return just the actual part dict here, to make it easier for developers to form the correct array for commit()

Make sure that the return value of this method is just the part record, and not the weird wrapper around it

That would be a nice method on a ChunkedUploadPart object.

mattwiller · 2018-10-12T17:37:57Z

test/unit/object/test_upload_session.py

+    parts = mock_box_session.get.return_value.json.return_value = {
+        'entries': [
+            {
+                'part': {


According to the API reference, this isn't what the response looks like. Please align with the actual response format and ensure that this still works correctly

into chunked_uploader

boxsdk/object/file.py

boxsdk/object/folder.py

mattwiller · 2018-10-18T23:07:59Z

docs/file.md

+part_bytes = BytesIO(b'abcdefgh')
+upload_session = client.upload_session('11493C07ED3EABB6E59874D3A1EF3581')
+chunk = part_bytes.read(upload_session.part_size)
+offset = 32


It might be worth showing that the offset is a multiple of the part size in this example

mattwiller · 2018-10-18T23:09:14Z

docs/file.md

+
+### Manual Process
+
+For more complicated upload scenarios, such as those being coordinated across


We should remove most of this verbiage, since the automatic uploader will not be available right away. Instead, it might be nice to take the manual test code you used and put an end-to-end example here

test/unit/object/test_file.py

test/unit/object/test_upload_session.py

mattwiller · 2018-10-18T23:19:22Z

test/unit/object/test_upload_session.py

+    part = test_upload_session.upload_part(chunk, offset, total_size)
+
+    mock_box_session.put.assert_called_once_with(expected_url, data=chunk, headers=expected_headers)
+    assert part['part']['sha1'] == expected_sha1


The return value of this method should be the part object itself; this should ideally assert that:

assert part['sha1'] == expected_sha1

Also, we should assert that this is a dict

Jeff-Meadows · 2018-10-18T23:05:39Z

boxsdk/object/file.py

+        }
+        if file_name is not None:
+            body_params['file_name'] = file_name
+        url = self.get_url('{0}'.format('upload_sessions')).replace(API.BASE_API_URL, API.UPLOAD_URL)


Suggested change

url = self.get_url('{0}'.format('upload_sessions')).replace(API.BASE_API_URL, API.UPLOAD_URL)

url = self.get_url('{0}'.format('upload_sessions')).replace(

self._session.api_config.BASE_API_URL,

self._session.api_config.UPLOAD_URL,

)

boxsdk/object/folder.py

boxsdk/object/upload_session.py

Jeff-Meadows · 2018-10-18T23:35:18Z

docs/file.md

+To create an upload session for uploading a large version, use `file.create_upload_session(file_size, file_name=None)`
+
+```python
+file_size = 197520


Consider using a realistic size, since our API won't let you upload such a small file in chunks.

Jeff-Meadows · 2018-10-18T23:38:55Z

docs/file.md

+
+```python
+from io import BytesIO
+offset = 32


This example isn't likely to help a developer. The numbers don't match what the API will allow, and the code to read chunk_size bytes from a stream is unlikely to work for a real chunk size.

docs/file.md

boxsdk/object/upload_session.py

docs/file.md

Co-Authored-By: carycheng <ccheng@box.com>

into chunked_uploader

mattwiller · 2018-10-26T23:56:36Z

boxsdk/object/file.py

+        }
+        if file_name is not None:
+            body_params['file_name'] = file_name
+        url = self.get_url('upload_sessions').replace(API.BASE_API_URL, API.UPLOAD_URL)


You should use the local config here, rather than the global one, to ensure that a user's configured URLs get used.

See https://box-python-sdk.readthedocs.io/en/latest/boxsdk.session.html#boxsdk.session.session.Session.api_config

boxsdk/object/folder.py

mattwiller · 2018-10-27T00:03:05Z

boxsdk/object/upload_session.py

+        :type fields:
+            `Iterable` of `unicode`
+        :returns:
+            Returns a `list` of parts uploaded so far.


This returns a BoxObjectCollection instead of a list now

mattwiller · 2018-10-27T00:04:59Z

boxsdk/object/upload_session.py

+        :param part_content_sha1:
+            SHA-1 hash of the part's content. If not specified, this will be calculated.
+        :type part_content_sha1:
+            `unicode` or None


Is this the right type? Below you encode the hash in a way that I think requires bytes

mattwiller · 2018-10-27T00:12:27Z

boxsdk/object/upload_session.py

+        else:
+            for part in self.get_parts():
+                parts_list.append(part)
+            body['parts'] = parts_list


This should be simplified to a list comprehension:

body['parts'] = [part for part in self.get_parts()]

boxsdk/object/upload_session.py

Co-Authored-By: carycheng <ccheng@box.com>

into chunked_uploader

chunked uploads endpoint

d337058

carycheng changed the base branch from 1.6 to master October 9, 2018 23:47

added documentation for chunked upload endpoint

2b43cf9

carycheng closed this Oct 11, 2018

carycheng reopened this Oct 11, 2018

carycheng added 9 commits October 11, 2018 13:24

removed collaboration whitelist changes

0a9c9d3

changed the get_url function

9e366e3

switched session

2063aeb

fixed for linter

3c1288f

fixed for linter

0f5d349

Delete collaboration_whitelist.py

f20d03f

re-added collaboration

1defb6f

removed collaboration whitelist from this pr

8b17990

fixed for linter

714bdcf

mattwiller reviewed Oct 12, 2018

View reviewed changes

carycheng added 8 commits October 16, 2018 16:02

added paging for chunked upload parts

03acd66

Merge branch 'master' into chunked_uploader

fac2ed1

converted to for loop

6e9fe4d

Merge branch 'chunked_uploader' of https://github.com/box/box-python-sdk

077afda

into chunked_uploader

remove unused import

c21b0f3

removed unused function

837dbc2

added tests for optional params in commit

40334b5

fixed docs

a81804a

mattwiller reviewed Oct 18, 2018

View reviewed changes

Jeff-Meadows reviewed Oct 18, 2018

View reviewed changes

Matt Willer and others added 3 commits October 19, 2018 10:55

Update boxsdk/object/file.py

a118b5d

Co-Authored-By: carycheng <ccheng@box.com>

Update boxsdk/object/file.py

6e69607

Co-Authored-By: carycheng <ccheng@box.com>

Update boxsdk/object/file.py

537cd7f

Co-Authored-By: carycheng <ccheng@box.com>

carycheng added 13 commits October 25, 2018 23:18

almost working example of chunked uploader

b03c72d

working chunked uploader example in docs

e51ad8d

Merge branch 'master' into chunked_uploader

c42ad9f

added assertion for sessions

80125b1

Merge branch 'chunked_uploader' of https://github.com/box/box-python-sdk

eb5f972

into chunked_uploader

fixed translator

e11611e

fixed tests assertion errors for upload_part_bytes and translator swap

8041657

changed from list comprehension to for loop

b503921

Merge branch 'master' into chunked_uploader

75eaec9

removed unused imports

4140d0e

Merge branch 'chunked_uploader' of https://github.com/box/box-python-sdk

af80437

into chunked_uploader

removed unused import

bca4b03

removed double test

42f3c2a

mattwiller reviewed Oct 27, 2018

View reviewed changes

Matt Willer and others added 13 commits October 28, 2018 12:48

Update boxsdk/object/folder.py

1edfd8b

Co-Authored-By: carycheng <ccheng@box.com>

added docstring changes

444d444

reverted web link docs

8fc4394

reverted web link docs from chunked uploader pr

0596f65

removed unused variable

9be9f00

swapped from global config to local config

9af64d7

Merge branch 'master' into chunked_uploader

7c0734d

removed unused import

26db804

Merge branch 'chunked_uploader' of https://github.com/box/box-python-sdk

39f712c

into chunked_uploader

removed bytesio and removed unused docstring

e919010

Merge branch 'master' into chunked_uploader

4c10d04

removed unused import

1024e60

Merge branch 'chunked_uploader' of https://github.com/box/box-python-sdk

2426bb6

into chunked_uploader

mattwiller approved these changes Oct 29, 2018

View reviewed changes

carycheng merged commit edbdb08 into master Oct 29, 2018

carycheng deleted the chunked_uploader branch October 29, 2018 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chunked uploader #350

Chunked uploader #350

carycheng commented Oct 9, 2018

boxcla commented Oct 9, 2018

coveralls commented Oct 11, 2018 •

edited

mattwiller Oct 12, 2018

mattwiller Oct 12, 2018

mattwiller Oct 12, 2018

mattwiller Oct 12, 2018

mattwiller Oct 12, 2018

mattwiller Oct 18, 2018

Jeff-Meadows Oct 18, 2018

mattwiller Oct 12, 2018

mattwiller Oct 18, 2018

mattwiller Oct 18, 2018

mattwiller Oct 18, 2018

Jeff-Meadows Oct 18, 2018

Jeff-Meadows Oct 18, 2018

Jeff-Meadows Oct 18, 2018

mattwiller Oct 26, 2018

mattwiller Oct 27, 2018

mattwiller Oct 27, 2018

mattwiller Oct 27, 2018


		range_end = min(offset + self.part_size - 1, total_size - 1) # pylint:disable=no-member

		return self._session.put(


		### Manual Process

		For more complicated upload scenarios, such as those being coordinated across

Chunked uploader #350

Chunked uploader #350

Conversation

carycheng commented Oct 9, 2018

boxcla commented Oct 9, 2018

coveralls commented Oct 11, 2018 • edited

Pull Request Test Coverage Report for Build 1265

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 11, 2018 •

edited