webapi t=mkdir-with-children and mkdir-immutable: behavior when directory already exists? #903

New Issue

warner · 2010-01-14T22:31:30Z

warner commented

2010-01-14 22:31:30 +00:00

I'm not entirely clear on what the webapi POST /uri/$DIRCAP/SUBDIRS../SUBDIR?t=mkdir calls should do when the given subdirectory already exists. The docs state that for the regular t=mkdir case (i.e. creating an empty mutable directory), "If the named target directory already exists, this will make no changes to it". I believe this is fine.

But for the other cases (t=mkdir-with-children, t=mkdir-immutable), the situation gets more interesting. We need to signal whether the new directory was created (and thus has the new contents provided by these calls), or if it already existed (and therefore still has some old contents). There should also be a clear way for the caller to indicate whether they want to get an error when the directory already exists, or if this "return anyways" behavior is ok.

In other calls, we have a "replace=" parameter which is related to this. There was some incomplete code for replace= in mkdir-immutable, and some slightly more complete code in mkdir-with-children, but I don't think the semantics are well defined yet.

I'm not entirely clear on what the webapi `POST /uri/$DIRCAP/SUBDIRS../SUBDIR?t=mkdir` calls should do when the given subdirectory already exists. The docs state that for the regular t=mkdir case (i.e. creating an empty mutable directory), "If the named target directory already exists, this will make no changes to it". I believe this is fine. But for the other cases (t=mkdir-with-children, t=mkdir-immutable), the situation gets more interesting. We need to signal whether the new directory was created (and thus has the new contents provided by these calls), or if it already existed (and therefore still has some old contents). There should also be a clear way for the caller to indicate whether they want to get an error when the directory already exists, or if this "return anyways" behavior is ok. In other calls, we have a "replace=" parameter which is related to this. There was some incomplete code for replace= in mkdir-immutable, and some slightly more complete code in mkdir-with-children, but I don't think the semantics are well defined yet.

warner added the

labels 2010-01-14 22:31:30 +00:00

warner added this to the undecided milestone 2010-01-14 22:31:30 +00:00

davidsarah commented

2010-01-18 01:58:59 +00:00

t=mkdir-immutable should clearly fail if the directory doesn't exist (because then it can't be created as immutable without first deleting it, and the operation didn't ask to do that).

Also, I don't think that automatically creating the intermediate directories makes a lot of sense for the mkdir-immutable-with-path operations:

if the intermediate directories are created as immutable, then they will only ever have one child, and that isn't very useful.
if they are created as mutable, that's irregular and might not be what the user (or programmer, if they're not paying close attention to docs) expected.

So I think that a mkdir-immutable-with-path should fail if the intermediate directories don't already exist (in which case, they must obviously be mutable). Explicit Is Better Than Implicit.

`t=mkdir-immutable` should clearly fail if the directory doesn't exist (because then it can't be created as immutable without first deleting it, and the operation didn't ask to do that). Also, I don't think that automatically creating the intermediate directories makes a lot of sense for the `mkdir-immutable`-with-path operations: * if the intermediate directories are created as immutable, then they will only ever have one child, and that isn't very useful. * if they are created as mutable, that's irregular and might not be what the user (or programmer, if they're not paying close attention to docs) expected. So I think that a `mkdir-immutable`-with-path should fail if the intermediate directories don't already exist (in which case, they must obviously be mutable). Explicit Is Better Than Implicit.

tahoe-lafs added

major

and removed

minor

labels 2010-01-18 01:58:59 +00:00

davidsarah commented

2010-01-18 01:59:31 +00:00

Replying to davidsarah:

t=mkdir-immutable should clearly fail if the directory doesn't exist ...

I meant, if the directory already exists.

Replying to [davidsarah](/tahoe-lafs/trac-2024-07-25/issues/903#issuecomment-74670): > `t=mkdir-immutable` should clearly fail if the directory doesn't exist ... I meant, if the directory already exists.

davidsarah commented

2010-01-18 02:11:47 +00:00

So, I would suggest to strip out support for replace= in mkdir-immutable.

For mkdir-with-children, it makes sense for replace=true to imply the same semantics as set_children. That has an overwrite parameter to control whether existing children can be overwritten without error. So there would be three cases when the directory already exists:

replace=false: error (regardless of overwrite setting)
replace=true&overwrite=false: works like set_children with overwrite=false
replace=true&overwrite=true: works like set_children with overwrite=true

So, I would suggest to strip out support for `replace=` in `mkdir-immutable`. For `mkdir-with-children`, it makes sense for `replace=true` to imply the same semantics as `set_children`. That has an `overwrite` parameter to control whether existing children can be overwritten without error. So there would be three cases when the directory already exists: * `replace=false`: error (regardless of `overwrite` setting) * `replace=true&overwrite=false`: works like `set_children` with `overwrite=false` * `replace=true&overwrite=true`: works like `set_children` with `overwrite=true`

tahoe-lafs changed title from ~~webapi t=mkdir: behavior when directory already exists?~~ to webapi t=mkdir-with-children and mkdir-immutable: behavior when directory already exists?

2010-01-18 02:11:47 +00:00

davidsarah commented

2010-01-18 02:15:21 +00:00

Oh, and blocking files should never be replaced with directories regardless of the settings of replace or overwrite.

Oh, and blocking files should never be replaced with directories regardless of the settings of `replace` or `overwrite`.

warner commented

2010-01-19 06:54:34 +00:00

sounds good to me

davidsarah commented

2010-01-24 22:44:53 +00:00

Looking at create_subdirectory in dirnode.py, it seems that the current support for replace=true in mkdir-with-children is doing something that is likely to be unexpected: it creates the new subdirectory with its children, and then overwrites the entry with the given name, effectively unlinking it (regardless of its type -- directory, file, or unknown).

Leaving this code as it is would create a forward-compatibility problem if we want to change the semantics to something similar to comment:74674, because a webapi client could not safely use replace=true and get consistent semantics between v1.6 and v1.7+. It would be better to always pass overwrite=False to create_subdirectory in this version, so that future clients that depend on the 1.7 semantics for replace=true (if we support it at all) will get an error with 1.6. That would be consistent with webapi.txt which doesn't mention any replace parameter.

Looking at `create_subdirectory` in dirnode.py, it seems that the current support for `replace=true` in `mkdir-with-children` is doing something that is likely to be unexpected: it creates the new subdirectory with its children, and then overwrites the entry with the given name, effectively unlinking it (regardless of its type -- directory, file, or unknown). Leaving this code as it is would create a forward-compatibility problem if we want to change the semantics to something similar to [comment:74674](/tahoe-lafs/trac-2024-07-25/issues/903#issuecomment-74674), because a webapi client could not safely use `replace=true` and get consistent semantics between v1.6 and v1.7+. It would be better to always pass `overwrite=False` to `create_subdirectory` in this version, so that future clients that depend on the 1.7 semantics for `replace=true` (if we support it at all) will get an error with 1.6. That would be consistent with webapi.txt which doesn't mention any `replace` parameter.

davidsarah commented

2010-01-24 22:51:26 +00:00

Attachment remove-mkdir-replace-darcspatch.txt (39947 bytes) added

Remove replace= parameter to mkdir-immutable and mkdir-with-children

**Attachment** remove-mkdir-replace-darcspatch.txt (39947 bytes) added Remove replace= parameter to mkdir-immutable and mkdir-with-children

remove-mkdir-replace-darcspatch.txt

39 KiB

davidsarah commented

2010-01-24 22:53:08 +00:00

Review needed for 1.6.

zooko commented

2010-01-26 04:10:07 +00:00

Looks good! Thanks for catching this potential forward-compatibility issue. Applied your patch as changeset:26ab58e0062e6921.

davidsarah commented

2010-01-26 18:06:46 +00:00

This ticket should stay open until we decide whether or how to define replace=true, but the semantics without replace are now well-defined, so it can be downgraded to minor.

webapi.txt is being changed for #833, so I'll include documentation that these commands return an error when the child already exists in that patch.

This ticket should stay open until we decide whether or how to define `replace=true`, but the semantics without `replace` are now well-defined, so it can be downgraded to minor. webapi.txt is being changed for #833, so I'll include documentation that these commands return an error when the child already exists in that patch.

tahoe-lafs added

minor

and removed

major

labels 2010-01-26 18:06:46 +00:00

zooko modified the milestone from undecided to 1.7.0

2010-02-27 06:41:06 +00:00

zooko modified the milestone from 1.7.0 to eventually

2010-06-17 04:36:33 +00:00

Sign in to join this conversation.