feat(params): adds optional param types #599

steffnay · 2019-12-26T23:28:35Z

Adds ability to optionally provide query parameter types when performing queries.

Fixes #400
🦕

…ypes

codecov · 2019-12-27T17:31:00Z

Codecov Report

Merging #599 into master will not change coverage.
The diff coverage is 45.02%.

@@           Coverage Diff           @@
##           master     #599   +/-   ##
=======================================
  Coverage   94.74%   94.74%           
=======================================
  Files           7        7           
  Lines        6218     6218           
  Branches      358      358           
=======================================
  Hits         5891     5891           
  Misses        326      326           
  Partials        1        1

Impacted Files	Coverage Δ
src/index.ts	`100% <100%> (ø)`	⬆️
src/routine.ts	`19.58% <19.58%> (ø)`	⬆️
src/dataset.ts	`93.7% <72.54%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c650f5...1f160a2. Read the comment docs.

bcoe

This is looking pretty slick 😄

Biggest thing that initially jumped out at me is I think we could potentially use IStandardSqlDataType rather than any perhaps? and I'd dig into whether there's a way to get a string representation of this type so that we don't duplicate the list of types.

Might be worth asking @alexander-fenster if this is possible.

bcoe · 2019-12-30T17:59:37Z

src/bigquery.ts

-        type: 'ARRAY',
-        arrayType: BigQuery.getType_(value[0]),
-      };
+      if (value.length === 0 || value[0] === null) {


could someone provide undefined, or null is a special case I'm guessing.

As far as I know, undefined shouldn't be an option, just null

src/bigquery.ts

alexander-fenster · 2019-12-30T19:23:37Z

I don't know much about this library (it's not a GAPIC), but I generally agree that we should avoid using any. It might make sense to have this one merged and then have a separate PR to get rid of all explicitly used any types in the code since I'm guessing most of the types should present in .d.ts.

JustinBeckwith · 2019-12-27T18:24:06Z

src/bigquery.ts

@@ -42,6 +42,7 @@ import {
 } from './table';
 import {GoogleErrorBody} from '@google-cloud/common/build/src/util';
 import bigquery from './types';
+import {type} from 'os';


Was this supposed to come in?

JustinBeckwith · 2019-12-27T18:24:51Z

src/bigquery.ts

@@ -94,6 +95,8 @@ export type Query = JobRequest<bigquery.IJobConfigurationQuery> & {
  // tslint:disable-next-line no-any
  params?: any[] | {[param: string]: any};
  dryRun?: boolean;
+  // tslint:disable-next-line: no-any
+  types?: any[] | {[type: string]: any};


Is this truly any? It looks like there's an array of types below that define what this can look like.

You're right, I can change this.

src/bigquery.ts

stephenplusplus · 2020-01-16T18:27:39Z

src/bigquery.ts

+          };
+        }),
+      };
+    } else if (is.boolean(providedType)) {


Is it valid/expected for a user to provide a boolean as a type?

Yeah, it's a BigQuery type that can be detected so they should be able to provide it as well.

In this code path, it looks like the "providedType" would be true or false, which would end up returning {type: false}, as opposed to {type: 'BOOL'}.

Oh, I see. Fixed.

src/bigquery.ts

stephenplusplus · 2020-01-23T15:39:54Z

Sorry for the delay @steffnay.

I believe we'll want a new describe block just to test getTypeDescriptorFromProvidedType_. That would handle all of the checks for if it throws when expected, recurses itself as intended, etc. Tests for other describes can then remain isolated to only verify behavior that happens in their respective functions.

I wrote up a working example of how you can verify that a sibling method is being called with the correct arguments, and that the value it returns is being used properly. This template should be repeatable for the different code paths, but let me know if you get stuck or have more questions about how to apply this elsewhere.

This is a test that would go under createQueryJob -> SQL parameters -> positional.

it('should convert value and type to query parameter', done => {
  const fakeQueryParameter = {fake: 'query parameter'};

  bq.createJob = (reqOpts: JobOptions) => {
    const queryParameters = reqOpts.configuration!.query!.queryParameters;
    assert.deepStrictEqual(queryParameters, [fakeQueryParameter]);
    done();
  };

  sandbox
    .stub(BigQuery, 'valueToQueryParameter_')
    .callsFake((value, type) => {
      assert.strictEqual(value, POSITIONAL_PARAMS[0]);
      assert.strictEqual(type, POSITIONAL_TYPES[0]);
      return fakeQueryParameter;
    });

  bq.createQueryJob({
    query: QUERY_STRING,
    params: POSITIONAL_PARAMS,
    types: POSITIONAL_TYPES,
  });
});

I hope this is helpful!

stephenplusplus · 2020-01-23T14:46:18Z

src/bigquery.ts

+          };
+        }),
+      };
+    } else if (is.boolean(providedType)) {


In this code path, it looks like the "providedType" would be true or false, which would end up returning {type: false}, as opposed to {type: 'BOOL'}.

test/bigquery.ts

stephenplusplus · 2020-01-24T15:10:08Z

test/bigquery.ts

+          types: INVALID_TYPES,
+        });
+      }, /Invalid type provided./);
+      done();


This done() and the ones that follow in this block can be removed. It's only needed when we want to hold the test from exiting prematurely while we wait for a callback to be executed. In this case, the assert.throws() will run sync and then exit the test block with a failure or a success.

I'm really curious about this because these tests that check for errors keep timing out if I don't call done(). Is there something else I should be doing to fix that?

I think you just need to remove the done argument from the test function's argument list:

- it('should ...', done => { + it('should ...', () => {

Oh, got it.

test/bigquery.ts

stephenplusplus · 2020-01-24T15:16:49Z

test/bigquery.ts

+        .callsFake(value_ => {
+          assert.strictEqual(value_, value);
+          setImmediate(done);
+          return {


We need to follow this through to verify this object we return is used as we expect.

test/bigquery.ts

src/bigquery.ts

JustinBeckwith · 2020-01-27T18:44:12Z

src/bigquery.ts

@@ -807,12 +873,18 @@ export class BigQuery extends common.Service {
   * @returns {object} A properly-formed `queryParameter` object.
   */
  // tslint:disable-next-line no-any
-  static valueToQueryParameter_(value: any) {
+  static valueToQueryParameter_(value: any, providedType: any = undefined) {


Is providedType really any? I'm surprised we can't further constrain this to {}. Side note - for undefined, you don't need to set a default value. You can say providedType?: {}

providedType can be a string, object, or array, so I don't understand how to make providedType?: {} work this way. I don't know how I need to refactor providedType[0] on line 899 to work with this change.

you could do something along the lines of:

providedType: string|{[key: string]: string}|string[]

☝️ | is used to allow for multiple types.

@bcoe The array type can also be populated with string, array, or object. Similarly, the object can have values that are arrays. How do I specify that without using any?

@steffnay you'd potentially probably want to start breaking it out into interfaces, but you'd do something along the lines of:

{[key: string]: string|string[]}

☝️ but as it gets complex like this, you'd probably want to start describing an interface. The advantage of putting in this work is that folks using the API have more safety, and it's easier to figure out what to pass to the API.

src/bigquery.ts

system-test/bigquery.ts

test/bigquery.ts

…uery into param-types

steffnay added 12 commits July 17, 2019 13:54

fix(docs): publish new version of docs with getTables documented

fa11961

update

37489fd

pull upstream master

690f652

update

cdb10c7

merge upstream

8137b2a

Merge branch 'master' of https://github.com/googleapis/nodejs-bigquery

172e47f

Merge branch 'master' of https://github.com/googleapis/nodejs-bigquery

7f1e88b

Merge branch 'master' of https://github.com/googleapis/nodejs-bigquery

7c7a582

draft

d6d38bf

feat(namedParams): adds ability to provide optional named parameter t…

4bebe15

…ypes

feat(types): adds optional provided types for positional params

091803c

linted

9795176

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 26, 2019

refactor(params): refactors positional params and tests

0f2894d

chore: remove unnecessary test comments

54fa1f8

steffnay changed the title ~~Param types~~ feat(params): Adds optional param types Dec 27, 2019

steffnay changed the title ~~feat(params): Adds optional param types~~ feat(params): adds optional param types Dec 27, 2019

refactor(getTypes): removes redundant providedType logic

1566bdb

steffnay requested review from callmehiphop, bcoe and JustinBeckwith December 27, 2019 17:55

Merge branch 'master' into param-types

2a10c07

bcoe suggested changes Dec 30, 2019

View reviewed changes

bcoe and others added 2 commits December 30, 2019 11:55

Merge branch 'master' into param-types

9482ca3

refactor(params): adds tests and removes repeated null type logic

62d4a77

bcoe approved these changes Jan 13, 2020

View reviewed changes

JustinBeckwith reviewed Jan 16, 2020

View reviewed changes

stephenplusplus reviewed Jan 16, 2020

View reviewed changes

stephenplusplus reviewed Jan 23, 2020

View reviewed changes

adds tests

e08f577

steffnay marked this pull request as ready for review January 23, 2020 22:42

steffnay requested a review from stephenplusplus January 23, 2020 22:42

Merge branch 'master' into param-types

56f750e

steffnay added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 23, 2020

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 23, 2020

stephenplusplus reviewed Jan 24, 2020

View reviewed changes

Merge branch 'master' into param-types

e34edca

JustinBeckwith suggested changes Jan 27, 2020

View reviewed changes

steffnay added 6 commits January 27, 2020 12:31

test: refactors tests

0a6bc3c

Merge branch 'param-types' of https://github.com/steffnay/nodejs-bigq…

fd332cd

…uery into param-types

refactor: updates param types and tests

460b002

fixes test and updates providedType

bac3935

adds interface for providedTypes and logic for nested structs

558ef38

updates providedType type

7c650f5

bcoe approved these changes Jan 30, 2020

View reviewed changes

JustinBeckwith approved these changes Jan 30, 2020

View reviewed changes

Merge branch 'master' into param-types

1f160a2

steffnay added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 30, 2020

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 30, 2020

steffnay merged commit 008946a into googleapis:master Jan 30, 2020

steffnay deleted the param-types branch January 30, 2020 17:51

blunderbuss-gcf bot assigned stephenplusplus Aug 23, 2020

release-please bot mentioned this pull request May 18, 2022

chore(main): release 6.0.0 #1117

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(params): adds optional param types #599

feat(params): adds optional param types #599

steffnay commented Dec 26, 2019 •

edited

Loading

codecov bot commented Dec 27, 2019 •

edited

Loading

bcoe left a comment

bcoe Dec 30, 2019

steffnay Jan 11, 2020

alexander-fenster commented Dec 30, 2019

JustinBeckwith Dec 27, 2019

steffnay Jan 18, 2020

JustinBeckwith Dec 27, 2019

steffnay Jan 18, 2020

stephenplusplus Jan 16, 2020

steffnay Jan 18, 2020

stephenplusplus Jan 23, 2020

steffnay Jan 23, 2020

stephenplusplus commented Jan 23, 2020

stephenplusplus Jan 23, 2020

stephenplusplus Jan 24, 2020

steffnay Jan 24, 2020

stephenplusplus Jan 24, 2020

steffnay Jan 27, 2020

stephenplusplus Jan 24, 2020

JustinBeckwith Jan 27, 2020

steffnay Jan 27, 2020 •

edited

Loading

bcoe Jan 27, 2020

steffnay Jan 28, 2020 •

edited

Loading

bcoe Jan 28, 2020

feat(params): adds optional param types #599

feat(params): adds optional param types #599

Conversation

steffnay commented Dec 26, 2019 • edited Loading

codecov bot commented Dec 27, 2019 • edited Loading

Codecov Report

bcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-fenster commented Dec 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenplusplus commented Jan 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steffnay Jan 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steffnay Jan 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steffnay commented Dec 26, 2019 •

edited

Loading

codecov bot commented Dec 27, 2019 •

edited

Loading

steffnay Jan 27, 2020 •

edited

Loading

steffnay Jan 28, 2020 •

edited

Loading