Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting a new key with an empty value creates a "dir" key #349

Closed
AaronO opened this issue Nov 30, 2013 · 10 comments
Closed

Setting a new key with an empty value creates a "dir" key #349

AaronO opened this issue Nov 30, 2013 · 10 comments

Comments

@AaronO
Copy link

AaronO commented Nov 30, 2013

The expected behavior should be to create a normal "key" with simply no value.

This behavior of creating a directory can cause obvious issues, since directories can be updated with subsequent sets for example, ...

Version : 0.2.0-rc1

Reproduce (unexpected) :

$ curl -L http://127.0.0.1:4001/v2/keys/foo -XPUT -d value=""
{"action":"delete","key":"/foo","dir":true,"modifiedIndex":2}
$ curl -L http://127.0.0.1:4001/v2/keys/foo -XPUT -d value="NEW_VALUE"
{"errorCode":102,"message":"Not A File","cause":"/foo","index":2}
$ # We should be able to update this key, since it was not our intention to create a dir, but a key (just empty to start with)

Reproduce (expected) :

$ # Delete the key in case it existed before
$ curl -L http://127.0.0.1:4001/v2/keys/foo?recursive=true -XDELETE
$ curl -L  http://localhost:4001/v2/http://127.0.0.1:4001/v2/keys/foo -XPUT -d value="xx"
{"action":"set","key":"/foo","value":"xx","modifiedIndex":4}
$ # This works as expected (normal use case)
@philips
Copy link
Contributor

philips commented Nov 30, 2013

I agree this is a little prickly of an API. I guess we should add a parameter like mkdir=true that will error if value is set.

@philips
Copy link
Contributor

philips commented Nov 30, 2013

@benbjohnson @xiangli-cmu I looked at the store and it seems it relies on the same logic as the API, that if a node has an empty value it is a directory. I guess we will have to change the internal format of the store to have a type?

@xiang90
Copy link
Contributor

xiang90 commented Dec 1, 2013

@AaronO We do not allow 1. a key with an empty value 2. a dir with value for now. So the expected behavior is to create a dir when no value is given. Is there any reason that you want to create a key with no value or a dir with value? We can do that if there is a clear need.

Also I cannot understand your example and do not think etcd will produce the similar result.

@philips
Copy link
Contributor

philips commented Dec 1, 2013

@xiangli-cmu Yes, I can't reproduce the case that @AaronO has above:

curl -L http://127.0.0.1:4001/v2/keys/akey -XPUT -d value=""
{"action":"set","key":"/akey","dir":true,"modifiedIndex":4}
 curl -L http://127.0.0.1:4001/v2/keys/akey -XPUT -d value=bar
{"errorCode":102,"message":"Not A File","cause":"/akey","index":4}

Although, I can see how this will really easily cause programming errors. A simple solution that we can do on the API side without changing the store, for now, is to:

  1. Require something like op=mkdir if the value is empty on the API side to protect from programming errors
  2. Enforce that the value be set to a non-empty value for keys, we can add the feature for empty value later

The current API is going to make it too easy for people to create clients that accidentally create directories. You were right on this one.

What do you think?

@philips
Copy link
Contributor

philips commented Dec 1, 2013

@AaronO I started a discussion on the mailing list about this: http://news.gmane.org/gmane.comp.distributed.etcd

@AaronO
Copy link
Author

AaronO commented Dec 1, 2013

In many ways etcd can be compared to a typical file system (they both offer key/value storage, namepsace/directory listing ...)

@xiangli-cmu Thus 2. seems fairly natural, since in a typical filesystem a directory stores no data, and serves mainly as a "namespace" for it's children (so developers assume this by default, I guess).

However 1. is more confusing since one can create an empty file with no data in a file system and most other key/value stores (Redis, ...).

One can easily work around not having empty values, but I think most programmers, because of the FS metaphor or how other k/v stores behave will view this behavior as non obvious (also this behavior should be documented for those writing applications or client libraries on top of etcd).

@philips @xiangli-cmu Sorry, there was a typo in my reproduce command, it's fixed now (I wrote value=bar instead of value=""). So you should be able to reproduce it now.

@philips I read your message on the mailing list, and I think it's a reasonable suggestion.

@xiang90
Copy link
Contributor

xiang90 commented Dec 11, 2013

@AaronO We solved the problem be adding a dir flag. And empty key is allowed.

@xiang90 xiang90 closed this as completed Dec 11, 2013
@AaronO
Copy link
Author

AaronO commented Dec 11, 2013

@xiangli-cmu Is this in the 0.2.0-rc2 release ?

@xiang90
Copy link
Contributor

xiang90 commented Dec 11, 2013

@AaronO I do not think so. It is in the current master. This #376 add that feature.

@philips
Copy link
Contributor

philips commented Dec 11, 2013

@AaronO We will cut another rc soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants