Restore a pad from the trash on Nextcloud 31 - #190
Open
Jaggob wants to merge 1 commit into
Open
Conversation
On Nextcloud 31 NodeRestoredEvent carries a node that cannot be resolved yet, so reading its id throws. handleRestore() does exactly that, the exception escaped the listener, and the restore itself was aborted: a .pad could not be brought back from the trash at all – HTTP 500 through the web UI and WebDAV, a bare "Failed:" through occ trashbin:restore, while plain files next to it came back fine. Nextcloud 32 and 33 hand over a resolvable node, which is why this went unnoticed. The listener now re-resolves the node from its path before passing it on. The owner comes from the path rather than the session, because occ has none. The error path made this worse: it logged the file id, on that same node, from inside the catch. That threw a second time and replaced the exception being reported, so the log showed an empty NotFoundException and nothing about the actual cause. Reading the id for a log line can no longer throw. Both are covered by unit tests that fail without the fix.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoFix Nextcloud 31 trash restore for .pad nodes by re-resolving event target
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. Resolved id not verified
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+146
to
+147
| return $resolved instanceof File ? $resolved : null; | ||
| } |
There was a problem hiding this comment.
1. Resolved id not verified 🐞 Bug ≡ Correctness
materialize() returns the path-resolved File without verifying that its getId() is readable, despite the method’s contract comment. If the resolved node still throws on getId(), restoreNode() will call LifecycleService::handleRestore(), which immediately reads getId() and can still abort restore processing with an exception.
Agent Prompt
## Issue description
`RestoreFromTrashListener::materialize()` intends to return a node whose id can be read, but it does not validate `getId()` on the *fallback* node obtained via `$this->rootFolder->getUserFolder(...)->get(...)`. If that fallback node is also not resolvable yet, `LifecycleService::handleRestore()` will still throw immediately on its first line (`$file->getId()`), potentially reintroducing the restore abort.
## Issue Context
The listener is explicitly working around Nextcloud 31 delivering an unresolved node; the fallback resolution should be held to the same “id is readable” standard as the original node.
## Fix Focus Areas
- lib/Listeners/RestoreFromTrashListener.php[114-147]
## Suggested fix
After `$resolved` is obtained and verified as `instanceof File`, attempt to read its id in a `try/catch`. If it throws, log a warning/debug (without calling `getId()` again elsewhere) and return `null` instead of returning `$resolved`.
## Test update
Add/extend a unit test where the fallback `$resolved` mock is a `File` whose `getId()` still throws, and assert the listener does not throw and does not call `LifecycleService::handleRestore()`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+50
to
+53
| $node = $this->materialize($node); | ||
| if ($node === null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
2. Silent materialize null returns 🐞 Bug ◔ Observability
When materialize() returns null, handle() returns immediately without logging, so lifecycle restore handling is skipped with no diagnostic trail. This makes future restore edge-cases (unexpected path formats, NotFoundException) hard to detect and debug.
Agent Prompt
## Issue description
Several `materialize()` failure paths return `null` without any log (e.g., invalid path format, `getPath()` failure, `NotFoundException`). `handle()` then returns silently, meaning `restoreNode()` / `LifecycleService::handleRestore()` are skipped with no indication why.
## Issue Context
A warning is already logged for the generic `\Throwable` resolution error branch, but the other `null` returns are silent. Adding a lightweight debug/warning log improves diagnosability without reintroducing the restore-aborting behavior.
## Fix Focus Areas
- lib/Listeners/RestoreFromTrashListener.php[35-56]
- lib/Listeners/RestoreFromTrashListener.php[122-137]
## Suggested fix
Add a debug (or warning) log when `materialize()` returns `null`, including at least the best-effort path (guarded by try/catch) and a short reason (e.g., `unexpected_path_format`, `not_found`, `get_path_failed`). Ensure the logging path cannot throw (do not call `getId()` in these logs; reuse `loggableFileId()` only if safe).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On Nextcloud 31 restoring a
.padfrom the trash fails. Through the web UI and WebDAV it ends in an HTTP 500; throughocc trashbin:restoreit prints a bareFailed:while plain files listed next to it come back fine. The file is not lost, but it cannot be brought back.NodeRestoredEventhands the listener a node that cannot be resolved yet, so reading its id throws.LifecycleService::handleRestore()does exactly that on its first line, the exception escapes the listener, and it aborts the restore Nextcloud is in the middle of. Nextcloud 32 and 33 pass a resolvable node, which is why this survived unnoticed — the suite only ever ran against one instance.The listener now re-resolves the node from its path before handing it on. The owner is taken from the path rather than the session, because
occhas none.The error path made the diagnosis worse than the bug: it logged the file id, from that same node, inside the
catch. That threw a second time and replaced the exception being reported, so the log showed an emptyNotFoundExceptionand nothing about the cause. Reading an id for a log line can no longer throw.Verified: two unit tests that fail without the fix — one for the unresolvable node, one asserting the original exception survives when the id cannot be logged. Full PHPUnit 576/2193, Psalm clean. Reproduced end to end on a Nextcloud 31 container before and after:
occ trashbin:restoregoes fromFailed:tosuccess, and thepad-trash-restorebrowser spec from red to green on 31, 32 and 33.The container stack those runs used arrives in the follow-up PR for #112.