Breaking: fix iterating buffer keys that contain bytes 196-255 (#88)

README.md

@@ -87,7 +87,7 @@ The `prefix` must be a string. If omitted, the effective prefix is two separator
 
 The optional `options` parameter has the following `subleveldown` specific properties:
 
-- `separator` _(string, default: `'!'`)_ Character for separating sublevel prefixes from user keys and each other. Should be outside the character (or byte) range of user keys.
+- `separator` _(string, default: `'!'`)_ Character for separating sublevel prefixes from user keys and each other. Must sort before characters used in prefixes. An error will be thrown if that's not the case.
 - `open` _(function)_ Optional open hook called when the underlying `levelup` instance has been opened. The hook receives a callback which must be called to finish opening.
 
 Any other `options` are passed along to the underlying [`levelup`][levelup] and [`encoding-down`][encoding-down] constructors. See their documentation for further details.

leveldown.js

@@ -7,7 +7,6 @@ var matchdown = require('./matchdown')
 var rangeOptions = 'start end gt gte lt lte'.split(' ')
 var defaultClear = abstract.AbstractLevelDOWN.prototype._clear
 var hasOwnProperty = Object.prototype.hasOwnProperty
-var END = Buffer.from([0xff])
 
 function concat (prefix, key, force) {
   if (typeof key === 'string' && (force || key.length)) return prefix + key
@@ -55,6 +54,15 @@ function SubDown (db, prefix, opts) {
   if (prefix[0] === separator) prefix = prefix.slice(1)
   if (prefix[prefix.length - 1] === separator) prefix = prefix.slice(0, -1)
 
+  var code = separator.charCodeAt(0) + 1
+  var ceiling = String.fromCharCode(code)
+
+  Buffer.from(prefix).forEach(function (byte) {
+    if (byte <= code) {
+      throw new RangeError('Prefix must sort after ' + code)
+    }
+  })
+
   this.db = db
   this.leveldown = null
   this.ownPrefix = separator + prefix + separator
@@ -68,8 +76,11 @@ function SubDown (db, prefix, opts) {
       return concat(self.prefix, x || '', true)
     },
     lt: function (x) {
-      if (Buffer.isBuffer(x) && !x.length) x = END
-      return concat(self.prefix, x || '\xff')
+      if (!x || isEmptyBuffer(x)) {
+        return self.prefix.slice(0, -1) + ceiling
+      } else {
+        return concat(self.prefix, x)
+      }
     }
   }
 
@@ -154,6 +165,10 @@ function isRangeOption (k) {
   return rangeOptions.indexOf(k) !== -1
 }
 
+function isEmptyBuffer (key) {
+  return Buffer.isBuffer(key) && key.length === 0
+}
+
 // TODO (refactor): use addRestOptions instead
 function extend (xopts, opts) {
   xopts.keys = opts.keys

test/index.js

@@ -13,35 +13,39 @@ var abstract = require('abstract-leveldown')
 var inherits = require('util').inherits
 
 // Test abstract-leveldown compliance
-suite({
-  test: test,
-  factory: function () {
-    return subdown(levelup(memdown()), 'test')
-  },
-
-  // Unsupported features
-  seek: false,
-  createIfMissing: false,
-  errorIfExists: false,
-
-  // Opt-in to new clear() tests
-  clear: true
+function runSuite (factory) {
+  suite({
+    test: test,
+    factory: factory,
+
+    // Unsupported features
+    seek: false,
+    createIfMissing: false,
+    errorIfExists: false,
+
+    // Opt-in to new clear() tests
+    clear: true
+  })
+}
+
+// Test basic prefix
+runSuite(function factory () {
+  return subdown(levelup(memdown()), 'test')
+})
+
+// Test empty prefix
+runSuite(function factory () {
+  return subdown(levelup(memdown()), '')
+})
+
+// Test custom separator
+runSuite(function factory () {
+  return subdown(levelup(memdown()), 'test', { separator: '%' })
 })
 
 // Test without a user-provided levelup layer
-suite({
-  test: test,
-  factory: function () {
-    return subdown(memdown(), 'test')
-  },
-
-  // Unsupported features
-  seek: false,
-  createIfMissing: false,
-  errorIfExists: false,
-
-  // Opt-in to new clear() tests
-  clear: true
+runSuite(function factory () {
+  return subdown(memdown(), 'test')
 })
 
 // Additional tests for this implementation
@@ -403,6 +407,66 @@ test('SubDb main function', function (t) {
   }
 })
 
+// https://github.com/Level/subleveldown/issues/87
+test('can store any key', function (t) {
+  t.test('iterating buffer keys with bytes above 196', function (t) {
+    t.plan(3)
+
+    var db = levelup(memdown())
+    var sub = subdb(db, 'test', { keyEncoding: 'binary' })
+
+    sub.once('open', function () {
+      const batch = sub.batch()
+
+      for (let i = 0; i < 256; i++) {
+        batch.put(Buffer.from([i]), 'test')
+      }
+
+      batch.write(function (err) {
+        t.ifError(err, 'no write error')
+
+        concat(sub.iterator(), function (err, entries) {
+          t.ifError(err, 'no concat error')
+          t.is(entries.length, 256, 'sub yields all entries')
+        })
+      })
+    })
+  })
+
+  t.test('range logic', function (t) {
+    const db = levelup(memdown())
+    const a = subdb(db, 'a', { separator: '#' })
+    const aA = subdb(a, 'a', { separator: '#' })
+    const b = subdb(db, 'b', { separator: '#' })
+    const next = after(3, verify)
+
+    a.once('open', next)
+    aA.once('open', next)
+    b.once('open', next)
+
+    function wrapper (sub) {
+      return reachdown(sub, 'subleveldown')._wrap
+    }
+
+    function verify () {
+      const ranges = [
+        wrapper(a).gt(),
+        wrapper(aA).gt(),
+        wrapper(aA).lt(),
+        wrapper(a).lt(),
+        wrapper(b).gt(),
+        wrapper(b).lt()
+      ]
+
+      t.same(ranges, ['#a#', '#a##a#', '#a##a$', '#a$', '#b#', '#b$'])
+      t.same(ranges.slice().sort(), ranges)
+      t.end()
+    }
+  })
+
+  t.end()
+})
+
 // Test that we peel off the levelup, deferred-leveldown and encoding-down
 // layers from db, but stop at any other intermediate layer like encrypt-down,
 // cachedown, etc.