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.

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

Cannot use 'prefix' and 'start' together #278

Closed
moander opened this issue Aug 21, 2018 · 14 comments
Closed

Cannot use 'prefix' and 'start' together #278

moander opened this issue Aug 21, 2018 · 14 comments
Assignees
Labels
api: bigtable Issues related to the googleapis/nodejs-bigtable API. 🚨 This issue needs some love. triage me I really want to be triaged. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@moander
Copy link

moander commented Aug 21, 2018

When prefix and start is used as read options only prefix is considered.

Expected result is to get rows b2 and b3.

Environment details

  • @google-cloud/bigtable version: 0.15.0

Steps to reproduce

await bt.createTable('test4', {
    families: [
        'c'
    ]
}).catch(swallowCode(6));


bt.table('test4').mutate([
    {
        key: 'a1',
        method: 'insert',
        data: { c: { test: 1 } }
    },
    {
        key: 'a2',
        method: 'insert',
        data: { c: { test: 2 } }
    },
    {
        key: 'a3',
        method: 'insert',
        data: { c: { test: 3 } }
    },
    {
        key: 'b1',
        method: 'insert',
        data: { c: { test: 4 } }
    },
    {
        key: 'b2',
        method: 'insert',
        data: { c: { test: 5 } }
    },
    {
        key: 'b3',
        method: 'insert',
        data: { c: { test: 6 } }
    },
])

let [rows] = await bt.table('test4').getRows({
    prefix: 'b',
    start: 'b2',
});

console.log('got rows', rows.map(r => r.id));

// Result:
// got rows [ 'b1', 'b2', 'b3' ]
@moander
Copy link
Author

moander commented Aug 21, 2018

This is weird. If you instead try the repro code using this mutation it will give you this result:

got rows [ 'b1', 'b2', 'b3', 'c1', 'c2', 'c3' ]
        bt.table('test4').mutate([
            {
                key: 'a1',
                method: 'insert',
                data: { c: { test: 1 } }
            },
            {
                key: 'a2',
                method: 'insert',
                data: { c: { test: 2 } }
            },
            {
                key: 'a3',
                method: 'insert',
                data: { c: { test: 3 } }
            },
            {
                key: 'b1',
                method: 'insert',
                data: { c: { test: 4 } }
            },
            {
                key: 'b2',
                method: 'insert',
                data: { c: { test: 5 } }
            },
            {
                key: 'b3',
                method: 'insert',
                data: { c: { test: 6 } }
            },
            {
                key: 'c1',
                method: 'insert',
                data: { c: { test: 4 } }
            },
            {
                key: 'c2',
                method: 'insert',
                data: { c: { test: 5 } }
            },
            {
                key: 'c3',
                method: 'insert',
                data: { c: { test: 6 } }
            },
        ])

@ajaaym
Copy link
Contributor

ajaaym commented Aug 21, 2018

can you try below? This should give you all row with prefix b starting from b2.

getRows({ start: 'b1', end: 'b' })

@JustinBeckwith JustinBeckwith added the triage me I really want to be triaged. label Aug 22, 2018
@moander
Copy link
Author

moander commented Aug 22, 2018

@ajaaym that gives me the following error:

Error: 3 INVALID_ARGUMENT: Error in field 'row_ranges' : Error in element #0 : start_key must be less than end_key

@ajaaym
Copy link
Contributor

ajaaym commented Aug 22, 2018

Sorry there was a typo. Use end: ‘c’ instead of end: ‘b’.

@moander
Copy link
Author

moander commented Aug 22, 2018

That won't help because I don't know the end in my actual code. I only know the prefix and the start key.

@ajaaym
Copy link
Contributor

ajaaym commented Aug 22, 2018

In that case you can use below.

let range = Table.createPrefixRange_(prefix);
range.start = 'b2';
getRows(range);

@JustinBeckwith JustinBeckwith added the 🚨 This issue needs some love. label Aug 26, 2018
@sduskis
Copy link
Contributor

sduskis commented Sep 6, 2018

I like ajaaym's solution. This also seems to work as intended. I don't think that we'll be doing more on this issue, unless there's a clear way forward.

@sduskis sduskis closed this as completed Sep 6, 2018
@moander
Copy link
Author

moander commented Sep 10, 2018

I disagree. The work around is fine but the implications here are huge.

Lets say you always use prefix´, but only sometimes you use start`. If the developer is not aware of this bug then he risks returning all the data from the table to the requestor.

@sduskis
Copy link
Contributor

sduskis commented Sep 12, 2018

Perhaps we can throw an exception if both are set. WDYT?

@moander
Copy link
Author

moander commented Sep 13, 2018

I can't use this library knowing you ignore reported security issues

@sduskis
Copy link
Contributor

sduskis commented Sep 17, 2018

@moander, why do you think this is a security issue?

@sduskis sduskis reopened this Sep 17, 2018
@sduskis
Copy link
Contributor

sduskis commented Sep 17, 2018

For Cloud Bigtable clients, prefix and start/end are meant to be used independently. The Node.js API doesn't make that clear.

@sduskis sduskis added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. and removed 🚨 This issue needs some love. triage me I really want to be triaged. labels Sep 21, 2018
@JustinBeckwith JustinBeckwith added 🚨 This issue needs some love. triage me I really want to be triaged. labels Sep 21, 2018
@sduskis sduskis added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. triage me I really want to be triaged. labels Sep 28, 2018
@JustinBeckwith JustinBeckwith added the triage me I really want to be triaged. label Sep 28, 2018
@sduskis sduskis removed the triage me I really want to be triaged. label Sep 28, 2018
@JustinBeckwith JustinBeckwith added the triage me I really want to be triaged. label Sep 28, 2018
@sduskis
Copy link
Contributor

sduskis commented Sep 28, 2018

There are four conflicting conditions that need to be exclusive:

  • a set of ranges
  • start / end
  • prefix
  • prefixes

There needs to be an exception if more than one of those conditions exist in the options, since there's no intuitive way to evaluate 2+ conditions.

@sduskis sduskis closed this as completed Sep 28, 2018
@sduskis sduskis reopened this Sep 28, 2018
@ghost ghost removed the priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. label Sep 28, 2018
@sduskis sduskis added priority: p2 Moderately-important priority. Fix may not be included in next release. and removed 🚨 This issue needs some love. triage me I really want to be triaged. labels Sep 28, 2018
@sduskis
Copy link
Contributor

sduskis commented Oct 5, 2018

The exceptions were added in #315

@sduskis sduskis closed this as completed Oct 5, 2018
@ghost ghost removed the priority: p2 Moderately-important priority. Fix may not be included in next release. label Oct 5, 2018
@google-cloud-label-sync google-cloud-label-sync bot added the api: bigtable Issues related to the googleapis/nodejs-bigtable API. label Jan 31, 2020
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. triage me I really want to be triaged. labels Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the googleapis/nodejs-bigtable API. 🚨 This issue needs some love. triage me I really want to be triaged. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

5 participants