#705 closed defect (fixed)

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

Reported by: zooko Owned by: kevan
Priority: critical Milestone: 1.5.0
Component: code-frontend-cli Version: 1.4.1
Keywords: reliability Cc: kevan
Launchpad Bug:

Description

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.

Attachments (7)

adder_tests.txt (29.6 KB) - added by kevan at 2009-07-18T19:52:04Z.
adder.txt (30.8 KB) - added by kevan at 2009-07-20T04:10:44Z.
Change only_files to only-files in the Adder class
webapi.2.txt (31.1 KB) - added by kevan at 2009-07-20T04:11:12Z.
webapi_tests.txt (29.4 KB) - added by kevan at 2009-07-20T04:11:25Z.
webapi.txt (31.1 KB) - added by kevan at 2009-07-20T04:11:36Z.
mv.txt (30.0 KB) - added by kevan at 2009-07-20T04:11:51Z.
tests.txt (32.0 KB) - added by kevan at 2009-07-20T04:12:02Z.

Download all attachments as: .zip

Change History (40)

comment:1 Changed at 2009-05-13T03:01:11Z by zooko

  • Keywords reliability added; data preservation removed

comment:2 Changed at 2009-05-13T03:13:04Z by 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.

comment:3 Changed at 2009-07-02T20:30:05Z by zooko

I like Kevin's suggestions.

comment:4 Changed at 2009-07-04T05:53:14Z by 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.

comment:5 Changed at 2009-07-04T05:59:14Z by kevan

  • Cc kevan added

comment:6 Changed at 2009-07-04T15:24:03Z by 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.

comment:7 Changed at 2009-07-07T06:08:20Z by 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.

comment:8 Changed at 2009-07-08T03:13:06Z by 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.

comment:9 Changed at 2009-07-11T05:48:57Z by 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?

comment:10 Changed at 2009-07-12T10:21:43Z by 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'.

comment:11 Changed at 2009-07-15T02:51:05Z by zooko

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

comment:12 Changed at 2009-07-15T02:51:58Z by zooko

  • Keywords review added

comment:13 Changed at 2009-07-15T02:52:11Z by zooko

  • Keywords changed from reliability, review to reliability review

comment:14 Changed at 2009-07-15T02:52:21Z by zooko

  • Keywords review removed

comment:15 Changed at 2009-07-15T02:52:27Z by zooko

  • Keywords review added

comment:16 Changed at 2009-07-16T02:22:02Z by zooko

  • Keywords review removed

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.

comment:17 Changed at 2009-07-16T02:24:27Z by zooko

  • Owner set to kevan

comment:18 Changed at 2009-07-16T04:08:56Z by 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.

comment:19 Changed at 2009-07-16T05:20:31Z by 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.

comment:20 Changed at 2009-07-16T12:36:35Z by 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.)

comment:21 Changed at 2009-07-18T03:19:13Z by 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.

comment:23 Changed at 2009-07-18T13:08:34Z by 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.

comment:24 Changed at 2009-07-18T19:47:17Z by 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.

Changed at 2009-07-18T19:52:04Z by kevan

comment:26 Changed at 2009-07-19T20:58:14Z by 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?

comment:27 Changed at 2009-07-19T22:13:43Z by 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.

comment:28 Changed at 2009-07-20T00:50:25Z by 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.

comment:29 Changed at 2009-07-20T01:48:31Z by 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.

comment:30 Changed at 2009-07-20T04:10:13Z by 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.

Changed at 2009-07-20T04:10:44Z by kevan

Change only_files to only-files in the Adder class

Changed at 2009-07-20T04:11:12Z by kevan

Changed at 2009-07-20T04:11:25Z by kevan

Changed at 2009-07-20T04:11:36Z by kevan

Changed at 2009-07-20T04:11:51Z by kevan

Changed at 2009-07-20T04:12:02Z by kevan

comment:32 Changed at 2009-07-20T15:43:34Z by 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.

comment:33 Changed at 2009-07-20T18:22:53Z by 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.

Note: See TracTickets for help on using tickets.