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

New Issue

zooko · 2009-05-13T03:00:52Z

zooko commented

2009-05-13 03:00:52 +00:00

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.

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.

zooko added the

labels 2009-05-13 03:00:52 +00:00

zooko added this to the 1.5.0 milestone 2009-05-13 03:00:52 +00:00

kpreid commented

2009-05-13 03:13:04 +00:00

Suggestion:

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.
To improve the behavior in the case you had, fail or prompt instead of overwriting an existing item, by default.

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 commented

2009-07-02 20:30:05 +00:00

I like Kevin's suggestions.

kevan commented

2009-07-04 05:53:14 +00:00

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.

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 commented

2009-07-04 15:24:03 +00:00

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 source: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.

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 source: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 commented

2009-07-07 06:08:20 +00:00

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.

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 commented

2009-07-08 03:13:06 +00:00

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.

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 commented

2009-07-11 05:48:57 +00:00

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?

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 commented

2009-07-12 10:21:43 +00:00

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'.

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 commented

2009-07-15 02:51:05 +00:00

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

zooko commented

2009-07-16 02:22:02 +00:00

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 source:docs/frontends/webapi.txt) 'replace' option indicating whether it should replace if there is already a child under the target name or abort. source: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.

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 source:docs/frontends/webapi.txt) 'replace' option indicating whether it should replace if there is already a child under the target name or abort. source: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 commented

2009-07-16 04:08:56 +00:00

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.

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 commented

2009-07-16 05:20:31 +00:00

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.

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 commented

2009-07-16 12:36:35 +00:00

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]source:src/allmydata/web/directory.py@20090715025814-92b7f-d4af644430e5daef6d6ad57cc550c8faceaeb2cf#L327, 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]source:src/allmydata/dirnode.py@20090713001320-92b7f-fe89f2baaab25c8eb3eb96c146177eb42108aeee#L73. 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.)

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]source:src/allmydata/web/directory.py@20090715025814-92b7f-d4af644430e5daef6d6ad57cc550c8faceaeb2cf#L327, 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]source:src/allmydata/dirnode.py@20090713001320-92b7f-fe89f2baaab25c8eb3eb96c146177eb42108aeee#L73. 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 commented

2009-07-18 03:19:13 +00:00

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.

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 commented

2009-07-18 12:56:37 +00:00

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 .

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 commented

2009-07-18 13:08:34 +00:00

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.

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 commented

2009-07-18 19:47:17 +00:00

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

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

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

kevan commented

2009-07-18 19:52:04 +00:00

Attachment adder_tests.txt (30326 bytes) added

**Attachment** adder_tests.txt (30326 bytes) added

adder_tests.txt

30 KiB

zooko commented

2009-07-18 22:34:15 +00:00

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

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

kevan commented

2009-07-19 20:58:14 +00:00

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?

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 commented

2009-07-19 22:13:43 +00:00

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.

`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 commented

2009-07-20 00:50:25 +00:00

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.

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 commented

2009-07-20 01:48:31 +00:00

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.

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 commented

2009-07-20 04:10:13 +00:00

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.

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 commented

2009-07-20 04:10:44 +00:00

Attachment adder.txt (31585 bytes) added

Change only_files to only-files in the Adder class

**Attachment** adder.txt (31585 bytes) added Change only_files to only-files in the Adder class

adder.txt

31 KiB

kevan commented

2009-07-20 04:11:12 +00:00

Attachment webapi.2.txt (31848 bytes) added

**Attachment** webapi.2.txt (31848 bytes) added

webapi.2.txt

31 KiB

kevan commented

2009-07-20 04:11:25 +00:00

Attachment webapi_tests.txt (30102 bytes) added

**Attachment** webapi_tests.txt (30102 bytes) added

webapi_tests.txt

29 KiB

kevan commented

2009-07-20 04:11:36 +00:00

Attachment webapi.txt (31848 bytes) added

**Attachment** webapi.txt (31848 bytes) added

webapi.txt

31 KiB

kevan commented

2009-07-20 04:11:51 +00:00

Attachment mv.txt (30744 bytes) added

**Attachment** mv.txt (30744 bytes) added

mv.txt

30 KiB

kevan commented

2009-07-20 04:12:02 +00:00

Attachment tests.txt (32782 bytes) added

**Attachment** tests.txt (32782 bytes) added

tests.txt

32 KiB

zooko commented

2009-07-20 14:05:56 +00:00

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

zooko added the

fixed

label 2009-07-20 14:05:56 +00:00

zooko closed this issue

2009-07-20 14:05:56 +00:00

warner commented

2009-07-20 15:43:34 +00:00

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.

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 commented

2009-07-20 18:22:53 +00:00

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.

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.

Sign in to join this conversation.