Filesapi renamefailcase #26629

PVince81 · 2016-11-14T17:33:11Z

Description

refactored Files Node API tests to have more coverage: basically make NodeTest abstract and put the common tests there
refactored "rename()" and "copy()" operations to be in the Node class instead of File and Folder
fixed "rename()" and "copy()" operations to not ignore failures

Why so much refatoring ? If I'd do the fix directly I'd have to add a lot of missing tests in the "FolderTest" class and also duplicate the fix across File and Folder.

Now I agree that throwing NotPermittedException in case of unknown failure from the view isn't very elegant. Any better suggestions welcome.

Related Issue

Fixes #26625

Motivation and Context

Because we must not ignore errors.
Also, this blocks #26615 where I need to rely on a move failure for a unit test.

How Has This Been Tested?

Running unit tests

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Tech debt

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

@jvillafanez @DeepDiver1975

mention-bot · 2016-11-14T17:33:12Z

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @icewind1991, @DeepDiver1975 and @nickvergessen to be potential reviewers.

PVince81 · 2016-11-14T17:35:26Z

lib/private/Files/Node/Node.php

+			$this->root->emit('\OC\Files', 'preRename', [$this, $nonExisting]);
+			$this->root->emit('\OC\Files', 'preWrite', [$nonExisting]);
+			if (!$this->view->rename($this->path, $targetPath)) {
+				throw new NotPermittedException('Could not move ' . $this->path . ' to ' . $targetPath);


this is the actual fix: add a if here and in the copy method later

jvillafanez · 2016-11-15T09:00:03Z

lib/private/Files/Node/File.php

 use OCP\Files\NotPermittedException;

 class File extends Node implements \OCP\Files\File {
+	protected $nonExistingClass = 'OC\Files\Node\NonExistingFile';


I think using a method is more clear. In the Node class you could use something like:

protected function getNonExistingClass() { if (__CLASS__ === 'OC\Files\Node\Node') { throw Exception(); } else { $convertedClass = convertToNonExistingString(__CLASS__); // 'OC\Files\Node\File' -> 'OC\Files\Node\NonExistingFile'; return new $convertedClass(); } }

We'll need to define the expected return values for the function.

This way, the subclasses won't need to define the $nonExistingClass variable. As long as the method is aware that the target class might not exists, it should be fine

You're right. I was assuming that no one would ever bother to extend that class 😄

Will adjust...

PVince81 · 2016-11-21T13:48:20Z

@jvillafanez adjusted slightly differently. I used a method to simply return the string name of the NonExisting class to use.

jvillafanez · 2016-11-22T08:27:30Z

Right now, the Node class needs to create the non-existent class, and to do that it needs to know how (particularly the parameters).

On the other hand, if an instance is returned, the Node class might only need to check if the instance is a Node class or subclass. In fact, taking into account the Node class isn't abstract we should make sure the returned instances are a subclass of Node but not Node.

Btw, the tests need to be adjusted.

PVince81 · 2016-11-22T17:41:02Z

@jvillafanez I've now adjusted the node to have a method that creates the non existing instance for a given path.

The tests still pass and I think they are fine. I don't want to add the same logic there because the tests really only check instanceof so a string for the class name to verify is enough.

jvillafanez · 2016-11-23T08:00:26Z

lib/private/Files/Node/File.php

+	 * @param string $path path
+	 * @return string non-existing node class
+	 */
+	protected function createNonExistingNode($path) {


missing return

uh oh... so the tests were lying... I'll add some hook tests to make sure this is caught too

jvillafanez · 2016-11-23T08:00:43Z

lib/private/Files/Node/Folder.php

+	 * @param string $path path
+	 * @return string non-existing node class
+	 */
+	protected function createNonExistingNode($path) {


missing return

Whenever a rename or copy operation failed on the view, we must throw an exception instead of just ignoring.

createMock only accepts a single param

PVince81 · 2016-11-24T11:10:45Z

I fixed what you said and tried adding a test for the rename hook, but that one isn't done yet. Need to properly mock $root but keeping listen as is. Will need more work.

PVince81 · 2016-11-24T16:17:58Z

So, added and fixed tests for the move/copy hooks.

Ready for another review.

jvillafanez · 2016-11-25T08:03:02Z

Code looks good 👍

lock · 2019-08-04T07:02:44Z

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.

Refactor files node API for more code reuse

0fe7202

PVince81 added the 3 - To Review label Nov 14, 2016

PVince81 added this to the 9.2 milestone Nov 14, 2016

PVince81 mentioned this pull request Nov 14, 2016

Node API's move doesn't check view->rename return value #26625

Closed

PVince81 commented Nov 14, 2016

View reviewed changes

jvillafanez reviewed Nov 15, 2016

View reviewed changes

PVince81 force-pushed the filesapi-renamefailcase branch from 1a8146d to 489f2e0 Compare November 21, 2016 13:48

PVince81 force-pushed the filesapi-renamefailcase branch from 489f2e0 to d8997b1 Compare November 22, 2016 17:40

jvillafanez reviewed Nov 23, 2016

View reviewed changes

Vincent Petry added 2 commits November 24, 2016 12:10

Fix files node API failed rename/copy

2937450

Whenever a rename or copy operation failed on the view, we must throw an exception instead of just ignoring.

Fix node API createMock usage

ba9bf30

createMock only accepts a single param

PVince81 force-pushed the filesapi-renamefailcase branch from d8997b1 to ba9bf30 Compare November 24, 2016 11:10

Add tests for move/copy hooks in Node API

026ed3a

PVince81 merged commit c154785 into master Nov 25, 2016

PVince81 deleted the filesapi-renamefailcase branch November 25, 2016 08:23

icewind1991 mentioned this pull request Jan 11, 2017

Fix files node API failed rename/copy nextcloud/server#3030

Merged

lock bot locked as resolved and limited conversation to collaborators Aug 4, 2019

Filesapi renamefailcase #26629

Filesapi renamefailcase #26629

Uh oh!

Conversation

PVince81 commented Nov 14, 2016

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

mention-bot commented Nov 14, 2016

Uh oh!

PVince81 Nov 14, 2016

Choose a reason for hiding this comment

Uh oh!

jvillafanez Nov 15, 2016

Choose a reason for hiding this comment

Uh oh!

jvillafanez Nov 15, 2016

Choose a reason for hiding this comment

Uh oh!

PVince81 Nov 18, 2016

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Nov 21, 2016

Uh oh!

jvillafanez commented Nov 22, 2016

Uh oh!

PVince81 commented Nov 22, 2016

Uh oh!

jvillafanez Nov 23, 2016

Choose a reason for hiding this comment

Uh oh!

PVince81 Nov 23, 2016

Choose a reason for hiding this comment

Uh oh!

jvillafanez Nov 23, 2016

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Nov 24, 2016

Uh oh!

PVince81 commented Nov 24, 2016

Uh oh!

jvillafanez commented Nov 25, 2016

Uh oh!

lock bot commented Aug 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants