googleapis / nodejs-bigquery

Node.js client for Google Cloud BigQuery: A fast, economical and fully-managed enterprise data warehouse for large-scale data analytics.
https://cloud.google.com/bigquery/
Apache License 2.0
467 stars 211 forks source link

Direct usage of `queryParameters` broke with 7.6.0 #1357

Closed stefandoorn closed 7 months ago

stefandoorn commented 7 months ago

Hi,

Per version 7.6.0 the direct usage of queryParameters that we do in a project, got broken. The docs do not seem to mention the usage of it at all, but someone we figured out we could use it directly. Since the 7.6.0 release, our set-up is broken.

Rewriting it to use params & types, as shown in the docs, does make it work again.

Is queryParameters to be used directly at all?

If not, we'll refactor on our end and do not bother. In that case maybe it should be marked as private.

If it is part of the public API, I can provide code that shows what happens / breaks.

Thanks.

alvarowolfx commented 7 months ago

can you share which usage of queryParameters broke with 7.6.0 @stefandoorn ? I'm going to investigate here, but if you can provide more information that would be nice.

we often build the queryParameters internally, so building it externally might cause issues, but are you passing as IJobConfigurationQuery to create a query job ? I think in that scenario might be fair to use it, but maybe we are not properly respecting that

alvarowolfx commented 7 months ago

oh I can see here where things might have caused the breaking behavior on PR https://github.com/googleapis/nodejs-bigquery/pull/1337.

We should only change queryParameters when query.params is informed. I'll send a fix for this.

stefandoorn commented 7 months ago

@alvarowolfx Thanks :)

What we were doing was basically this:

config.queryParameters = [
  {
    name: parameterName,
    parameterValue: {
      value: 'valueHere',
    },
    parameterType: 'STRING',
  },
  ...
];

Then indeed we pass config (IJobConfigurationQuery) to the job.

Issue we were facing was that the parameter could not be found anymore in the query. That seems to align with your finding in #1359.

As I probably figured out myself that I could do this, and it's nowhere documented, it might be that you don't want me to use this directly.

I've refactored things on our end to use params & types.

If that assumption is correct, maybe it would make sense to mark the queryParameters as internal / private?

alvarowolfx commented 7 months ago

queryParameters is part of the public API, so you're 100% correct that this was a unintended breaking change. I'm just waiting for some approvals on my PR so I can push a bugfix release. Sorry for the inconvenience again and now that should be covered with automated tests.

alvarowolfx commented 7 months ago

@stefandoorn v7.6.1 was released with the fix.

stefandoorn commented 7 months ago

Thanks!