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

custom encoding options on db from client. #63

Open
dweinstein opened this issue Feb 15, 2015 · 10 comments
Open

custom encoding options on db from client. #63

dweinstein opened this issue Feb 15, 2015 · 10 comments

Comments

@dweinstein
Copy link

if you pass require('bytewise/hex') as the keyEncoding in a .batch, .put, .createReadStream... (as passed in the opts parameter) on the client it'll just receive an error back saying .encode is not a method.

I'm guessing this isn't something that could be stored in the manifest. So is this something that one would have to tweak/setup server-side? Shim the client's methods to do the key-encoding client side?

The documentation says binary encoding is supported, but maybe it would help other's to say custom encodings aren't. Bytewise seems to be a pretty fundamental encoding for level... I almost wish it were native like json and binary but maybe it's bloat. Thoughts?

Probably this is related to #17 and #12 but not certain.

@dweinstein
Copy link
Author

The test I used initially is this:

var getDb = require('./util').getDb;
var test = require('tape');
var binary = require('bops');
var bytewise = require('bytewise/hex');

  test('batch', function (t) {
    var opts = { keyEncoding: bytewise, valueEncoding : 'binary' };
    getDb(function (db, dispose) {
      db.put(['test', 'foo'], binary.from([1,2,3]), opts, function (err) {
        t.error(err);
        db.get(['test', 'foo'], opts, function (err, value) {
          t.error(err);
          t.ok(binary.is(value));
          t.equal(binary.to(value), binary.to(binary.from([1,2,3])));
          dispose();
          t.end();
        });
      });
    });
  });

error:

...
not ok 5 Error: Object #<Object> has no method 'encode'
...

@dominictarr
Copy link
Collaborator

ah... I think that multilevel doesn't actually implement this correctly. The quick fix is to encode your own keys using bytewise/hex and then pass that to multilevel as a string. The better fix is to make a pull request to multilevel that does this for all requests... in a way that is consistent with levelup.

@dweinstein
Copy link
Author

I set a couple breakpoints and:

break in node_modules/multilevel/lib/server.js:35
 33       if (method.type == 'async') {
 34         server.createLocalCall(name, function (args, cb) {
 35           debugger;
 36           access(server.sessionData, db, k, args);
 37           args.push(cb);
debug> repl
Press Ctrl + C to leave debug repl
> args
[ [ { type: 'put',
      key: [Object],
      value: 0 },
    { type: 'put',
      key: [Object],
      value: 0 },
    { type: 'put',
      key: [Object],
      value: 0 } ],
  { keyEncoding: { buffer: false, type: 'bytewise-hex' } } ]
 //                              ^-- missing .decode/.encode

I'll see if I can figure out what's going on but based on this the decode method didn't make it across (not super familiar with how the RPC stuff works here yet but I'll figure it out). I guess stepping back I'm not sure I'd expect the .encode and .decode methods from the the client to magically appear at the server, so maybe this is expected.

Hopefully I can work towards that PR... we'll see. :-)

@dominictarr
Copy link
Collaborator

Whats happening is the object is encoded with json which removes the function (that would be a massive security hole, like an sql injection)

Basically you need to encode the object before you send it, or make it so that multilevel encodes the object for you...

hmm, now that I think about it, multilevel does use an encoding that supports buffers, so if you set keyEncoding: require('bytewise/hex') on the server instance, then I think this will all work for you as is.

@juliangruber
Copy link
Owner

that will work but multilevel should still support encodings the same way as levelup, so i'll be working on this now

@juliangruber
Copy link
Owner

i'm working on an implementation that will work, but i'm thinking more and more that multilevel as a leveldown implementation will work better, so we don't need to reinvent levelup logic

@juliangruber
Copy link
Owner

relevant: #64 and Level/levelup#313

@mafintosh
Copy link

does this mean i can't do this anymore?

db.get('foo', {valueEncoding: myOwnEncoder}, cb)

i usually do that when using protocol-buffers, https://github.com/mafintosh/protocol-buffers#leveldb-encoding-compatibility

@juliangruber
Copy link
Owner

currently you can't do that from the client, but i don't remember this ever working with multilevel

@mafintosh
Copy link

@juliangruber ah whoops. meant to add the comment to Level/levelup#313 (added it there as well now)

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