From 6b399f7e3e581d726d119b207a0a170d605aa40b Mon Sep 17 00:00:00 2001 From: Silas Sewell Date: Thu, 15 Dec 2022 12:45:10 -0500 Subject: [PATCH] Handle backwards/zero index in watch (#136) Fixes #134 #135 --- .jshintrc | 3 +- lib/utils.js | 22 ++++++++++-- lib/watch.js | 18 +++++++--- test/utils.js | 44 +++++++++++++++++------- test/watch.js | 94 +++++++++++++++++++++++++++++++++++---------------- 5 files changed, 129 insertions(+), 52 deletions(-) diff --git a/.jshintrc b/.jshintrc index 7b201f0..f58c32d 100644 --- a/.jshintrc +++ b/.jshintrc @@ -1,6 +1,6 @@ { "node": true, - "esversion": 9, + "esversion": 11, "globals": { // Mocha "describe": false, @@ -15,7 +15,6 @@ "eqnull": true, "indent": 2, "latedef": true, - "newcap": true, "quotmark": "double", "trailing": true, "undef": true, diff --git a/lib/utils.js b/lib/utils.js index fae079b..b6bc49f 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -147,6 +147,21 @@ function parseDuration(value) { return (n * constants.DURATION_UNITS[m[2]]) / 1e6; } +/** + * Return BigInt or undefined if parse failed. + */ +function safeBigInt(value) { + if (value === undefined || value === null || value === "") { + return undefined; + } + + try { + return BigInt(value); + } catch (err) { + return undefined; + } +} + /** * Common options */ @@ -556,9 +571,9 @@ function createCheck(src) { * Has the Consul index changed. */ function hasIndexChanged(index, prevIndex) { - if (typeof index !== "string" || !index) return false; - if (typeof prevIndex !== "string" || !prevIndex) return true; - return index !== prevIndex; + if (typeof index !== "bigint" || index <= 0) return false; + if (typeof prevIndex !== "bigint") return true; + return index > prevIndex; } /** @@ -599,6 +614,7 @@ exports.options = options; exports.defaultCommonOptions = defaultCommonOptions; exports.clone = clone; exports.parseDuration = parseDuration; +exports.safeBigInt = safeBigInt; exports.setTimeoutContext = setTimeoutContext; exports.createServiceCheck = createServiceCheck; exports.createService = createService; diff --git a/lib/watch.js b/lib/watch.js index 9cdd820..4e75510 100644 --- a/lib/watch.js +++ b/lib/watch.js @@ -18,7 +18,7 @@ class Watch extends events.EventEmitter { let options = utils.normalizeKeys(opts.options || {}); options = utils.defaults(options, consul._defaults); options.wait = options.wait || "30s"; - options.index = options.index || "0"; + options.index = utils.safeBigInt(options.index || "0"); if ( typeof options.timeout !== "string" && @@ -150,14 +150,22 @@ class Watch extends events.EventEmitter { this._attempts = 0; this._attemptsMaxed = false; - const newIndex = res.headers["x-consul-index"]; - + const newIndex = utils.safeBigInt(res.headers["x-consul-index"]); if (newIndex === undefined) { return this._err(errors.Validation("Watch not supported"), res); } + if (newIndex === 0n) { + return this._err( + errors.Consul("Consul returned zero index value"), + res + ); + } + + const prevIndex = this._options.index; + const reset = prevIndex !== undefined && newIndex < prevIndex; - if (utils.hasIndexChanged(newIndex, this._options.index)) { - this._options.index = newIndex; + if (reset || utils.hasIndexChanged(newIndex, prevIndex)) { + this._options.index = reset ? 0n : newIndex; this.emit("change", data, res); } diff --git a/test/utils.js b/test/utils.js index efbe3a3..af1fbee 100644 --- a/test/utils.js +++ b/test/utils.js @@ -239,11 +239,28 @@ describe("utils", function () { should(utils.parseDuration("1.5s")).equal(1500); should(utils.parseDuration("10.03m")).equal(601800); - should(utils.parseDuration()).be.undefined; - should(utils.parseDuration("")).be.undefined; - should(utils.parseDuration(".")).be.undefined; - should(utils.parseDuration("10x")).be.undefined; - should(utils.parseDuration(".ms")).be.undefined; + should(utils.parseDuration()).be.undefined(); + should(utils.parseDuration("")).be.undefined(); + should(utils.parseDuration(".")).be.undefined(); + should(utils.parseDuration("10x")).be.undefined(); + should(utils.parseDuration(".ms")).be.undefined(); + }); + }); + + describe("safeBigInt", function () { + it("should work", function () { + should(utils.safeBigInt(0)).equal(0n); + should(utils.safeBigInt(-1)).equal(-1n); + should(utils.safeBigInt(500)).equal(500n); + should(utils.safeBigInt("0")).equal(0n); + should(utils.safeBigInt("-1")).equal(-1n); + should(utils.safeBigInt("500")).equal(500n); + + should(utils.safeBigInt("")).be.undefined(); + should(utils.safeBigInt("a")).be.undefined(); + should(utils.safeBigInt("1.0")).be.undefined(); + should(utils.safeBigInt(null)).be.undefined(); + should(utils.safeBigInt({})).be.undefined(); }); }); @@ -843,14 +860,15 @@ describe("utils", function () { it("should work", function () { should(utils.hasIndexChanged()).equal(false); should(utils.hasIndexChanged("")).equal(false); - should(utils.hasIndexChanged("1")).equal(true); - should(utils.hasIndexChanged("1", "")).equal(true); - should(utils.hasIndexChanged("10", "1")).equal(true); - should(utils.hasIndexChanged("0", "1")).equal(true); - should(utils.hasIndexChanged("1", "1")).equal(false); - should(utils.hasIndexChanged("1", "0")).equal(true); - should(utils.hasIndexChanged("2", "1")).equal(true); - should(utils.hasIndexChanged("2", "2")).equal(false); + should(utils.hasIndexChanged(0n)).equal(false); + should(utils.hasIndexChanged(1n)).equal(true); + should(utils.hasIndexChanged(1n, "")).equal(true); + should(utils.hasIndexChanged(10n, 1n)).equal(true); + should(utils.hasIndexChanged(0n, 1n)).equal(false); + should(utils.hasIndexChanged(1n, 1n)).equal(false); + should(utils.hasIndexChanged(1n, 0n)).equal(true); + should(utils.hasIndexChanged(2n, 1n)).equal(true); + should(utils.hasIndexChanged(2n, 2n)).equal(false); }); }); }); diff --git a/test/watch.js b/test/watch.js index 1d30c52..3519ae2 100644 --- a/test/watch.js +++ b/test/watch.js @@ -20,6 +20,12 @@ describe("Watch", function () { .get("/v1/kv/key1?index=10&wait=30s") .reply(200, [{ n: 5 }], { "X-Consul-Index": "15" }) .get("/v1/kv/key1?index=15&wait=30s") + .reply(200, [{ n: 6 }], { "X-Consul-Index": "14" }) + .get("/v1/kv/key1?index=0&wait=30s") + .reply(200, [{ n: 7 }], { "X-Consul-Index": "6" }) + .get("/v1/kv/key1?index=6&wait=30s") + .reply(200, [{ n: 8 }], { "X-Consul-Index": "0" }) + .get("/v1/kv/key1?index=6&wait=30s") .reply(400); const watch = this.consul.watch({ @@ -27,6 +33,14 @@ describe("Watch", function () { options: { key: "key1" }, }); + let doneCalled = false; + const safeDone = (err) => { + if (doneCalled) return; + doneCalled = true; + done(err); + watch.end(); + }; + should(watch.isRunning()).be.true(); should(watch.updateTime()).be.undefined(); @@ -40,6 +54,10 @@ describe("Watch", function () { const called = {}; watch.on("error", (err) => { + if (err.message.includes("Nock")) { + return safeDone(err); + } + called.error = true; errors.push(err); @@ -48,13 +66,17 @@ describe("Watch", function () { watch.on("cancel", () => { called.cancel = true; - should(list).eql([1, 4, 5]); + try { + should(list).eql([1, 4, 5, 6, 7]); - watch._run(); - watch._err(); + watch._run(); + watch._err(); - watch.end(); - should(watch.isRunning()).be.false(); + watch.end(); + should(watch.isRunning()).be.false(); + } catch (err) { + safeDone(err); + } }); watch.on("change", (data, res) => { @@ -62,34 +84,48 @@ describe("Watch", function () { list.push(data.n); - switch (res.headers["x-consul-index"]) { - case "5": - should(watch.isRunning()).be.true(); - should(watch.updateTime()).be.a.number(); - should(errors).be.empty(); - break; - case "10": - should(watch.isRunning()).be.true(); - should(watch.updateTime()).be.a.number(); - should(errors).have.length(1); - should(errors[0]).have.property( - "message", - "consul: kv.get: internal server error" - ); - break; - case "15": - break; - default: - break; + try { + switch (res.headers["x-consul-index"]) { + case "5": + should(watch.isRunning()).be.true(); + should(watch.updateTime()).be.a.Number(); + should(errors).be.empty(); + break; + case "10": + should(watch.isRunning()).be.true(); + should(watch.updateTime()).be.a.Number(); + should(errors).have.length(1); + should(errors[0]).have.property( + "message", + "consul: kv.get: internal server error" + ); + break; + case "15": + break; + default: + break; + } + } catch (err) { + safeDone(err); } }); watch.on("end", () => { - should(called).have.property("cancel", true); - should(called).have.property("change", true); - should(called).have.property("error", true); - - done(); + try { + should(called).have.property("cancel", true); + should(called).have.property("change", true); + should(called).have.property("error", true); + + should(errors).have.length(3); + should(errors[1]).have.property( + "message", + "Consul returned zero index value" + ); + + safeDone(); + } catch (err) { + safeDone(err); + } }); });