Unable to pass null as a parameter value

[pdfabbro]

Hi,

I am calling createQueryJob using this library ("@google-cloud/bigquery": "^5.5.0"). I have a parameter that is an array. Originally my code was failing for empty arrays because I wasn't providing the type for the param. So I added the type and all is good. However, if I pass null as the value of the param, the code breaks. I think the issue is with this line here:

https://github.com/googleapis/nodejs-bigquery/blob/master/src/bigquery.ts#L1107

Since value is null, value.type blows up. I think it should perhaps be updated to if (value !== null && value.type)?

[stephenplusplus]

Thanks for reporting!

@steffnay I believe you added this in #873. Do you think _getValue() should check that value exists before checking for the type property, or is it an indication of a bigger problem that we are passing it null in the first place?

[hammad2506]

Taking directly from BigQuery documentation, this would fail:

bigquery.query({
  query: [
    'SELECT url',
    'FROM `publicdata.samples.github_nested`',
    'WHERE repository.owner = ?'
  ].join(' '),

  params: [null],

  types: ['string']
}, function(err, rows) {});

bigquery.query(query).then(function(data) {
  const rows = data[0];
});

So would its named counterpart

bigquery.query({
  query: "SELECT url, FROM \`publicdata.samples.github_nested\` WHERE repository.owner = @owner",
  params: {owner: null},
  types: {owner: 'string'}
}, function(err, rows) {});

bigquery.query(query).then(function(data) {
  const rows = data[0];
});

[pdfabbro]

As a side note, I think this documentation https://googleapis.dev/nodejs/bigquery/latest/BigQuery.html#createQueryJob should be updated to say that "query" can be a string or an object. Without looking at the source code you wouldn't know that you can pass types via the query property (or even params for that matter).

It does say "A query string, following the BigQuery query syntax, of the query to execute.", but I think it should probably reference this https://googleapis.dev/nodejs/bigquery/latest/BigQuery.html#query and say it can be a string that represents the query or an object with any of the Query properties

[steffnay]

@stephenplusplus yeah, I did add this and the bug slipped through because we were missing a test case with the null value. Checking that value exists should be enough to fix it. I opened #920.

@hammad, the error your getting should be fixed with #920! Unfortunately, however, I think you still won't be able to use query parameters in the manner shown in the example code you provided. Those two queries will return Operands of = cannot be literal NULL. If you use the SQL syntax that is expected for handling null values like this, you will not be able to use a parameterized query.

Here are some examples of what I mean:

Case: Incorrect SQL syntax for null.
Resulting SQL error: Operands of = cannot be literal NULL

bigquery.query({
  query: [
    'SELECT url',
    'FROM `publicdata.samples.github_nested`',
    'WHERE repository.owner = ?'
  ].join(' '),
  params: [null],
  types: ['string']
}, function(err, rows) {});

Case: Correct SQL syntax, but query parameters not supported.
Resulting SQL error: Expected keyword FALSE or keyword NULL or keyword TRUE but got "?"

bigquery.query({
  query: [
    'SELECT url',
    'FROM `publicdata.samples.github_nested`',
    'WHERE repository.owner is ?'
  ].join(' '),
  params: [null],
 types: ['string']
}, function(err, rows) {});

My SQL is pretty basic, so this may be a nonissue for you, but figured I'd note those couple of things just in case you continue to encounter friction after my PR is merged.

[pdfabbro]

Yup, in our case we're properly checking for null in the query, but we just can't pass it over via the api so we've had to do workarounds like casting to string, etc. Thanks very much @steffnay for fixing this.

[hammad2506]

@steffnay I think query parameters with null values should be supported. This is an essential use case, also the example in my original comment was taken from BQ documentation so anything else would be inconsistent.

I think we should follow @stephenplusplus's suggestion here and short circuit null values to support them
#920 (comment)

Thanks again for looking into this.

[radek-vizlib]

Same issue here, #920 solves the problem

[whiterook6]

Until this is merged, how are we supposed to support null parameters?

[steffnay]

@whiterook6 Should be good to go now.

[whiterook6]

@whiterook6 Should be good to go now.

Thank for merging this. How do I get this updated code? Do I need to wait for the next release?