Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decomposedfs fix revision download #3473

Closed
wants to merge 24 commits into from

Conversation

butonic
Copy link
Contributor

@butonic butonic commented Nov 17, 2022

We rewrote the finish upload code to use a write lock when creating and updating node metadata. This prevents some corner cases and allows us to calculate the size diff atomically for a subsequent PR where we will propagate that size diff.

We now also check the correct permissions when downloading a revision. Downloading revisions only magically works because we are using a suffix that survives being listed at the ocdav meta versions endpoint, initiating a normal file download via the CS3 api and then the dataprovider being able to directly access that version. The DownloadRevision code was never used and was in fact implemented wrong. It would always have returned an empty file.

The workaround is that we can detect revision downloads using the .REV. delimiter that we put between the nodeid and the revision timestamp. When Download detects this delimiter it will now delegate to the fixed DownloadRevision.

fixes owncloud/ocis#3868
fixes owncloud/ocis#765

I will postpone the size diff propagation to a subsequent PR, as this already changes quite a bit.

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic butonic marked this pull request as draft November 17, 2022 13:21
@butonic butonic self-assigned this Nov 17, 2022
@butonic butonic added the bug Something isn't working label Nov 17, 2022
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@lgtm-com
Copy link

lgtm-com bot commented Nov 17, 2022

This pull request introduces 1 alert when merging 150fe7d into 3dbdcc5 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@lgtm-com
Copy link

lgtm-com bot commented Nov 17, 2022

This pull request introduces 1 alert when merging 4dae4b6 into 3dbdcc5 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@lgtm-com
Copy link

lgtm-com bot commented Nov 17, 2022

This pull request introduces 1 alert when merging 11fc7c4 into 3dbdcc5 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

pkg/storage/utils/decomposedfs/tree/tree.go Outdated Show resolved Hide resolved
pkg/storage/utils/decomposedfs/upload.go Show resolved Hide resolved
pkg/storage/utils/decomposedfs/upload.go Show resolved Hide resolved
pkg/storage/utils/decomposedfs/upload.go Outdated Show resolved Hide resolved
butonic and others added 4 commits November 18, 2022 10:01
Co-authored-by: Andre Duffeck <aduffeck@users.noreply.github.com>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic butonic marked this pull request as ready for review November 18, 2022 11:54
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic butonic changed the title Decomposedfs lock parent Decomposedfs fix revision download Nov 18, 2022
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic butonic marked this pull request as draft November 18, 2022 13:10
@butonic
Copy link
Contributor Author

butonic commented Nov 18, 2022

versions can still not be downloaded, it seemst the resource id is constructed wrong ... 👀

@butonic butonic marked this pull request as ready for review November 18, 2022 13:40
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Copy link

@dragonchaser dragonchaser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, if ci gets green :)

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
…sion permission

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic
Copy link
Contributor Author

butonic commented Nov 21, 2022

ocis si is green: https://drone.owncloud.com/owncloud/ocis/16907

@butonic
Copy link
Contributor Author

butonic commented Nov 23, 2022

superseded by #3482 as it contains the necessary test fixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants