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

LevelUP.createReadStream returns an empty buffer. #105

Closed
max-mapper opened this issue Jan 26, 2014 · 14 comments
Closed

LevelUP.createReadStream returns an empty buffer. #105

max-mapper opened this issue Jan 26, 2014 · 14 comments
Assignees

Comments

@max-mapper
Copy link

test case:

var levelup = require('levelup')
var concat = require('concat-stream')

var db = levelup('./test-data', { db: require('locket') })

db.put('foo', 'bar', function(err) {
  if (err) throw err
  db.createReadStream().pipe(concat(function(rows) {
    console.log('done:', rows)
  }))
})

output is:

done: <Buffer >

if you switch locket to memdown output is:

done: [ { key: 'foo', value: 'bar' } ]
@ghost ghost assigned bigeasy Jan 27, 2014
@bigeasy
Copy link
Owner

bigeasy commented Jan 27, 2014

Okay. Wow. I've been testing directly against Locket which is implemented according to AbstractLevelDOWN. What you're telling me is that LevelUP is an interface, and Locket is an implementation and you specify an implementation to LevelUP as a parameter, so that it is composition. LevelUP is not the C bindings to LevelDB but an abstraction.

All my testing is directly against Locket. That is, the tests that are not AbstractLevelDOWN tests. I need to update my tests to use the LevelUP API. Correct?

@bigeasy
Copy link
Owner

bigeasy commented Jan 27, 2014

I've made a change to one of my tests, testing through LevelUP. I'd really appreciate some feedback as to whether I'm on the right track to modeling actual API usage.

e0c0748

// @rvagg @dominictarr

@bigeasy
Copy link
Owner

bigeasy commented Jan 27, 2014

@ogd: I take it you were able to...

npm install && npm test

?

@max-mapper
Copy link
Author

yes the locket tests passed for me. If you dont want to deal with levelup I can try to reproduce using only the iterator API (what createReadStream in levelup uses)

@bigeasy
Copy link
Owner

bigeasy commented Jan 27, 2014

@ogd: Just looking for clarity on the architecture. LevelUP is pure-JavaScript, I see. I thought it was the C bindings for LevelDB. If people are going to be using LevelUP to use Locket, then yes, I want my tests to reflect the user experience.

@max-mapper
Copy link
Author

this reproduces the bug using only locket:

var locket = require('locket')

var db = new locket('./test-data')
db.open({ createIfMissing: true }, function() {
  db.put('foo', 'bar', function(err) {
    if (err) throw err

    // these are a copy of what levelup passes to the
    // leveldown iterator when it does createReadStream
    var iteratorOptions = {
      keys: true,
      values: true,
      createIfMissing: true,
      errorIfExists: false,
      keyEncoding: 'utf8',
      valueEncoding: 'utf8',
      compression: true,
      limit: -1,
      keyAsBuffer: false,
      valueAsBuffer: false
    }

    var iterator = db.iterator(iteratorOptions)
    iterator.next(function(key, val) {
      console.log('iterator first hit:', key, val)
      iterator.end(function() {
        iterator.end(function() {
          console.log('iterator ended')
        })
      })
    })
  })
})

I noticed that if you take limit: -1 out of the options it returns data, but with it in it returns undefined, so maybe the bug is in how locket implements the limit option?

@bigeasy
Copy link
Owner

bigeasy commented Jan 27, 2014

Yes. A less than zero limit is going to be interpreted as zero, not unlimited. Will fix this.

Thank you for the awesome test.

@max-mapper
Copy link
Author

@rvagg does this mean abstract-leveldown needs a test for negative limits?

@rvagg
Copy link

rvagg commented Jan 27, 2014

@maxogden probably!

@bigeasy
Copy link
Owner

bigeasy commented Jan 27, 2014

Need to step away from the computer for the evening. Fix is simple. Will fix in the morning. Thank you.

@bigeasy
Copy link
Owner

bigeasy commented Jan 27, 2014

Let me know if you're now able to proceed.

@max-mapper
Copy link
Author

another issue:

var locket = require('locket')

var db = new locket('./test-data')
db.open({ createIfMissing: true }, function() {
  db.put('ÿsÿ01', 'hello', function(err) {
    if (err) throw err

    // these are a copy of what levelup passes to the
    // leveldown iterator when it does createReadStream
    var iteratorOptions = {
      keys: true,
      values: true,
      createIfMissing: true,
      errorIfExists: false,
      keyEncoding: 'utf8',
      valueEncoding: 'binary',
      compression: true,
      init: true,
      defaults: true,
      writeBufferSize: 16777216,
      start: 'ÿdÿ',
      end: 'ÿdÿÿ',
      limit: -1,
      keyAsBuffer: false,
      valueAsBuffer: true
    }

    var iterator = db.iterator(iteratorOptions)
    iterator.next(function(key, val) {
      console.log('iterator first hit:', key, val)
      iterator.end(function() {
        console.log('iterator ended')
      })
    })
  })
})

the above code should not return any results but I get:

iterator first hit: null ÿsÿ01
iterator ended

@bigeasy
Copy link
Owner

bigeasy commented Jan 27, 2014

@maxogden: It will be a while before I can do my Locket for the day. Can you use lte for your upper bound in the mean time? That one is implemented and tested.

@bigeasy
Copy link
Owner

bigeasy commented Jan 27, 2014

@maxogden: Thought I'd posted this. I didn't implement end, but I did implement lte. It is a synonym. lte should work. I'll have a fix this afternoon during my Node.js time.

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

3 participants