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

Auto casting to String on set?? #22

Closed
davglass opened this issue Nov 24, 2013 · 5 comments
Closed

Auto casting to String on set?? #22

davglass opened this issue Nov 24, 2013 · 5 comments

Comments

@davglass
Copy link

I ran into this with my latest LevelDown abstraction:
https://github.com/rvagg/abstract-leveldown/blob/master/abstract-leveldown.js#L87

if (!this._isBuffer(value) && !process.browser)
    value = String(value)

Anyway we can make that an option and have process.browser default it to true? This way the abstraction can deal with the encoding?

My issue is that my backend server fully supports JSON encoding. Even if I said encoding: 'json' in my options, it's still casting that to a string on set. I'd like to have it just send the raw data to my abstraction and let me deal with the encoding.

If this is ok, I'll work on a PR just wanted to ask first :)

@juliangruber
Copy link
Member

If I remember correctly we discussed this before with with level.js, @maxogden?

This is getting to a point where levelup is progressing into nodeup, i.e. a common database abstraction layer and not primarily an interface to a leveldb backend.

I agree that in this case the backend should take care of the encoding, but it can't be automatic, you still have to set encoding:json if you store or retrieve JSON, otherwise: On set it converts your data to a String. On get it can't know whether the data originally was a String or an object.

@rvagg
Copy link
Member

rvagg commented Nov 25, 2013

@davglass a PR would probably be a better place for discussion on this so we can consider more what the impact might be elsewhere. We just have to tread very carefully here because the assumption thus far has been that the backend will only ever store String or Buffer types.

Regarding your backend, is it a native Node backend or is there still a native layer or even an HTTP layer in between? Is it possible that there is a JSON encoding and decoding going on anyway and hence there would be no cost difference in letting LevelUP to do the encode and decode vs your binding layer? A JSON object can't just come out of V8 as a "JSON object", it has to be serialised and de-serialised regardless of whether the backend supports JSON or not so it might be worth questioning whether it's even worth complicating it here?

@davglass
Copy link
Author

I didn't submit a PR since I wasn't sure if it was appropriate or not :)

I'm using a DB backend at Yahoo called Sherpa, it has a full REST library that we all have to use. It expects some sort of value to be given to it. Whether it's JSON or a String. So I would rather just pass it on to this existing library instead of "mucking" with the information.

It's default storage is JSON, but it also has the ability to set arbitrary keys and submit the difference. So the value may be just a string and the key may be a key on the actual object stored. Instead of putting that logic in my adapter layer, I just wanted to pass the raw value off to the backend and let it handle it.

@rvagg
Copy link
Member

rvagg commented Nov 26, 2013

Unfortunately you have LevelUP to contend with also: https://github.com/rvagg/node-levelup/blob/master/lib/util.js#L34-L69 as it's expecting to have to deal with String or Buffer and nothing else; the easiest way around this is to use a custom encoding that passes on raw JS objects through to the backend db but only if you're using Sherpa. I'd still suggest that letting LevelUP do the JSON.stringify for you and then passing that string on to Sherpa via REST would be the easiest since it's going to happen in the chain somewhere (even if it's done in here).
But anyway, happy to consider alternatives for storage where it makes sense.

@vweevers
Copy link
Member

Anyway we can make that an option and have process.browser default it to true? This way the abstraction can deal with the encoding?

Since then, we added _serializeKey and _serializeValue, which indeed allows an implementation to choose not to stringify, or in fact do whatever it wants. Implementations like memdown and level-js use this serialization mechanism to allow storage of many JS types.

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

No branches or pull requests

4 participants