"tahoe mv" unlinks the target even when it is a directory

[zooko]

I just ran the following command:

tahoe mv --node-directory=~/.tahoe-volunteergrid "01 Battery.flac" "Metallica-Master of Puppets-4-of-6-fec"

Since "Metallica-Master of Puppets-4-of-6-fec" was an existing directory, I expected it to move the file "01 Battery.flac" into that directory. Instead it unlinked that directory and renamed the file to "Metallica-Master of Puppets-4-of-6-fec". Fortunately the directory was empty so no data was lost.

[kpreid]

Suggestion:

1. Have the move-into behavior triggered not by the presence of a directory, but of a trailing slash in the target pathname. The former behavior makes it harder to e.g. write robust scripts.
2. To improve the behavior in the case you had, fail or prompt instead of overwriting an existing item, by default.

[zooko]

I like Kevin's suggestions.

[kevan]

It turns out that the logic for the first point was already in tahoe_mv.py -- just not the behavior itself.

From a cursory look at the webapi docs, appending something to a directory is as simple as sticking the name of the child after the name of the directory in the URL -- so if we want to add tahoe:test.pdf to tahoe:testdir/, we can make

http://127.0.0.1:3456/uri/<cap>/testdir/test.pdf

unless I'm misunderstanding something. That's basically what I did in this patch.

I haven't looked into implementing the second part yet.

[zooko]

I looked at your patch and it looks right to me. I think I would change the if path.endswith("/"): to if to_url.endswith("/"):. Also the patch description is not "Make mv move files into directories, instead of overrwriting directories.", because this patch doesn't (yet) achieve that in the case that the target directory name doesn't have a trailing slash.

We need a test to go along with this patch -- perhaps just add some statements to src/allmydata/test/test_system.py to move a file into a directory-with-trailing-slash and assert that it works. Err, wait a second -- there is already a test of this in src/allmydata/test/test_system.py#L973​. It says:

d1.addCallback(self.log, "mv 'P/sekret data' P/personal/")
d1.addCallback(lambda res:
    home.move_child_to(u"sekrit data", personal))

This test currently passes, even without your patch that looks for a trailing slash and appends the filename to the target URL. Why is that? We should have a test that fails before we apply a patch to make the test pass.

For the next part of the issue, we should add a test that tries to mv a file to a directory, without trailing slash. According to Kevin Reid's suggestion, Tahoe should refuse to do anything in that case, e.g. if 'sekret data' is a file and 'personal' is a directory, then tahoe mv "sekret data" personal should emit an error message and make no change. The test should make sure that tahoe emits a suitable error message, in addition to making sure that it doesn't move or remove the "sekret data" file.

[kevan]

The test passes because it's testing the filesystem layer, while tahoe_mv.py uses the webapi.

Basically, we want tahoe mv to do the following:

• If we're doing tahoe mv file1 file2, file1 should replace file2.
• If we're doing tahoe mv file1 folder1/, file1 should be inserted into folder1.
• If we're doing tahoe mv file1 folder1, tahoe mv should print an error.

To the user, this looks more or less the same regardless of how we do it on the backend -- tahoe mv should output "OK" in the first two cases, and an error message in the second. I'm attaching some tests that make sure that happens.

I guess there are a few ways to implement this sort of functionality.

• We could rely on the CLI (i.e., tahoe mv) to be smart enough to distinguish between files and folders, and structure HTTP requests appropriately. By using the methods described in === Get Information About A File Or Directory (as JSON) ===, we can easily (and rather laboriously) determine whether the target is a file or a directory, and then act accordingly.
• Alternatively, we could maybe bake some sort of functionality into the webapi that covers this. I'm not sure of the best way to do this, though.

[zooko]

Those tests look good! The test should be more picky and require tahoe mv to explain more clearly what went wrong when someone does tahoe mv file directory.

[kevan]

I'm attaching a patch that implements + tests for the behavior described above.

However, when testing, I came upon another maybe-bug.

If, using the trunk build from last night, I do

tahoe mv tahoe:file1 file1

with the expectation that it will copy the remote file to my system, it purports to succeed (i.e.: no error message) but doesn't actually copy anything to my system, and unlinks the remote file. Is this what we want to happen?

[warner]

Hm, good question. "tahoe mv" was indeed meant for tahoe-to-tahoe moves, so that an unqualified "file1" really means "tahoe:file1". (the fact that "tahoe mv alias:file1 alias:file1" results in deleting a file is certainly a bug, probably in the dirnode code).

I don't know if it's better to add docs/etc to teach people to expect that "tahoe mv" doesn't touch the local disk, or to add code/tests to make "tahoe mv" behave more like people's existing expectations. There's value in having all tahoe CLI commands that happen to overlap regular unix commands (cp, ln, mv, rm) be prepared to handle both local-disk and tahoe-filesystem arguments. But it also adds code, complication, and redundancy (e.g. why would you ever use "tahoe rm ~/.emacs" instead of regular rm?).

Overall, I guess 'mv' should handle both local and tahoe-side files, and should behave like 'cp'.

[zooko]

We need someone to review Kevan's patch. Do it now and this patch can go into TahoeLAFS v1.5!

[zooko]

The comment "# we should probably pick some output that is more informative, and # put that here" can be removed from the test patch.

Hm, the implementation works by using urllib.urlopen() to send a query for the metadata (?t=json and then test whether the target exists and if so whether it is a directory. If that test passes then it goes ahead and does the HTTP PUT to overwrite the target.

The problem with this is that there is a race condition, also known as a TOCTTOU ("Time Of Check To Time Of Use") issue, where the object under the target name may be non-existent or be a file at the time the check happens, but be a directory when the subsequent PUT happens.

A safer implementation would extend the semantics of the PUT to tell the webapi server "except don't do it if the target turns out to be a directory". Hm, I guess 'tahoe mv' really ought to be using POST /uri/$DIRCAP/[SUBDIRS../]?t=rename anyway instead of PUT. Hm, I see that that POST command has an undocumented (in docs/frontends/webapi.txt) 'replace' option indicating whether it should replace if there is already a child under the target name or abort. src/allmydata/web/directory.py@20090715025814-92b7f-d4af644430e5daef6d6ad57cc550c8faceaeb2cf#L327

I guess the right implementation of this ticket is to extend that webapi command with a replace=only_files option which will abort if there is a child under the target name and that child is a directory.

Unsetting the 'review' keyword. Kevan: what do you think? I don't want to punt this issue out of 1.5 because it is a potentially data-losing ui issue.

[kevan]

Thanks for the review, and for seeing that -- I didn't even think about race conditions.

The reason I didn't use the rename method you reference is

This operation cannot move the child to a different directory.

which I took to mean that, if someone did tahoe mv tahoe:dir/file1 tahoe:file1, using that wouldn't work. Perhaps I'm mistaken, or misinterpreting something.

I think I agree with your thoughts on the implementation. I don't suppose that any amount of client-side hacking is going to be enough to address the race condition above. Presumably the webapi would be a better place to address this.

Then, what we'd want to do is

• Write tests for the extension of the webapi command (in addition to the ones for tahoe mv, which are probably still a good idea even if we don't end up doing a lot of stuff there)
• Alter the webapi command to pass the tests
• Re-write tahoe_mv.py to use the new functionality

Seem okay? If so, I'll start poking around/starting that.

[kevan]

From what I understand of the implementation of POST /uri/$DIRCAP/[SUBDIRS../]?t=rename, the webapi docs are right -- it's calling move_child_to (source:src/allmydata/dirnode.py@20090713001320-92b7f-fe89f2baaab25c8eb3eb96c146177eb42108aeee#L515) with the new_parent argument set to the parent of the child being moved -- if I understand things correctly, anyway, that means that we can only use that command to move things around within a directory. I'm not sure how easy it'd be to tweak that to behave as we'd want.

The PUT method seems like it'd be easy enough to modify. We could check for whatever replace option we wanted to correspond to the behavior we want to implement in tahoe_mv.py (only_files is fine by me), then examine to_file and self.node in the DirectoryNodeHandler to see if they're as they should be -- if so, we upload as we do now, and if not, we can raise an exception.

I realize that I probably seem like a cheerleader for PUT at this point, but, given my understanding of the POST method, I'm not aware of any better options.

[zooko]

You're right about POST ?t=rename not offering this functionality -- sorry I overlooked that. In the future, it might be good to extend POST ?t=rename to do that, but I agree with you that doing so would take longer and be more prone to error than other ways to solve this ticket right now. If you wanted to extend POST ?t=rename, you'd start here in directory.py, and change it to look up the target node by name before calling move_child_to.

Your proposal for how to extend PUT to have replace=only_files sounds right to me. You should push this extended semantics of replace all the way down into the Adder class. That class currently takes a constructor argument overwrite which is a boolean. You could either change it to a multi-option argument (i.e. overwrite can be one of "yes", "no", "only_files"), or add a second argument named something like preserve_directories which is a boolean and add a precondition assertion that not ((not overwrite) and preserve_directories) (i.e. it is a mistake if someone passed overwrite=False and preserve_directories=True to the constructor of Adder.)

[kevan]

Good idea with implementing the behavior in Adder.

I've written some tests for Adder, and then implemented your first suggestion. If you set overwrite to "only_files", then it will refuse to overwrite directories. Instead of changing from True to "yes" and False to "no", I left those as they are now, since it seemed to stand less of a chance of breaking a bunch of stuff. Let me know if you want me to change this.

Do we want to add tests for the new overwrite option to the tests for methods that use Adder (e.g.: set_node), or is what I have now good enough?

I'll start working on the webapi stuff next, then alter tahoe_mv.py to use the new options.

[zooko]

Okay, I'm reviewing ​http://allmydata.org/trac/tahoe/attachment/ticket/705/adder_tests.txt and ​http://allmydata.org/trac/tahoe/attachment/ticket/705/adder.txt .

[zooko]

review:

I don't understand the comment at ​http://allmydata.org/trac/tahoe/attachment/ticket/705/adder_tests.txt#L44 . "We have lost important things. Let's try it with a directory."

Other than that, these patches look good! I will wait to see if Kevan wants to amend-record to change that comment that I didn't understand, but otherwise I'm ready to apply them.

[kevan]

It's just a joke -- I filled file1 with "Important Things", and then overwrote it. :)

If it's confusing, I'll remove it, though.

[zooko]

Committed ​http://allmydata.org/trac/tahoe/attachment/ticket/705/adder.txt in c476c66b0ea37912 and ​http://allmydata.org/trac/tahoe/attachment/ticket/705/adder_tests.txt in ca4de9ee974af40d.

[kevan]

I'm uploading patches for the tests and functionality involved in the webapi part of this.

I notice that source:/src/allmydata/web/directory.py​ and source:/src/allmydata/web/filenode.py​ do some checking of the replace parameter before sending it to lower levels. Should I expand that checking to deal with the only_files case, or is it okay to let Adder handle that?

[kevan]

mv.txt and tests.txt are the fixes for tahoe_mv.py and the revised unit tests, respectively, and should hopefully be it for this issue.

[zooko]

I just reviewed these four most recent patches. They are all good, except for the way it catches AssertionError from boolean_of_arg() and then tries again with just get_arg(). I think a cleaner way to do this is to define a special function to use on ?replace= instead of boolean_of_arg(). Name it something like parse_replace_arg() and have it do the same thing that boolean_of_arg() does unless the argument is "only_files". By the way, one of your tests has a typo in which it says ?replace=only_fles! With the new suggested arg parsing, that would yield a nice error message instead of silently treating it as the same as ?replace=true.

[kevan]

Thanks for the feedback. I added parse_replace_arg to source:/src/allmydata/web/common.py​, added tests for it, changed the try...except blocks to use it, and fixed the typo in my other unit test.

[kevan]

Okay, I followed some suggestions from warner:

• We now use only-files instead of only_files as an argument for {{{replace}}
• I've corrected inaccurate help text for tahoe mv that indicated that one could move local files to the grid; this was probably just a bad test on my part.
• I've added documentation to the webapi docs about this feature.
• A bug where tahoe mv didn't correctly move nested directories is now fixed.
• Some miscellaneous formatting issues have been cleaned up.

The added adder.txt is the patch for the Adder class + tests to reflect the change to only-files. webapi.txt is the added PUT functionality, and webapi_tests.txt are tests for that. mv.txt and tests.txt are changes to tahoe_mv.py and tests for those changes, respectively.

I took a quick glance at webapi.txt to see if there were any POST methods that were more or less similiar to the PUT method that I modified (at least enough so that it'd make sense to also use the only-files option with them), and didn't see anything. If anyone is aware of something that I might have missed, please tell me.

[kevan]

Change only_files to only-files in the Adder class

[zooko]

Committed in 3a9f1f2952a7b890, 7ab92c751173a78e, 90677745b3ec196e. 74207d8334dc0340, 18a80d99b1042480, 40360a7a9c05b1fa, 8eb7ddab6b7f20fa, 36f2e012756b65a4, 52aceb1a8e64129c, 4331326b21028ff5, 0d8b1e29fa128c64. Thanks, Kevan!

[warner]

Looking at webapi.txt, it looks like "POST t=uri" is the POST that corresponds to the "PUT t=uri" that you modified ("This behaves much like the PUT t=uri operation"). Although the docs don't mention it, the code (in webish.directory.DirectoryNodeHandler._POST_uri line 305) shows that it accepts a replace= argument and passes it through to dirnode.set_uri, so we should probably update it too. That means updating _POST_uri to process the replace= argument differently, adding a note to webapi.txt that says "this accepts the same replace= argument as 'PUT t=uri'", and adding a test to test_web.py just like the one you wrote for "PUT t=uri".

Searching webapi.txt for "replace=", it looks like "POST t=upload" also accepts a replace= argument, so we should update that one for consistency too. It might be worth grepping through src/allmydata/webish/*.py for "get_arg" and "replace" to find any others.

On the other hand, if we're itching to get 1.5 out the door, we could survive without having replace= be completely consistent (I just pushed a small docs patch to make sure the docs match the implementation, so we could release 1.5 now without any undocumented inconsistencies). It just makes the docs easier to follow, and reduces some surprises later on (if someone used replace=only-files on one of the two other webapi operations without testing it first, and clobbered a directory), when all instances of the replace= argument behave the same way.

[kevan]

Between those and the POST method that zooko mentioned earlier, we could probably stand to open a new ticket (obviously for after 1.5.0) with the things that we still want to do here.