#903 new defect

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

Reported by: warner Owned by:
Priority: minor Milestone: eventually
Component: code-frontend-web Version: 1.5.0
Keywords: usability docs Cc:
Launchpad Bug:


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.

Attachments (1)

remove-mkdir-replace-darcspatch.txt (39.0 KB) - added by davidsarah at 2010-01-24T22:51:26Z.
Remove replace= parameter to mkdir-immutable and mkdir-with-children

Download all attachments as: .zip

Change History (13)

comment:1 Changed at 2010-01-14T23:49:23Z by davidsarah

  • Keywords reliability added

comment:2 follow-up: Changed at 2010-01-18T01:58:59Z by davidsarah

  • Keywords usability docs added
  • Priority changed from minor to major

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.

comment:3 in reply to: ↑ 2 Changed at 2010-01-18T01:59:31Z by davidsarah

Replying to davidsarah:

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

I meant, if the directory already exists.

comment:4 Changed at 2010-01-18T02:11:47Z by davidsarah

  • Summary changed from webapi t=mkdir: behavior when directory already exists? to webapi t=mkdir-with-children and mkdir-immutable: behavior when directory already exists?

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

comment:5 Changed at 2010-01-18T02:15:21Z by davidsarah

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

comment:6 Changed at 2010-01-19T06:54:34Z by warner

sounds good to me

comment:7 Changed at 2010-01-24T22:44:53Z by davidsarah

  • Keywords forward-compatibility added

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.

Changed at 2010-01-24T22:51:26Z by davidsarah

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

comment:8 Changed at 2010-01-24T22:53:08Z by davidsarah

  • Keywords review-needed added

Review needed for 1.6.

comment:9 Changed at 2010-01-26T04:10:07Z by zooko

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

comment:10 Changed at 2010-01-26T18:06:46Z by davidsarah

  • Keywords reliability forward-compatibility review-needed removed
  • Priority changed from major to minor

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.

comment:11 Changed at 2010-02-27T06:41:06Z by zooko

  • Milestone changed from undecided to 1.7.0

comment:12 Changed at 2010-06-17T04:36:33Z by zooko

  • Milestone changed from 1.7.0 to eventually
Note: See TracTickets for help on using tickets.