Store new dav chunks directly, no hooks #28795
Conversation
PVince81
added
the
p1-urgent
|
I've set this to p1 because triggering hooks for every chunk is likely to cause unwanted and undiscovered side effects. |
|
checksums are not properly computed now?
|
|
@DeepDiver1975 I need to look into this. Some tests were expecting checksums for every chunks and need to be adjusted. |
|
apparently there isn't even a test for new dav final move with checksum, I'll add one |
|
@DeepDiver1975 tests adjusted, see f09c06c |
|
@owncloud/qa can you guys test this with objectstore ? I expect uploaded chunks to be stored in object store by default and should still work . |
Store new dav chunks directly instead of going through File::put() which would run all hooks. Hooks are now only triggered at the end of the transfer when the future file is moved to the final file, in which case the assemble data goes through File::put() and triggers the hooks.
Don't send checksums for each chunk but for the final move. Added test for the final move. Removed test code for chunk checksum.
PVince81
force-pushed
the
newdav-directchunk
branch
from
f09c06c to
eef29c1
Compare
who are you talking about ? never heard of such person 🕴 🕶 |
| return $mtime; | ||
| } | ||
|
|
||
| public function getView() { |
There was a problem hiding this comment.
I'm not sure if we should expose this. Taking into account that we only need one method of the view, we can wrap that method we need inside a specific method in the node. This way we don't need to expose the underlying view object.
There was a problem hiding this comment.
you mean creating a method putDirectly($path, $data) and call that from outside ?
|
stable10: #28817 |
| // TODO: verify name - should be a simple number | ||
| $this->node->createFile($name, $data); | ||
| // need to bypass hooks for individual chunks | ||
| $this->node->createFileDirectly($name, $data); |
There was a problem hiding this comment.
If the firewall is using hooks then those aren't triggered any more.
|
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. |
Description
Store new dav chunks directly instead of going through File::put() which
would run all hooks. Hooks are now only triggered at the end of the
transfer when the future file is moved to the final file, in which case
the assemble data goes through File::put() and triggers the hooks.
Note: this likely doesn't solve the problem for storage wrappers. For storage wrappers, these need to verify whether the target path is outside of "files" and bypass operation in such cases. Not part of this PR, needs to be implemented in every storage wrapper.
Related Issue
https://github.com/owncloud/enterprise/issues/2229
Motivation and Context
Prevent hooks to be fired for every chunk.
How Has This Been Tested?
Upload 100 MB file in web UI.
Before this fix: oc_activity polluted with extra entries for each chunk
After this fix: oc_activity only
Also works with encryption.
Can someone test this with object store ? @DeepDiver1975 @SergioBertolinSG
Screenshots (if appropriate):
Types of changes
Checklist: