Skip to content

Commit

Permalink
fix: allow filters using false-y but not undefined values or ranges
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
airhorns committed Nov 8, 2024
1 parent da373b5 commit 8706bfa
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 24 deletions.
16 changes: 9 additions & 7 deletions src/filter.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable eqeqeq */
// Copyright 2016 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -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));
}

Expand All @@ -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;
}
}
Expand Down Expand Up @@ -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);
}
Expand Down
81 changes: 64 additions & 17 deletions test/filter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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();
};
Expand All @@ -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();
};
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit 8706bfa

Please sign in to comment.