New: MultiputUpload part2 #127

wenboyu2 · 2017-12-05T22:33:53Z

create session (initiation, error, success, retry)
upload part (initiation, error, success, retry, progress)

boxcla · 2017-12-05T22:33:58Z

Verified that @wenboyu2 has signed the CLA. Thanks for the pull request!

priyajeet · 2017-12-05T22:44:14Z

src/api/uploads/MultiputPart.js


 const PART_STATE_NOT_STARTED: 0 = 0;
 const PART_STATE_COMPUTING_DIGEST: 1 = 1;
 const PART_STATE_DIGEST_READY: 2 = 2;
 const PART_STATE_UPLOADING: 3 = 3;
 const PART_STATE_UPLOADED: 4 = 4;
+const noop = () => {};


Use lodash.noop

priyajeet · 2017-12-05T22:45:22Z

src/api/uploads/MultiputPart.js

+            numUploadRetriesPerformed: this.numUploadRetriesPerformed,
+            numDigestRetriesPerformed: this.numDigestRetriesPerformed,
+            sha256: this.sha256,
+            xhr: this.xhr.xhr,


Not sure what this would result in.
Might just be {}

Add a toJSON() method in XHR.js if you want more useful info. Then calling JSON.stringify(this.xhr) will automatically use that method.

This whole method can be the toJSON method for this class too

looks like currently in the webapp xhr field printed is always null or {}, removing that from being printed

priyajeet · 2017-12-05T22:48:46Z

src/api/uploads/MultiputPart.js

+            'Content-Type': 'application/octet-stream',
+            Digest: `sha-256=${this.sha256}`,
+            'Content-Range': `bytes ${this.offset}-${this.rangeEnd}/${this.size}`,
+            'X-Box-Client-Event-Info': JSON.stringify(clientEventInfo)


Double check if this header is whitelisted in our headers for app.box.com

Good point, it is not. @wenboyu2 you're going to need to to add this header to our CORS whitelist which I'll message you.

I always go back and forth about whether or not to remove this shortcut... This is gonna cause a site issue or at a minimum block some release someday.

I'm not sure if whitelisting is necessary as this request hits UPXY directly (and UPXY also generates the CORS response)

This is the CORS shortcut we have in nginx (since nginx responds a lot faster than PHP):

if ($http_origin ~* (^https://(.+\.)?(app|ent)\.(box\.com|boxcn\.net|boxenterprise\.net)$)) { set $cors "box"; } if ($request_method = 'OPTIONS') { set $cors "${cors}options"; } # If CORS pre-flight request has subdomain app or ent on the root domain box.com, boxcn.net, # or boxenterprise.net, handle in nginx to improve performance if ($cors = "boxoptions") { // Set CORS headers + list of allowed headers. The list of headers needs to be manually specified since not all browser support Access-Control-Allow-Headers: * yet }

This conf is included in the api.box.com nginx template, so you're maybe right that it won't get triggered if we're hitting UPXY directly.

tonyjin · 2017-12-05T23:02:24Z

src/api/uploads/MultiputPart.js

+                offset: this.offset
+            },
+            xhr_ready_state: this.xhr.xhr.readyState,
+            xhr_status_text: this.xhr.xhr.statusText


Is this just the event format directly ported over from WebApp code?

tonyjin · 2017-12-05T23:02:44Z

src/api/uploads/MultiputPart.js

+        try {
+            if (this.uploadedBytes < this.size) {
+                // Not all bytes were uploaded to the server. So upload part again.
+                throw new Error();


Add a message to this error?

tonyjin · 2017-12-05T23:03:17Z

src/api/uploads/MultiputPart.js

+                return;
+            }
+            this.consoleLog(`Part ${this.stringify()} is not available on server. Re-uploading.`);
+            throw new Error();


Add a message to the error.

tonyjin · 2017-12-05T23:04:47Z

src/util/url.js

+ * @param {mixed} value - the parameter value to be updated/added
+ * @return {string}
+ */
+function updateQueryStringParameter(uri: string, key: string, value: any): string {


Probably best to not handle query string params yourself, lots of edge cases - Preview uses jsuri, see: box/box-content-preview#405

dev/peerDep it

copied these over from the monolith. will look into jsuri which sounds like a better idea

priyajeet · 2017-12-05T23:15:35Z

src/api/uploads/BaseMultiput.js

+     * @param {function} msgFunc
+     * @return {void}
+     */
+    consoleLogFunc = (msgFunc: Function): void => {


Whats the use case of this?

@klk- do you remember why this was needed?

It looks like we use this in event/error handlers where something needs to be dynamically calculated then for logging, see: https://github.com/box/box-ui-elements/pull/127/files#diff-ea9a71cb823147e0f8100d32ad629368R203

But doesn't this function evaluates the msgFunc immediately and console.log the returned string? Why do we need to wrap it in another function?

Right, maybe this is to be consistent with consoleLog above and wrap the logic for checking whether the config allows logging. I was thinking of just having one log method that takes either a string log or a log function:

logToConsole(msgOrMsgFunc: string | Function): void => {}

Looking at all the use cases for consoleLogFunc I'm still not sure what do we get from it.

this.consoleLogFunc(() => `Upload completed: ${this.stringify()}. Parts state: ${this.getPartsState()}`);

is the same as

this.consoleLog(`Upload completed: ${this.stringify()}. Parts state: ${this.getPartsState()}`);

right?

oh wait maybe it's because getPartsState() (or whatever needs to be called to compose the message) is expensive, and the point of having consoleLogFunc is to avoid calling that unnecessarily

priyajeet · 2017-12-05T23:20:20Z

src/api/uploads/BaseMultiput.js

+     * @return {void}
+     */
+    consoleLog = (msg: string): void => {
+        if (this.config.console && window.console) {


This can be moved to Base.js, with the console boolean config being part of options. Maybe something like:

// Base.js
this.logInfo = !!options.console && !!window.console ? window.console.log || noop : noop;
this.logError = !!options.console && !!window.console ? window.console.error || noop : noop;

// Child classes
this.logInfo(some string);

In the future, might change this to emit() log or error events as opposed to console logging.
So the consumer of this API, can listen and either console.log or bind it to an onError() react prop.

thiccar · 2017-12-06T19:54:15Z

src/api/uploads/BaseMultiput.js

+    initialRetryDelayMs: 5000, // Base for exponential backoff on retries
+    maxRetryDelayMs: 60000, // Upper bound for time between retries
+    parallelism: 5, // Maximum number of parts to upload at a time
+    requestTimeoutMS: 120000, // Idle timeout on part upload, overall request timeout on other requests


You use Ms everywhere else, use it here too

thiccar · 2017-12-06T19:55:59Z

src/api/uploads/MultiputPart.js

+        });
+
+    /**
+     * Returns file part associated with this Part.


Clarify what exactly "file part" means here, it's not clear from context

thiccar · 2017-12-06T19:57:49Z

src/api/uploads/MultiputPart.js

+
+        const newUploadedBytes = parseInt(event.loaded, 10);
+        this.onProgress(this.uploadedBytes, newUploadedBytes);
+        this.uploadedBytes = newUploadedBytes;


IMO it makes sense to set the internal bookkeeping state before calling the onProgress callback

updated for clarity

thiccar · 2017-12-06T19:58:08Z

src/api/uploads/MultiputPart.js

+            return;
+        }
+
+        this.consoleLog(


consoleLogFunc?

oh good catch

thiccar · 2017-12-06T19:59:06Z

src/api/uploads/MultiputPart.js

+     * @param {number} limit - Number of parts to be listed. Optional.
+     * @return {Promise<Array<Object>>} Array of parts
+     */
+    listParts = async (partIndex: number, limit: number): Promise<Array<Object>> => {


Should this go in BaseMultiput?

hmm for now this function is only used when retrying part upload. We can move it to the base class if we need it for creating/committing session or something else, but I doubt it

Got it, yeah eventually we'd probably call this from other places when resuming an upload (not sure if we're ever planning to support that functionality), so we can move it then

thiccar · 2017-12-06T20:00:53Z

src/api/uploads/MultiputPart.js

        this.size = size;
        this.state = PART_STATE_NOT_STARTED;
        this.timing = {};
        this.uploadedBytes = 0;
+        this.data = {};
+        this.uploadUrl = `${this.uploadHost}/api/2.0/files/upload_sessions/${sessionId}`;


The uploadUrl should come from sessionEndpoints

thiccar · 2017-12-06T20:02:07Z

src/api/uploads/MultiputPart.js

+        }
+
+        this.state = PART_STATE_UPLOADED;
+        this.consoleLogFunc(() => `Upload completed: ${this.toJSON()}. Parts state: ${this.getPartsState()}`);


If you need to pass in the getPartsState function into this class just for this log line, it may not be worth it.

You mean we don't need to log the parts states here? Or are you suggesting some other approach to make an individual part aware of the big picture?

Definitely not needed, except for potential debugging purposes in the future. If you want to simplify the code to get rid of it, I'd say go for it

thiccar · 2017-12-06T20:03:59Z

src/api/uploads/__tests__/MultiputPart-test.js

+    });
+
+    describe('upload()', () => {
+        it('should noop if sha256 is not available', () => {


Trying to upload the part before the SHA-256 is ready would imply a logic error in the code, which is really bad. In this case I think it makes sense to fail more loudly (with an exception) as the expectation is that it should never happen, and we definitely want to know if it does.

thiccar · 2017-12-06T20:04:42Z

src/api/uploads/__tests__/MultiputPart-test.js

+            MultiputPartTest.upload();
+        });
+
+        it('should noop if blob is not available', () => {


Same comment here

thiccar · 2017-12-06T20:06:36Z

src/util/url.js

+ * @param {Object} queryParams
+ * @return {string}
+ */
+function updateQueryParameters(url: string, queryParams: Object): string {


Can sessionEndpoints' values all be of type Uri instead of String, so that we can just call addQueryParam/replaceQueryParam directly?

we only update the query params for commit and listParts, not sure if it's worth it. I'll consider it when we find ourselves messing with the urls more

thiccar · 2017-12-11T18:56:09Z

src/api/uploads/MultiputPart.js

+    onProgress: Function;
+    onSuccess: Function;
+    onError: Function;
+    data: Object;


Is it possible to use a more descriptive type for this variable?

thiccar · 2017-12-11T18:58:00Z

src/api/uploads/MultiputPart.js

@@ -26,38 +30,299 @@ class MultiputPart extends Base {
        | typeof PART_STATE_UPLOADED;
    timing: Object;
    uploadedBytes: number;
+    uploadUrl: string;


[minor] It's redundant to have this information in both this variable and session_endpoints, let's simplify it. SInce you use session_endpoints.list_part for the URL in implementation of listParts(), I would say just keep session_endpoints.

thiccar · 2017-12-11T18:58:44Z

src/api/uploads/MultiputPart.js

+    onError: Function;
+    data: Object;
+    config: MultiputConfig;
+    id: number;


IIRC we use strings for part IDs

priyajeet · 2017-12-12T18:49:20Z

src/api/uploads/BaseMultiput.js

+
+        this.config = config || DEFAULT_MULTIPUT_CONFIG;
+        this.sessionEndpoints = sessionEndpoints;
+        this.canConsoleLog = !!options.consoleLog && !!window.console && !!window.console.log;


Is this still needed? Since this.consoleLog will be a noop, you can just call it without check of existance.

Yes, because if you can't console log, then we won't call the function to generate the console message which could potentially be costly. see consoleLogFunc

priyajeet · 2017-12-12T18:52:21Z

src/api/uploads/MultiputPart.js

+            return;
+        }
+
+        this.consoleLogFunc(


Still confused, why are consoleLogFunc() needed?
Why can't this just be this.consoleLog(string) like line 240 below?

ah previously this message generator function had a costly function call, so we only want to evaluate that when we can actually console log something. but since it's now removed we can probably just do this.consoleLog

priyajeet · 2017-12-12T18:53:48Z

src/api/uploads/__tests__/MultiputPart-test.js

+
+            MultiputPartTest.uploadProgressHandler(event);
+
+            assert.equal(MultiputPartTest.uploadedBytes, 1);


All of our tests should use chai expectations/asserts.
So expect().to.be...

got it, I'll convert those with the next multiput PR which is coming very soon

New: MultiputUpload part2

f6a5c59

update

debbaca

priyajeet reviewed Dec 5, 2017

View reviewed changes

tonyjin reviewed Dec 5, 2017

View reviewed changes

priyajeet reviewed Dec 5, 2017

View reviewed changes

Wenbo Yu added 3 commits December 5, 2017 17:37

code review

0aa9d51

update

ccbb90a

tests for MultiputUpload

2e984c3

thiccar reviewed Dec 6, 2017

View reviewed changes

Wenbo Yu added 2 commits December 6, 2017 12:09

tests for MultiputPart

19744f3

cr

5e2d955

thiccar reviewed Dec 11, 2017

View reviewed changes

thiccar previously approved these changes Dec 11, 2017

View reviewed changes

code review

0410506

wenboyu2 dismissed thiccar’s stale review via 0410506 December 12, 2017 04:12

remove getPartsState

13f4180

priyajeet reviewed Dec 12, 2017

View reviewed changes

priyajeet approved these changes Dec 12, 2017

View reviewed changes

wenboyu2 merged commit f8c9821 into box:master Dec 12, 2017

wenboyu2 deleted the multiput-upload-2 branch December 12, 2017 19:20


		MultiputPartTest.uploadProgressHandler(event);

		assert.equal(MultiputPartTest.uploadedBytes, 1);

New: MultiputUpload part2 #127

New: MultiputUpload part2 #127

Conversation

wenboyu2 commented Dec 5, 2017

boxcla commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyajeet Dec 5, 2017 • edited Loading

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

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

wenboyu2 Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

priyajeet Dec 5, 2017 • edited Loading

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

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

priyajeet Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyajeet Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

wenboyu2 Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

priyajeet Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyajeet Dec 5, 2017 •

edited

Loading

wenboyu2 Dec 6, 2017 •

edited

Loading

priyajeet Dec 5, 2017 •

edited

Loading

priyajeet Dec 12, 2017 •

edited

Loading

priyajeet Dec 12, 2017 •

edited

Loading

wenboyu2 Dec 12, 2017 •

edited

Loading

priyajeet Dec 12, 2017 •

edited

Loading