From 1e08b300d8550f46c90315f54438a50db83c0ea2 Mon Sep 17 00:00:00 2001 From: Vincent Weevers Date: Tue, 8 Nov 2022 15:06:35 +0100 Subject: [PATCH] Breaking: use undefined instead of error for non-existing entries (#49) Closes /~https://github.com/Level/community/issues/76. --- README.md | 20 ++++---------------- abstract-level.js | 14 ++++++-------- test/batch-test.js | 19 ++++++++++--------- test/deferred-open-test.js | 11 ++++++----- test/del-test.js | 12 +++++------- test/factory-test.js | 2 +- test/get-test.js | 19 ++++++++----------- test/put-get-del-test.js | 6 ++---- test/util.js | 13 +------------ 9 files changed, 43 insertions(+), 73 deletions(-) diff --git a/README.md b/README.md index 0d2a87a..bf481d6 100644 --- a/README.md +++ b/README.md @@ -99,7 +99,6 @@ - [`del` (deprecated)](#del-deprecated) - [`batch` (deprecated)](#batch-deprecated) - [Errors](#errors) - - [`LEVEL_NOT_FOUND`](#level_not_found) - [`LEVEL_DATABASE_NOT_OPEN`](#level_database_not_open) - [`LEVEL_DATABASE_NOT_CLOSED`](#level_database_not_closed) - [`LEVEL_ITERATOR_NOT_OPEN`](#level_iterator_not_open) @@ -296,7 +295,7 @@ Get a value from the database by `key`. The optional `options` object may contai - `keyEncoding`: custom key encoding for this operation, used to encode the `key`. - `valueEncoding`: custom value encoding for this operation, used to decode the value. -The `callback` function will be called with an error if the operation failed. If the key was not found, the error will have code [`LEVEL_NOT_FOUND`](#errors). If successful the first argument will be `null` and the second argument will be the value. If no callback is provided, a promise is returned. +The `callback` function will be called with an error if the operation failed. If successful the first argument will be nullish and the second argument will be the value. If the `key` was not found then the value will be `undefined`. If no callback is provided, a promise is returned. If the database indicates support of snapshots via `db.supports.snapshots` then `db.get()` _should_ read from a snapshot of the database, created at the time `db.get()` was called. This means it should not see the data of simultaneous write operations. However, this is currently not verified by the [abstract test suite](#test-suite). @@ -1138,10 +1137,6 @@ A database may also throw [`TypeError`](https://developer.mozilla.org/en-US/docs Error codes will be one of the following. -#### `LEVEL_NOT_FOUND` - -When a key was not found. - #### `LEVEL_DATABASE_NOT_OPEN` When an operation was made on a database while it was closing or closed. Or when a database failed to `open()` including when `close()` was called in the mean time, thus changing the eventual `status`. The error may have a `cause` property that explains a failure to open: @@ -1296,7 +1291,6 @@ Let's implement a basic in-memory database: ```js const { AbstractLevel } = require('abstract-level') -const ModuleError = require('module-error') class ExampleLevel extends AbstractLevel { // This in-memory example doesn't have a location argument @@ -1323,14 +1317,8 @@ class ExampleLevel extends AbstractLevel { } _get (key, options, callback) { + // Is undefined if not found const value = this._entries.get(key) - - if (value === undefined) { - return this.nextTick(callback, new ModuleError(`Key ${key} was not found`, { - code: 'LEVEL_NOT_FOUND' - })) - } - this.nextTick(callback, null, value) } @@ -1420,9 +1408,9 @@ The default `_close()` is a sensible noop and invokes `callback` on a next tick. ### `db._get(key, options, callback)` -Get a value by `key`. The `options` object will always have the following properties: `keyEncoding` and `valueEncoding`. If the key does not exist, call the `callback` function with an error that has code [`LEVEL_NOT_FOUND`](#errors). Otherwise call `callback` with `null` as the first argument and the value as the second. +Get a value by `key`. The `options` object will always have the following properties: `keyEncoding` and `valueEncoding`. If an error occurs, call the `callback` function with an error. Otherwise call `callback` with `null` as the first argument and the value as the second. If the `key` was not found then use `undefined` as value. -The default `_get()` invokes `callback` on a next tick with a `LEVEL_NOT_FOUND` error. It must be overridden. +The default `_get()` invokes `callback` on a next tick with an `undefined` value. It must be overridden. ### `db._getMany(keys, options, callback)` diff --git a/abstract-level.js b/abstract-level.js index 2e59407..751bd92 100644 --- a/abstract-level.js +++ b/abstract-level.js @@ -368,16 +368,14 @@ class AbstractLevel extends EventEmitter { this._get(this.prefixKey(keyEncoding.encode(key), keyFormat, true), options, (err, value) => { if (err) { - // Normalize not found error for backwards compatibility with abstract-leveldown and level(up) - if (err.code === 'LEVEL_NOT_FOUND' || err.notFound || /NotFound/i.test(err)) { - if (!err.code) err.code = 'LEVEL_NOT_FOUND' // Preferred way going forward - if (!err.notFound) err.notFound = true // Same as level-errors - if (!err.status) err.status = 404 // Same as level-errors - } - return callback(err) } + // Entry was not found + if (value === undefined) { + return callback() + } + try { value = valueEncoding.decode(value) } catch (err) { @@ -394,7 +392,7 @@ class AbstractLevel extends EventEmitter { } _get (key, options, callback) { - this.nextTick(callback, new Error('NotFound')) + this.nextTick(callback, null, undefined) } getMany (keys, options, callback) { diff --git a/test/batch-test.js b/test/batch-test.js index 8926429..ad38fe1 100644 --- a/test/batch-test.js +++ b/test/batch-test.js @@ -1,7 +1,7 @@ 'use strict' const { Buffer } = require('buffer') -const { verifyNotFoundError, assertAsync } = require('./util') +const { assertAsync } = require('./util') const { illegalKeys, illegalValues } = require('./util') let db @@ -206,9 +206,8 @@ exports.batch = function (test, testCommon) { }) db.get('foobatch2', function (err, value) { - t.ok(err, 'entry not found') - t.ok(typeof value === 'undefined', 'value is undefined') - t.ok(verifyNotFoundError(err), 'NotFound error') + t.error(err, 'no error') + t.is(value, undefined, 'not found') done() }) @@ -264,7 +263,7 @@ exports.batch = function (test, testCommon) { exports.atomic = function (test, testCommon) { test('test batch() is atomic', function (t) { - t.plan(4) + t.plan(6) let async = false @@ -276,11 +275,13 @@ exports.atomic = function (test, testCommon) { t.ok(err, 'should error') t.ok(async, 'callback is asynchronous') - db.get('foobah1', function (err) { - t.ok(err, 'should not be found') + db.get('foobah1', function (err, value) { + t.error(err, 'no error') + t.is(value, undefined, 'should not be found') }) - db.get('foobah3', function (err) { - t.ok(err, 'should not be found') + db.get('foobah3', function (err, value) { + t.error(err, 'no error') + t.is(value, undefined, 'should not be found') }) }) diff --git a/test/deferred-open-test.js b/test/deferred-open-test.js index 2129173..015f08d 100644 --- a/test/deferred-open-test.js +++ b/test/deferred-open-test.js @@ -13,8 +13,9 @@ exports.all = function (test, testCommon) { t.is(db.status, 'open', 'status is ok') if (--pendingGets <= 0) { - db.get('k4', { valueEncoding: 'utf8' }, function (err) { - t.ok(err) + db.get('k4', { valueEncoding: 'utf8' }, function (err, v) { + t.ifError(err, 'no get() error') + t.is(v, undefined, 'not found') db.close(t.ifError.bind(t)) }) } @@ -24,7 +25,7 @@ exports.all = function (test, testCommon) { // NOTE: copied from levelup test('deferred open(): put() and get() on new database', function (t) { - t.plan(15) + t.plan(16) // Open database without callback, opens in next tick const db = testCommon.factory() @@ -47,7 +48,7 @@ exports.all = function (test, testCommon) { // NOTE: copied from levelup test('deferred open(): batch() on new database', function (t) { - t.plan(13) + t.plan(14) // Open database without callback, opens in next tick const db = testCommon.factory() @@ -67,7 +68,7 @@ exports.all = function (test, testCommon) { // NOTE: copied from levelup test('deferred open(): chained batch() on new database', function (t) { - t.plan(13) + t.plan(14) // Open database without callback, opens in next tick const db = testCommon.factory() diff --git a/test/del-test.js b/test/del-test.js index bab9b0d..3cc53f8 100644 --- a/test/del-test.js +++ b/test/del-test.js @@ -1,6 +1,6 @@ 'use strict' -const { verifyNotFoundError, illegalKeys, assertAsync } = require('./util') +const { illegalKeys, assertAsync } = require('./util') let db @@ -36,9 +36,8 @@ exports.del = function (test, testCommon) { db.del('foo', function (err) { t.error(err) db.get('foo', function (err, value) { - t.ok(err, 'entry properly deleted') - t.ok(typeof value === 'undefined', 'value is undefined') - t.ok(verifyNotFoundError(err), 'NotFound error') + t.error(err, 'no error') + t.is(value, undefined, 'not found') t.end() }) }) @@ -51,9 +50,8 @@ exports.del = function (test, testCommon) { db.del('foo').then(function (err) { t.error(err) db.get('foo', function (err, value) { - t.ok(err, 'entry properly deleted') - t.ok(typeof value === 'undefined', 'value is undefined') - t.ok(verifyNotFoundError(err), 'NotFound error') + t.error(err, 'no error') + t.is(value, undefined, 'not found') t.end() }) }).catch(t.fail.bind(t)) diff --git a/test/factory-test.js b/test/factory-test.js index 519b84e..01202fc 100644 --- a/test/factory-test.js +++ b/test/factory-test.js @@ -41,7 +41,7 @@ module.exports = function (test, testCommon) { db1.put('key', 'value', function (err) { t.error(err, 'put key in db1') db2.get('key', function (err, value) { - t.ok(err, 'db2 should be empty') + t.error(err, 'no error') t.is(value, undefined, 'db2 should be empty') close() }) diff --git a/test/get-test.js b/test/get-test.js index 8d746c0..a3c8738 100644 --- a/test/get-test.js +++ b/test/get-test.js @@ -1,7 +1,7 @@ 'use strict' const isBuffer = require('is-buffer') -const { verifyNotFoundError, illegalKeys, assertAsync } = require('./util') +const { illegalKeys, assertAsync } = require('./util') let db @@ -69,9 +69,8 @@ exports.get = function (test, testCommon) { db.get('promises').then(function (value) { t.is(value, 'yes', 'got value without options') - db.get('not found').catch(function (err) { - t.ok(err, 'should error') - t.ok(verifyNotFoundError(err), 'correct error') + db.get('not found').then(function (value) { + t.is(value, undefined, 'not found') if (!db.supports.encodings.buffer) { return t.end() @@ -105,9 +104,8 @@ exports.get = function (test, testCommon) { for (let i = 0; i < 10; ++i) { db.get('not found', function (err, value) { - t.ok(err, 'should error') - t.ok(verifyNotFoundError(err), 'correct error') - t.ok(typeof value === 'undefined', 'value is undefined') + t.error(err, 'no error') + t.is(value, undefined, 'not found') done() }) } @@ -115,14 +113,13 @@ exports.get = function (test, testCommon) { }) test('test get() not found error is asynchronous', function (t) { - t.plan(4) + t.plan(3) let async = false db.get('not found', function (err, value) { - t.ok(err, 'should error') - t.ok(verifyNotFoundError(err), 'correct error') - t.ok(typeof value === 'undefined', 'value is undefined') + t.error(err, 'no error') + t.is(value, undefined, 'not found') t.ok(async, 'callback is asynchronous') }) diff --git a/test/put-get-del-test.js b/test/put-get-del-test.js index 37da10a..b7cf342 100644 --- a/test/put-get-del-test.js +++ b/test/put-get-del-test.js @@ -1,6 +1,5 @@ 'use strict' -const { verifyNotFoundError } = require('./util') const { Buffer } = require('buffer') let db @@ -28,9 +27,8 @@ function makeTest (test, type, key, value, expectedResult) { let async = false db.get(key, function (err, value) { - t.ok(err, 'entry properly deleted') - t.ok(verifyNotFoundError(err), 'correct error') - t.is(value, undefined, 'value is undefined') + t.error(err, 'no error') + t.is(value, undefined, 'not found') t.ok(async, 'callback is asynchronous') t.end() }) diff --git a/test/util.js b/test/util.js index d2ccbb7..b78f1f4 100644 --- a/test/util.js +++ b/test/util.js @@ -1,15 +1,10 @@ 'use strict' -const ModuleError = require('module-error') const { AbstractLevel, AbstractChainedBatch } = require('..') const { AbstractIterator, AbstractKeyIterator, AbstractValueIterator } = require('..') const spies = [] -exports.verifyNotFoundError = function (err) { - return err.code === 'LEVEL_NOT_FOUND' && err.notFound === true && err.status === 404 -} - exports.illegalKeys = [ { name: 'null key', key: null }, { name: 'undefined key', key: undefined } @@ -130,14 +125,8 @@ class MinimalLevel extends AbstractLevel { } _get (key, options, callback) { + // Is undefined if not found const value = this[kEntries].get(key) - - if (value === undefined) { - return this.nextTick(callback, new ModuleError(`Key ${key} was not found`, { - code: 'LEVEL_NOT_FOUND' - })) - } - this.nextTick(callback, null, value) }