-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix: catch ManuallyLockedException and use app context #37787
Conversation
249f13b
to
2e84a3f
Compare
2e84a3f
to
254c039
Compare
254c039
to
52db34b
Compare
eba9a67
to
f45f75f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo, else nice 👍
/backport to stable26 |
/backport to stable25 |
/backport to stable24 |
71cb044
to
5378aa5
Compare
if ($storage1->instanceOfStorage('\OC\Files\ObjectStore\ObjectStoreStorage') || $storage2->instanceOfStorage('\OC\Files\ObjectStore\ObjectStoreStorage')) { | ||
$source = $storage1->fopen($internalPath1, 'r'); | ||
$target = $storage2->fopen($internalPath2, 'w'); | ||
[, $result] = \OC_Helper::streamCopy($source, $target); |
Check notice
Code scanning / Psalm
PossiblyInvalidArgument
$source = $storage1->fopen($internalPath1, 'r'); | ||
$target = $storage2->fopen($internalPath2, 'w'); | ||
[, $result] = \OC_Helper::streamCopy($source, $target); | ||
fclose($source); |
Check notice
Code scanning / Psalm
PossiblyInvalidArgument
$target = $storage2->fopen($internalPath2, 'w'); | ||
[, $result] = \OC_Helper::streamCopy($source, $target); | ||
fclose($source); | ||
fclose($target); |
Check notice
Code scanning / Psalm
PossiblyInvalidArgument
$storage1->unlink($internalPath1); | ||
try { | ||
// TODO add a proper way of overwriting a file while maintaining file ids | ||
if ($storage1->instanceOfStorage('\OC\Files\ObjectStore\ObjectStoreStorage') || $storage2->instanceOfStorage('\OC\Files\ObjectStore\ObjectStoreStorage')) { |
Check notice
Code scanning / Psalm
ArgumentTypeCoercion
$storage1->unlink($internalPath1); | ||
try { | ||
// TODO add a proper way of overwriting a file while maintaining file ids | ||
if ($storage1->instanceOfStorage('\OC\Files\ObjectStore\ObjectStoreStorage') || $storage2->instanceOfStorage('\OC\Files\ObjectStore\ObjectStoreStorage')) { |
Check notice
Code scanning / Psalm
ArgumentTypeCoercion
if ($storage1->instanceOfStorage('\OC\Files\ObjectStore\ObjectStoreStorage') || $storage2->instanceOfStorage('\OC\Files\ObjectStore\ObjectStoreStorage')) { | ||
$source = $storage1->fopen($internalPath1, 'r'); | ||
$target = $storage2->fopen($internalPath2, 'w'); | ||
[, $result] = \OC_Helper::streamCopy($source, $target); |
Check notice
Code scanning / Psalm
PossiblyInvalidArgument
The files_lock app may throw ManuallyLockedExceptions when attempting to revert a file that is currently opened. This would prevent the user from rolling back a opened file. Text and Richdocuments handle changes of the file while editing. Allow reverting files even when they are locked by these apps and let the apps handle the conflict. Signed-off-by: Max <max@nextcloud.com>
5378aa5
to
337fc11
Compare
$storage1->unlink($internalPath1); | ||
try { | ||
// TODO add a proper way of overwriting a file while maintaining file ids | ||
if ($storage1->instanceOfStorage('\OC\Files\ObjectStore\ObjectStoreStorage') || $storage2->instanceOfStorage('\OC\Files\ObjectStore\ObjectStoreStorage')) { |
Check notice
Code scanning / Psalm
RedundantCondition
Failure unrelated |
The backport to stable24 failed. Please do this backport manually. # Switch to the target branch and update it
git checkout stable24
git pull origin/stable24
# Create the new backport branch
git checkout -b fix/foo-stable24
# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123
# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable24 More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport |
Summary
The files_lock app may throw ManuallyLockedExceptions when attempting to revert a file that is currently opened. This would prevent the user from rolling back a opened file.
Text and Richdocuments handle changes of the file while editing. Allow reverting files even when they are locked by these apps and let the apps handle the conflict.
Alternatives considered
I tried to tackle this in lower levels first:
Storage\LockWrapper
infiles_lock
- but it turns out we use different functions (rename
andfopen
) to rollback to an old version depending on the storage type. Thefopen
happens separatedly for the source and the target and therefore it's impossible to detect that it was triggered by a version rollback.Storage
offiles_version
- it's possible to get a hold of the actual node there. But it would only fix the issue in the normal version storage. Separate fixes would still be required forcollectives
andgroup_folders
and any other versioning implementation.So i think this is the easiest way to go.
TODO
files_lock
app - looks like it cannot due to load order issues. Once the files_lock app is loaded other apps might already have dependency injected the VersionsManager. So we cannot change it from within thefiles_lock
app.Checklist