Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 278 Fix #315

Merged
merged 4 commits into from
Oct 5, 2018
Merged

Issue 278 Fix #315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19bc19b
Fix for Issue278
muraliQlogic Sep 28, 2018
692bfe6
revert
muraliQlogic Sep 28, 2018
718badb
Issue#278 fix
muraliQlogic Oct 3, 2018
e7e3a65
converted createPrefixRange_ to public method
muraliQlogic Oct 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 23 additions & 4 deletions src/table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,11 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`
* @returns {object} range
*
* @example
* Table.createPrefixRange_('start');
* const Bigtable = require('@google-cloud/bigtable');
* const bigtable = new Bigtable();
* const instance = bigtable.instance('my-instance');
* const table = instance.table('prezzy');
* table.createPrefixRange('start');
* // => {
* // start: 'start',
* // end: {
Expand All @@ -112,7 +116,7 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`
* // }
* // }
*/
static createPrefixRange_(start) {
createPrefixRange(start) {
const prefix = start.replace(new RegExp('[\xff]+$'), '');
let endKey = '';

Expand Down Expand Up @@ -401,6 +405,11 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`
let numRequestsMade = 0;

if (options.start || options.end) {
if (options.ranges || options.prefix || options.prefixes) {
throw new Error(
'start/end should be used exclusively to ranges/prefix/prefixes.'
);
}
ranges.push({
start: options.start,
end: options.end,
Expand All @@ -412,12 +421,22 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`
}

if (options.prefix) {
ranges.push(Table.createPrefixRange_(options.prefix));
if (options.ranges || options.start || options.end || options.prefixes) {
throw new Error(
'prefix should be used exclusively to ranges/start/end/prefixes.'
);
}
ranges.push(this.createPrefixRange(options.prefix));
}

if (options.prefixes) {
if (options.ranges || options.start || options.end || options.prefix) {
throw new Error(
'prefixes should be used exclusively to ranges/start/end/prefix.'
);
}
options.prefixes.forEach(prefix => {
ranges.push(Table.createPrefixRange_(prefix));
ranges.push(this.createPrefixRange(prefix));
});
}

Expand Down
108 changes: 97 additions & 11 deletions test/table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,41 +214,41 @@ describe('Bigtable/Table', function() {
});
});

describe('createPrefixRange_', function() {
describe('createPrefixRange', function() {
it('should create a range from the prefix', function() {
assert.deepStrictEqual(Table.createPrefixRange_('start'), {
assert.deepStrictEqual(table.createPrefixRange('start'), {
start: 'start',
end: {
value: 'staru',
inclusive: false,
},
});

assert.deepStrictEqual(Table.createPrefixRange_('X\xff'), {
assert.deepStrictEqual(table.createPrefixRange('X\xff'), {
start: 'X\xff',
end: {
value: 'Y',
inclusive: false,
},
});

assert.deepStrictEqual(Table.createPrefixRange_('xoo\xff'), {
assert.deepStrictEqual(table.createPrefixRange('xoo\xff'), {
start: 'xoo\xff',
end: {
value: 'xop',
inclusive: false,
},
});

assert.deepStrictEqual(Table.createPrefixRange_('a\xffb'), {
assert.deepStrictEqual(table.createPrefixRange('a\xffb'), {
start: 'a\xffb',
end: {
value: 'a\xffc',
inclusive: false,
},
});

assert.deepStrictEqual(Table.createPrefixRange_('com.google.'), {
assert.deepStrictEqual(table.createPrefixRange('com.google.'), {
start: 'com.google.',
end: {
value: 'com.google/',
Expand All @@ -258,15 +258,15 @@ describe('Bigtable/Table', function() {
});

it('should create an inclusive bound when the prefix is empty', function() {
assert.deepStrictEqual(Table.createPrefixRange_('\xff'), {
assert.deepStrictEqual(table.createPrefixRange('\xff'), {
start: '\xff',
end: {
value: '',
inclusive: true,
},
});

assert.deepStrictEqual(Table.createPrefixRange_(''), {
assert.deepStrictEqual(table.createPrefixRange(''), {
start: '',
end: {
value: '',
Expand Down Expand Up @@ -561,13 +561,99 @@ describe('Bigtable/Table', function() {
table.createReadStream(options);
});

it('should throw if ranges and start is set together', function() {
const options = {
ranges: [
{
start: 'a',
end: 'b',
},
{
start: 'c',
end: 'd',
},
],
start: 'a',
};
assert.throws(function() {
table.createReadStream(options, assert.ifError);
}, /start\/end should be used exclusively to ranges\/prefix\/prefixes\./);
});

it('should throw if ranges and end is set together', function() {
const options = {
ranges: [
{
start: 'a',
end: 'b',
},
{
start: 'c',
end: 'd',
},
],
end: 'a',
};
assert.throws(function() {
table.createReadStream(options, assert.ifError);
}, /start\/end should be used exclusively to ranges\/prefix\/prefixes\./);
});

it('should throw if ranges and prefix is set together', function() {
const options = {
ranges: [
{
start: 'a',
end: 'b',
},
{
start: 'c',
end: 'd',
},
],
prefix: 'a',
};
assert.throws(function() {
table.createReadStream(options, assert.ifError);
}, /prefix should be used exclusively to ranges\/start\/end\/prefixes\./);
});

it('should throw if ranges and prefixes is set together', function() {
const options = {
ranges: [
{
start: 'a',
end: 'b',
},
{
start: 'c',
end: 'd',
},
],
prefixes: [{prefix: 'a'}],
};
assert.throws(function() {
table.createReadStream(options, assert.ifError);
}, /prefixes should be used exclusively to ranges\/start\/end\/prefix\./);
});

it('should throw if prefix and start is set together', function() {
const options = {
start: 'a',
prefix: 'a',
};
assert.throws(function() {
table.createReadStream(options, assert.ifError);
}, /start\/end should be used exclusively to ranges\/prefix\/prefixes\./);
});

describe('prefixes', function() {
beforeEach(function() {
FakeFilter.createRange = common.util.noop;
});

afterEach(function() {
Table.createPrefixRange_.restore();
table.createPrefixRange.restore();
});

it('should transform the prefix into a range', function(done) {
Expand All @@ -580,7 +666,7 @@ describe('Bigtable/Table', function() {
const fakePrefix = 'abc';

const prefixSpy = sinon
.stub(Table, 'createPrefixRange_')
.stub(table, 'createPrefixRange')
.callsFake(function() {
return fakePrefixRange;
});
Expand Down Expand Up @@ -614,7 +700,7 @@ describe('Bigtable/Table', function() {
{start: 'def', end: 'deg'},
];
const prefixSpy = sinon
.stub(Table, 'createPrefixRange_')
.stub(table, 'createPrefixRange')
.callsFake(function() {
const callIndex = prefixSpy.callCount - 1;
return prefixRanges[callIndex];
Expand Down