fix: Pass parent to NonExistingFile instances #40312
Conversation
juliusknorr
requested review from
icewind1991,
max-nextcloud,
skjnldsv and
come-nc
juliusknorr
force-pushed
the
bugfix/noid/non-existing-node-parent
branch
from
581c2be
to
0dfddfc
Compare
lib/private/Files/Node/File.php
Outdated
| @@ -40,7 +40,7 @@ class File extends Node implements \OCP\Files\File { | |||
| * @return NonExistingFile non-existing node | |||
| */ | |||
| protected function createNonExistingNode($path) { | |||
| return new NonExistingFile($this->root, $this->view, $path); | |||
| return new NonExistingFile($this->root, $this->view, $path, $this->root->get(dirname($path))); | |||
There was a problem hiding this comment.
Can you go into the details of what breaks without this and why is the lazy parent not enough?
There was a problem hiding this comment.
getParent used to just directly get it in https://github.com/nextcloud/server/pull/38150/files#diff-edc136e5646ea4171607db53b38833e8f6caaacc64874a0a850aa4444f0a56edL303-R309 but now always makes use of the file info which is not available for a nonexisting file.
There was a problem hiding this comment.
Then wouldn’t the right fix be to check for fileinfo presence before calling getParentId on it and fallback on previous behavior if fileInfo is not there? I’m afraid there might be other side effects we missed.
There was a problem hiding this comment.
Yeah might be indeed a good idea, will check that
There was a problem hiding this comment.
Pushed a more generic approach but still passing the parent to the NonExistingFile to avoid an extra queries in that case.
There was a problem hiding this comment.
to avoid an extra queries in that case.
doesn't it just move the query to the constructor call here?
As is it always does the "get parent" query even when getParentId isn't called, without this it only does the query when getParentId is called
juliusknorr
force-pushed
the
bugfix/noid/non-existing-node-parent
branch
from
0dfddfc
to
a86fa96
Compare
lib/private/Files/Node/Node.php
Outdated
| @@ -297,10 +297,18 @@ public function getParent(): INode|IRootFolder { | |||
| return $this->root; | |||
| } | |||
|
|
|||
| // Manually the parent if the current node doesn't have a file info yet | |||
There was a problem hiding this comment.
| // Manually the parent if the current node doesn't have a file info yet | |
| // Manually fetch the parent if the current node doesn't have a file info yet |
Or get? But a word is missing there I think.
juliusknorr
force-pushed
the
bugfix/noid/non-existing-node-parent
branch
from
a86fa96
to
9d584b0
Compare
|
I cannot see the same psalm error locally :/ |
lib/private/Files/Node/File.php
Outdated
| @@ -40,7 +40,7 @@ class File extends Node implements \OCP\Files\File { | |||
| * @return NonExistingFile non-existing node | |||
| */ | |||
| protected function createNonExistingNode($path) { | |||
| return new NonExistingFile($this->root, $this->view, $path); | |||
| return new NonExistingFile($this->root, $this->view, $path, $this->root->get(dirname($path))); | |||
There was a problem hiding this comment.
to avoid an extra queries in that case.
doesn't it just move the query to the constructor call here?
As is it always does the "get parent" query even when getParentId isn't called, without this it only does the query when getParentId is called
Yeah let me maybe drop that then instead and just rely on the manual get in getParent instead |
juliusknorr
force-pushed
the
bugfix/noid/non-existing-node-parent
branch
from
9d584b0
to
b9bffa9
Compare
Signed-off-by: Julius Härtl <[email protected]>
juliusknorr
force-pushed
the
bugfix/noid/non-existing-node-parent
branch
from
b9bffa9
to
7f958e8
Compare
Summary
Fixes calls to getParent after #38150 when calling on NonExistingFile instances, e.g. within a BeforeNodeRenamedEvent.
Found in failing tests for moving files in text which has special handling. CI is passing with this branch again: nextcloud/text#4779
Checklist