Handle OC-Total-Length in new chunking by DeepDiver1975 · Pull Request #28545 · owncloud/core

DeepDiver1975 · 2017-07-31T20:47:51Z

Description

Fix short coming of new chunking implementation -

Related Issue

fixes#26988

Motivation and Context

Behave properly ...

How Has This Been Tested?

Tested with the cli tool as shipped in apps/dav/bin

Please test on various browsers

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

PVince81 · 2017-08-01T07:35:55Z

@DeepDiver1975 a few weeks ago I was wondering whether X-OC-Mtime on the final move was considered with new chunking. A local test showed that it was considered. But now in this PR you've added extra code for this. I suspect that the reason it worked was maybe that the final MOVE does a Sabre\File::put() operation with the data which itself contains the X-OC-Mtime handling. If we leave your PR as is, we'll end up touching the file twice.

We can't easily move away from 'Sabre\File::put()` due to the part file and hooks logic there. Maybe need some kind of flag ?

PVince81

Looks good in general, some minor problem with mtime touching that happens twice.

See comments.

Integration tests will tell whether chunking still works.

Would be good to add integration tests for X-OC-Total-Size.

PVince81 · 2017-08-01T07:36:35Z

+ return;
+ }
+
+ $mTime = $this->server->httpRequest->getHeader('X-OC-mtime');


no more sanitize?

done in Node::touch

PVince81 · 2017-08-01T07:37:01Z

+ throw new BadRequest('Http header X-OC-Total-Size is missing');
+ }
+ $actualSize = $this->sourceNode->getSize();
+ if ($expectedSize != $actualSize) {


use strict !==

the header is a string and I dont want to cast it to an int due to 32/64 bit nightmare

what does PHP do internally ? whatever PHP does by itself could be even worse ?

if you insist on keeping it, please add a PHP comment to justify this exception for not respecting the coding style

DeepDiver1975 · 2017-08-01T07:50:52Z

@DeepDiver1975 a few weeks ago I was wondering whether X-OC-Mtime on the final move was considered with new chunking. A local test showed that it was considered. But now in this PR you've added extra code for this. I suspect that the reason it worked was maybe that the final MOVE does a Sabre\File::put() operation with the data which itself contains the X-OC-Mtime handling. If we leave your PR as is, we'll end up touching the file twice.

I'll retest this scenario .... THX for the hint

DeepDiver1975 · 2017-08-01T10:32:30Z

aaaaargh ... Jenkins was restarted? 💣

guruz · 2017-08-01T15:59:44Z

logic wise the code looks ok to me. I haven't tested it though and I don't know why the test fails
I guess it is ok to mandate X-OC-Total-Size. But then you could also mandate X-OC-Mtime?

DeepDiver1975 · 2017-08-02T06:52:30Z

I haven't tested it though and I don't know why the test fails

tests are failing because X-OC-Total-Size is missing in the tests.

Is X-OC-Total-Size considered mandatory?

ogoffart · 2017-08-02T07:16:33Z

Hold on! I just checked the client source and the header is called OC-Total-Length (No X-, and Length).
Also, we only sent this header from MOVE since the client 2.3.2, so the version 2.3.0 and 2.3.1 would break if we make the header mandatory. Hence it should not be mandatory.

EDIT: "Length" and not Lenght, of course

DeepDiver1975 · 2017-08-02T07:27:26Z

Hold on! I just checked the client source and the header is called OC-Total-Lenght (No X-, and Lenght).

the server code in the old chunking used OC-Total-Length - are you sure about the twisted chars?
Can we correct this? Kind of peinlich ....

ogoffart · 2017-08-02T07:36:57Z

Yes, we re-used the same header on purpose.

DeepDiver1975 · 2017-08-02T11:34:07Z

jenkins green - please test @SamuAlfageme @ogoffart @guruz

guruz · 2017-08-02T12:39:32Z

We even have the wrong changelog :-)
Size vs Length

guruz@mgmacbookpro mirall $ git grep -i 'Total-'
ChangeLog:* Improve compatibility with server 10.0 (#5691, X-OC-Total-Size)
csync/ChangeLog: * Add OC-Total-Length header for better quota handling.
src/libsync/propagateuploadng.cpp: headers["OC-Total-Length"] = QByteArray::number(_item->_size);
src/libsync/propagateuploadng.cpp: headers["OC-Total-Length"] = QByteArray::number(fileSize);
src/libsync/propagateuploadv1.cpp: headers["OC-Total-Length"] = QByteArray::number(fileSize);

mrow4a · 2017-08-03T11:38:43Z

@DeepDiver1975 will have a look

guruz · 2017-08-04T12:18:42Z

@mrow4a To clarify: The wrong size message needs to be displayed, not the "The computed checksum does not match the one received from the client.".

(So actually this issue is not sooo important as the client sends a checksum anyway ;-) )

PVince81 · 2017-08-07T14:35:07Z

restarted CI job

PVince81 · 2017-08-08T07:11:55Z

Would be good to adjust the integration tests about new chunking too to also send this header

PVince81 · 2017-08-08T07:20:49Z

Integration tests to add:

send the wrong total size and verify that the upload failed
send the correct total size and verify that the upload succeeded

DeepDiver1975 · 2017-08-10T11:26:28Z

tested with Chromium Version 60.0.3112.78

DeepDiver1975 · 2017-08-10T11:28:53Z

tested with firefox 53.0 (64-bit)

PVince81 · 2017-08-11T10:23:50Z

+ if (mtime) {
+ headers['X-OC-Mtime'] = mtime / 1000;
+ }
+ if (size) {


This would exclude the size "0" but then the chunking code isn't triggered for such sizes, so it's ok

PVince81

Code changes look fine 👍

individual-it · 2017-08-11T11:01:36Z

tested with IE & Safari both are sending "OC-Total-Length" coirrectly

PVince81 · 2017-08-11T12:16:46Z

well, and also fixes a critical bug when you upload a file with new chunking in IE11 which results in EPOCH being set as mtime

DeepDiver1975 · 2017-08-11T12:19:36Z

what about edge? does it work there as well? @phil-davis @individual-it THX

phil-davis · 2017-08-11T12:38:47Z

Edge on Win10 laptop with current August "patch Tuesday" patches all applied:
oC server on Ubuntu VM PHP dev server running core master:

Accept: */*
Accept-Encoding: gzip, deflate
Accept-Language: en-AU, en-US; q=0.8, en; q=0.6, ne-NP; q=0.4, ne; q=0.2
Cache-Control: no-cache
Connection: Keep-Alive
Content-Length: 0
Cookie: ocr6rsb60140=9m02hs1jeq22592nvbju4438j1; oc_sessionPassphrase=ba1k5GOeSzLVJVIU%2BS6OZ31lCrpxrgPSjPqHM%2F469yMMlJBSZm3xaYdbzgsuvREXO%2BzJyDOtk1n18U%2BZHPGiG4%2FwajB8tMB9u%2FmG3VXHkVXRT7xMs4qpLaIWbTZGNX6H
Destination: http://10.49.209.23:8080/remote.php/dav/files/admin/LION_Title_01_01.mp4
Host: 10.49.209.23:8080
requesttoken: ZhcOLwMYHAJnFhgJYHANTQA5EFd8WwQWfEULdS5QPmA=:MdDafpOkHr7x9CObkiu9LbJs1umGvaHS9jRzHh6Teac=
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.79 Safari/537.36 Edge/14.14393
X-OC-Mtime: NaN
X-Requested-With: XMLHttpRequest

oC server on Ubuntu VM PHP dev server running core verify-chunking-total-size branch (this code):

Accept: */*
Accept-Encoding: gzip, deflate
Accept-Language: en-AU, en-US; q=0.8, en; q=0.6, ne-NP; q=0.4, ne; q=0.2
Cache-Control: no-cache
Connection: Keep-Alive
Content-Length: 0
Cookie: ocr6rsb60140=9m02hs1jeq22592nvbju4438j1; oc_sessionPassphrase=ba1k5GOeSzLVJVIU%2BS6OZ31lCrpxrgPSjPqHM%2F469yMMlJBSZm3xaYdbzgsuvREXO%2BzJyDOtk1n18U%2BZHPGiG4%2FwajB8tMB9u%2FmG3VXHkVXRT7xMs4qpLaIWbTZGNX6H
Destination: http://10.49.209.23:8080/remote.php/dav/files/admin/LION_Title_01_01.mp4
Host: 10.49.209.23:8080
OC-Total-Length: 1443516495
requesttoken: fRcSIVFYYwJoJW0wMmVyFi0lPSd8XDcsO1wHQioCJAA=:VdXo400kGABAkV09FuXILeyIvlapr3R3gZ2u+eybLxE=
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.79 Safari/537.36 Edge/14.14393
X-Requested-With: XMLHttpRequest

OC-Total-Length is good - the size of the movie uploaded.
X-OC-Mtime: NaN has disappeared - without this PR the file gets 1 Jan 1970 date, with this PR the file gets the current date-time??? But when I do it in Chrome on Win10 (both before and with the PR) it sends X-OC-Mtime: 1487326866

PVince81 · 2017-08-11T14:19:33Z

So this is ready for merge after CI ?

@DeepDiver1975 can you prepare the stable10 PR in advance ? Thanks

DeepDiver1975 · 2017-08-11T14:59:05Z

So this is ready for merge after CI ?

yes

@DeepDiver1975 can you prepare the stable10 PR in advance ? Thanks

done

lock · 2019-08-02T21:02:15Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

DeepDiver1975 added the 3 - To Review label Jul 31, 2017

DeepDiver1975 added this to the development milestone Jul 31, 2017

DeepDiver1975 self-assigned this Jul 31, 2017

DeepDiver1975 requested review from PVince81, guruz and ogoffart July 31, 2017 20:47

PVince81 reviewed Aug 1, 2017
View reviewed changes

DeepDiver1975 force-pushed the verify-chunking-total-size branch from deb9230 to 04088caCompare August 1, 2017 07:58

DeepDiver1975 changed the title ~~Handle X-OC-Total-Size and X-OC-MTime in new chunking~~Handle X-OC-Total-Size in new chunkingAug 1, 2017

DeepDiver1975 force-pushed the verify-chunking-total-size branch from 04088ca to 1583ab8Compare August 1, 2017 07:59

DeepDiver1975 force-pushed the verify-chunking-total-size branch from 1583ab8 to 095e0e2Compare August 2, 2017 07:39

DeepDiver1975 changed the title ~~Handle X-OC-Total-Size in new chunking~~Handle OC-Total-Length in new chunkingAug 2, 2017

DeepDiver1975 added the backport-request label Aug 2, 2017

DeepDiver1975 force-pushed the verify-chunking-total-size branch from 095e0e2 to 9e31de0Compare August 10, 2017 10:23

DeepDiver1975 added 3 commits August 11, 2017 12:07

Handle OC-Total-Length in new chunking -fixes#26988
f522d1a

Add integration tests for length header on new chunking
f077743

Transmit OC-Total-Length in browser as well
87b4929

DeepDiver1975 force-pushed the verify-chunking-total-size branch from 9e31de0 to 87b4929Compare August 11, 2017 10:08

Only set X-OC-Mtime when browser provided lastModified on upload
a08ddd4

PVince81 reviewed Aug 11, 2017
View reviewed changes

PVince81 approved these changes Aug 11, 2017
View reviewed changes

PVince81 added the p1-urgent Critical issue, need to consider hotfix with just that issue label Aug 11, 2017

Add mb4 container id ....
ee33eca

DeepDiver1975 mentioned this pull request Aug 11, 2017
[stable10] Handle OC-Total-Length in new chunking #28657
Merged

DeepDiver1975 merged commit 91e148c into masterAug 11, 2017

DeepDiver1975 deleted the verify-chunking-total-size branch August 11, 2017 14:59

PVince81 removed the backport-request label Aug 15, 2017

danxuliu mentioned this pull request Dec 15, 2017
[oc] Handle OC-Total-Length in new chunking nextcloud/server#7533
Merged

ownclouders added the status/STALE label Dec 20, 2017

lockBot locked as resolved and limited conversation to collaborators Aug 2, 2019

Uh oh!

Conversation

DeepDiver1975 commented Jul 31, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

PVince81 commented Aug 1, 2017

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

PVince81Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975 commented Aug 1, 2017

Uh oh!

DeepDiver1975 commented Aug 1, 2017

Uh oh!

guruz commented Aug 1, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeepDiver1975 commented Aug 2, 2017

Uh oh!

ogoffart commented Aug 2, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeepDiver1975 commented Aug 2, 2017

Uh oh!

ogoffart commented Aug 2, 2017

Uh oh!

DeepDiver1975 commented Aug 2, 2017

Uh oh!

guruz commented Aug 2, 2017

Uh oh!

mrow4a commented Aug 3, 2017

Uh oh!

guruz commented Aug 4, 2017

Uh oh!

PVince81 commented Aug 7, 2017

Uh oh!

PVince81 commented Aug 8, 2017

Uh oh!

PVince81 commented Aug 8, 2017

Uh oh!

DeepDiver1975 commented Aug 10, 2017

Uh oh!

DeepDiver1975 commented Aug 10, 2017

Uh oh!

PVince81Aug 11, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

individual-it commented Aug 11, 2017

Uh oh!

PVince81 commented Aug 11, 2017

Uh oh!

DeepDiver1975 commented Aug 11, 2017

Uh oh!

DeepDiver1975 commented Jul 31, 2017•
edited
Loading

guruz commented Aug 1, 2017•
edited
Loading

ogoffart commented Aug 2, 2017•
edited
Loading