Add _serializeRange?

[vweevers]

I was thinking about serialization in leveldown. The following would work for keys and values (without logic for null or undefined because those are now rejected early by abstract-leveldown):

LevelDOWN.prototype._serializeKey =
LevelDOWN.prototype._serializeValue = function (datum) {
  return Buffer.isBuffer(datum) ? datum : String(datum)
}

But seeing as _serializeKey is now also used on range options (where null and undefined are valid) and this is a valid use case:

var db = levelup(leveldown('db'))

And we explain in the abstract-leveldown readme that:

An abstract-leveldown implementation is expected to either encode these [empty or nullish] range option types, serialize them, delegate to an underlying store, or finally, ignore them.

This means we should do:

LevelDOWN.prototype._serializeKey = function (key) {
  // Ignore nullish as range option
  return key == null ? '' : Buffer.isBuffer(key) ? key : String(key)
}

So to make the distinction between serializing range options and keys clearer, I had an idea: what if we add _serializeRange (name TBD) as:

AbstractLevelDOWN.prototype._serializeRange = function (opt) {
  return this._serializeKey(opt)
}

Then leveldown can do:

LevelDOWN.prototype._serializeKey =
LevelDOWN.prototype._serializeValue = function (datum) {
  return Buffer.isBuffer(datum) ? datum : String(datum)
}

LevelDOWN.prototype._serializeRange = function (opt) {
  return opt == null ? '' : this._serializeKey(opt)
}

Not sure if this is better, just throwing it out there. This might be a case of second system syndrome ;)

[vweevers]

I also thought about doing:

AbstractLevelDOWN.prototype._serializeRange = function (opt) {
  return opt == null ? '' : this._serializeKey(opt)
}

But then we're back to making assumptions about the semantics of null as well as ''. That leveldown ignores '' is an implementation detail. Better said: we shouldn't fall in the trap again of applying expected leveldown behavior (which should ignore '') to abstract-leveldown (which has no defined behavior, by design).

[vweevers]

I forgot that the target in iterator.seek(target) is now also serialized. Leveldown should ignore (or effectively, reject) null there too and "serializeRange" makes less sense now, at least as a name.

In addition, leveldown does a target.length !== 0 check, which happens after serialization, while in all other operations, abstract-leveldown does a key.length !== 0 check, which happens before serialization.

Ugh.

[vweevers]

In addition, leveldown does a target.length !== 0 check, which happens after serialization, while in all other operations, abstract-leveldown does a key.length !== 0 check, which happens before serialization.

That's OK, because the check by abstract-leveldown is meant for keys only, not for range options or seek targets (which are similar conceptually). I wonder though. What if, instead of throwing on an empty target, leveldown would seek to the first key? Because that's what db.iterator({ gt: '' }).next() does.

If I had a dog, I would walk it. I'll come back to this.

[ralphtheninja]

If I had a dog, I would walk it. I'll come back to this.

:dog: :walking_man:

[vweevers]

What if, instead of throwing on an empty target, leveldown would seek to the first key? Because that's what db.iterator({ gt: '' }).next() does.

Nah. If we want the ability to seek to the first key, an explicit feature like .seekToFirst() would be much easier to describe. It's hard to say what db.iterator({ gt: 'foo' }).seek('') should do, if it doesn't throw.

Also, while I was hoping to land on a clear set of rules in v6, regarding nullish, serialization, etc - which we did achieve for the most part - seek() is a new feature. It's okay to be fuzzy on the rules here.

Lastly, because it would be used by seek(), I think it's also too soon to introduce _serializeRange.

[vweevers]

Another thought, regarding:

An abstract-leveldown implementation is expected to either encode these [empty or nullish] range option types, serialize them, delegate to an underlying store, or finally, ignore them.

In the interest of simpilfying things, would it really be so bad if leveldown were to serialize null to 'null' and undefined to 'undefined'? E.g. just do:

LevelDOWN.prototype._serializeKey = function (key) {
  return Buffer.isBuffer(key) ? key : String(key)
}

Seeing as abstract-leveldown@6 has no defined behavior for these types, why should leveldown? Then we can say to users: if you want null or undefined to have a meaning, you must use an encoding that supports these types.

@ralphtheninja WDYT?

[ralphtheninja]

Sorry, I haven't been following this that closely, away in Germany. Still need feedback on something?

[vweevers]

For anyone stumbling upon this brain fart of a thread: we decided in Level/leveldown#506 that yes, it is fine for leveldown to have no defined behavior.