range options should be serialized

[vweevers]

See #121 (comment).

@ralphtheninja @juliangruber should we include this in levelup@2? If so I can try to tackle this today/tomorrow.

[ralphtheninja]

Would be nice to know what happened here. Has this always been the case or did we break it?

[vweevers]

Has been the case since the _serialize functions were added. Simply forgot to include them in iterators.

[vweevers]

But no one has been bitten by this yet, it seems

[vweevers]

Even before _serialize (#85), when we just coerced non-buffers to strings, range options were not coerced AFAICT.

[vweevers]

@juliangruber can you think of any reason why this has been the case since forever? Are we missing something?

[juliangruber]

the null / undefined, buffer / utf8 etc situation was weird to me from the very beginning, and no one ever really understood the whole picture iirc

[juliangruber]

same as for example the process.browser checks

[ralphtheninja]

Maybe we could write some tests for it to to illustrate when it happens. What is the exact usage of the serialization code when doing db.createReadStream()? And when does it become a problem?

[vweevers]

So far I can't actually think of a use case that would fail without range option serialization. It merely feels like the right thing to do.

I know two Buffers can become equal when serialized to (UTF8) strings:

Buffer('90', 'hex').toString() === Buffer('80', 'hex').toString()

Maybe we can find something there, some case where the sort order is off.

[vweevers]

Then again, if we need to actively search for issues..

[vweevers]

If nothing else, I think it's a safety mechanism. In normal use of level(up), you'd use an encoding for non-standard types. Take:

db.createReadStream({ gt: 2 })

The db here should be level, levelup with encoding-down, or otherwise backed by an abstract-leveldown that supports numbers for keys.

In case it is not, abstract-leveldown must serialize the gt option to a type supported by the underlying store. If not to prevent errors, then for symmetry with serialized keys.

In the case of leveldown it'd be for symmetry, because the C++ currently ignores options that aren't buffers or strings. Without encoding-down and range option serialization, the following example would output two keys (a stringified 1 and 2) because the gt option is ignored, against expectation.

db.batch([
  { key: 1, value: 'one' },
  { key: 2, value: 'two' }
], function (err) {
  db.createKeyStream({ gt: 2 }).on('data', function (key) {
    console.log(key)
  })
})

[vweevers]

Even before _serialize (#85), when we just coerced non-buffers to strings, range options were not coerced AFAICT.

Makes sense because encodings used to be built-in, so users were protected against the above scenario (unless they used the id aka none encoding).

[vweevers]

@juliangruber @ralphtheninja in conclusion, I do think we need this, but it's not a priority. 🦆

[vweevers]

Found a use case. If localstorage-down wants to support Buffer keys, it needs to convert them to lexicographically ordered strings (with d64 or base64-lex). Ideally _serializeKey is the only code path used to achieve this. Then internally, localstorage-down can just work on strings, and use native comparators for sorting keys.