From 8706bfa65d958c1805fcd01236fb366ef9d29478 Mon Sep 17 00:00:00 2001 From: Harry Brundage Date: Fri, 8 Nov 2024 15:05:01 +0000 Subject: [PATCH] fix: allow filters using false-y but not undefined values or ranges Before this change, the filter code was erroneously omitting filters that checked if a cell value was equal to `0`, as `0` is not truth-y in JS. It is however a valid byte string to check against inside bigtable, so we should still send it along as a filter. This bug applied to both `{value: 0}` exact value filters, or `{value: {start: 0}}` style range filters, and this commit fixes both. --- src/filter.ts | 16 +++++----- test/filter.ts | 81 +++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 73 insertions(+), 24 deletions(-) diff --git a/src/filter.ts b/src/filter.ts index aa2883a8c..8e8435b58 100644 --- a/src/filter.ts +++ b/src/filter.ts @@ -1,3 +1,4 @@ +/* eslint-disable eqeqeq */ // Copyright 2016 Google LLC // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -213,17 +214,17 @@ export class Filter { * ``` */ static createRange( - start: BoundData | null, - end: BoundData | null, + start: BoundData | null | undefined, + end: BoundData | null | undefined, key: string ) { const range = {}; - if (start) { + if (start != null) { Object.assign(range, createBound('start', start, key)); } - if (end) { + if (end != null) { Object.assign(range, createBound('end', end, key)); } @@ -233,7 +234,8 @@ export class Filter { const isInclusive = boundData.inclusive !== false; const boundKey = boundName + key + (isInclusive ? 'Closed' : 'Open'); const bound = {} as {[index: string]: {}}; - bound[boundKey] = Mutation.convertToBytes(boundData.value || boundData); + const value = boundData.value != null ? boundData.value : boundData; + bound[boundKey] = Mutation.convertToBytes(value); return bound; } } @@ -994,14 +996,14 @@ export class Filter { v = value as ValueFilter; } - if (v.value) { + if (v.value != null) { const valueReg = Mutation.convertToBytes( Filter.convertToRegExpString(v.value) ); this.set('valueRegexFilter', valueReg); } - if (v.start || v.end) { + if (v.start != null || v.end != null) { const range = Filter.createRange(v.start!, v.end!, 'Value'); this.set('valueRangeFilter', range); } diff --git a/test/filter.ts b/test/filter.ts index e3041523c..9ec17e009 100644 --- a/test/filter.ts +++ b/test/filter.ts @@ -22,7 +22,7 @@ import {Row} from '../src/row'; const sandbox = sinon.createSandbox(); const FakeMutation = { - convertToBytes: sandbox.spy(value => { + convertToBytes: sandbox.stub().callsFake(value => { return value; }), createTimeRange: sandbox.stub(), @@ -531,19 +531,14 @@ describe('Bigtable/Filter', () => { .stub(Filter, 'convertToRegExpString') .returns(fakeRegExValue); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const bytesSpy = ((FakeMutation as any).convertToBytes = sandbox.spy( - () => { - return fakeConvertedValue; - } - )); + FakeMutation.convertToBytes.resetBehavior(); + FakeMutation.convertToBytes.callsFake(() => fakeConvertedValue); filter.set = (filterName, val) => { assert.strictEqual(filterName, 'valueRegexFilter'); assert.strictEqual(fakeConvertedValue, val); assert(regSpy.calledWithExactly(value.value)); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - assert((bytesSpy as any).calledWithExactly(fakeRegExValue)); + assert(FakeMutation.convertToBytes.calledWithExactly(fakeRegExValue)); regSpy.restore(); done(); }; @@ -561,19 +556,14 @@ describe('Bigtable/Filter', () => { .stub(Filter, 'convertToRegExpString') .returns(fakeRegExValue); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const bytesSpy = ((FakeMutation.convertToBytes as any) = sandbox.spy( - () => { - return fakeConvertedValue; - } - )); + FakeMutation.convertToBytes.resetBehavior(); + FakeMutation.convertToBytes.callsFake(() => fakeConvertedValue); filter.set = (filterName, val) => { assert.strictEqual(filterName, 'valueRegexFilter'); assert.strictEqual(fakeConvertedValue, val); assert(regSpy.calledWithExactly(value)); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - assert((bytesSpy as any).calledWithExactly(fakeRegExValue)); + assert(FakeMutation.convertToBytes.calledWithExactly(fakeRegExValue)); regSpy.restore(); done(); }; @@ -601,6 +591,63 @@ describe('Bigtable/Filter', () => { filter.value(value); }); + it('should accept value filters that test against 0', done => { + const value = { + value: '0', + }; + const fakeRegExValue = '000'; + const fakeConvertedValue = '111'; + + const regSpy = sandbox + .stub(Filter, 'convertToRegExpString') + .returns(fakeRegExValue); + + FakeMutation.convertToBytes.resetBehavior(); + FakeMutation.convertToBytes.callsFake(() => fakeConvertedValue); + + filter.set = (filterName, val) => { + assert.strictEqual(filterName, 'valueRegexFilter'); + assert.strictEqual(fakeConvertedValue, val); + assert(regSpy.calledWithExactly(value.value)); + assert(FakeMutation.convertToBytes.calledWithExactly(fakeRegExValue)); + regSpy.restore(); + done(); + }; + + filter.value(value); + }); + + it('should accept value filters that test against a range starting with a falsey value like 0', done => { + sandbox.restore(); + FakeMutation.convertToBytes.resetBehavior(); + FakeMutation.convertToBytes.callsFake(value => value); + + const filter = Filter.parse({value: {start: 0}}); + assert.deepStrictEqual(filter, {valueRangeFilter: {startValueClosed: 0}}); + + const filter2 = Filter.parse({ + value: {start: {value: 0, inclusive: false}}, + }); + assert.deepStrictEqual(filter2, {valueRangeFilter: {startValueOpen: 0}}); + + done(); + }); + + it('should accept value filters that test against a range ending with a falsey value like 0', done => { + sandbox.restore(); + FakeMutation.convertToBytes.resetBehavior(); + FakeMutation.convertToBytes.callsFake(value => value); + + const filter = Filter.parse({value: {end: 0}}); + assert.deepStrictEqual(filter, {valueRangeFilter: {endValueClosed: 0}}); + + const filter2 = Filter.parse({ + value: {end: {value: 0, inclusive: false}}, + }); + assert.deepStrictEqual(filter2, {valueRangeFilter: {endValueOpen: 0}}); + done(); + }); + it('should apply the strip label transformer', done => { const value = { strip: true,