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

[warner]

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.

[davidsarah]

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.

[davidsarah]

Replying to davidsarah:

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

I meant, if the directory already exists.

[davidsarah]

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

[davidsarah]

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

[warner]

sounds good to me

[davidsarah]

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:4, 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]

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

[davidsarah]

Review needed for 1.6.

[zooko]

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

[davidsarah]

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.