table.createLoadJob has mutually exclusive polymorphic return

[zamnuts]

Affects @google-cloud/[email protected]

The table.createLoadJob is a callback-first method that has been promisified via promisifyAll, i.e. it doesn't meet the requirements of a non-promise method (Stream/constructor/etc). However, when the source argument is a string, the current implementation attempts a synchronous return type of Stream.Writable (EventEmitter). See https://github.com/googleapis/nodejs-bigquery/blob/v4.7.0/src/table.ts#L1226-L1248

This polymorphic return is problematic:

1. With callback and without await, Writable is returned
2. With callback and with await, Writable is returned
3. Without callback and without await, Promise (of Job? can't actually test this) is returned
4. Without callback and with await, Promise of Job is returned

The corresponding overloads are available:

createLoadJob(source: string): Writable; // Never reached
createLoadJob(source: string, callback: JobCallback): Writable; // 1, 2

// 4 is not documented as an overload, but is possible in the concrete signature:
createLoadJob(source: string): Promise<JobResponse> {...}

One of the tests expects a Writable without a supplied callback, but, upon promisifyAll refactor in the unit tests to actually be applied, it fails due to the method assuming to be used like a promise, which corresponds to scenario 3 in previous: https://github.com/googleapis/nodejs-bigquery/blob/v4.7.0/test/table.ts#L58-L64

In the current unit tests, this issue is not presented due to the promisifyAll stub, which removes the promisification functionality. See https://github.com/googleapis/nodejs-bigquery/blob/v4.7.0/test/table.ts#L58-L64

Example test cases (all succeed):

// fix the promisifyAll fake
const fakePfy = extend({}, pfy, {
  promisifyAll: (c: Function) => {
    if (c.name === 'Table') {
      promisified = true;
    }
    pfy.promisifyAll(c); // ADDED THIS
  },
});

// test cases
it('+cb +async', async () => {
  sandbox.stub(table, 'createWriteStream_').returns(new stream.Writable());
  const result = await table.createLoadJob(FILEPATH, () => {}); // noop
  assert(result instanceof stream.Writable);
  result.emit('job', {metadata: 'foo'});
});

it('+cb -async', (done) => {
  sandbox.stub(table, 'createWriteStream_').returns(new stream.Writable());
  const result = table.createLoadJob(FILEPATH, done);
  assert(result instanceof stream.Writable);
  result.emit('job', {metadata: 'bar'});
});

it('-cb +async', async () => {
  const w = new stream.Writable();
  sandbox.stub(table, 'createWriteStream_').returns(w);
  setImmediate(() => w.emit('job', {metadata: 'qux'}));
  const result = await table.createLoadJob(FILEPATH);
  assert.deepStrictEqual(result, [{metadata: 'qux'}, 'qux']);
});

it('-cb -async', () => {
  // cannot test this, table.createLoadJob always returns a Promise
});

Typically, combining promises, callbacks, and streams in a single function signature is mutually exclusive.

If a Writable result is really desired that coincides with the createLoadJob(source: string): Writable; signature, I recommend implementing a createLoadJobStream(source: string): Writable; method which will not be promisified.

[stephenplusplus]

I would guess that all the implementor wanted was an early return from the function, and all of the annotations suggesting stream support was done to get the linter off their back. Here's a diff of all I believe we want: https://gist.github.com/stephenplusplus/c20d164ff46aefe0fc0a7165667534a9

[zamnuts]

Thanks @stephenplusplus, that would be breaking API change, and wouldn't reveal the Stream whatsoever. Are we ok with that?

[stephenplusplus]

It's currently not possible to actually get the stream back, right? Would it only break TypeScript compilers if a user was under the impression they'd get a stream back?

[zamnuts]

In the current implementation, there are two scenarios that return the Writable stream, but considering it also invokes the supplied callback upon completion, i don't really see the point of returning the Writable. So yes, it is possible to get the stream back (the Writable).

1. With callback and without await, Writable is returned
2. With callback and with await, Writable is returned

Additionally, since it returns the Writable, which has a Readable already piped to it, the Writable can't be used except to listen for events like close; i.e. you wouldn't want to multiplex multiple Readables to it, nor can you read from the Writable since it is not a Duplex.

[steffnay]

@stephenplusplus @JustinBeckwith @shollyman I've not seen a breaking change handled in this library, so I'm curious if you can give @zamnuts and myself more feedback on this

[JustinBeckwith]

We are going to be doing a semver major release here in the next or so. If you want to make a breaking change, now would be a good time.

[stephenplusplus]

I tested this out, but it wouldn't actually return a stream to me:

'use strict'

const bigQuery = new (require('@google-cloud/bigquery')).BigQuery()

const dataset = bigQuery.dataset('stephen_has_a_new_dataset')
const table = dataset.table('stephen_has_a_new_table')
const schema = [{"type":"STRING","name":"Name"},{"type":"INTEGER","name":"Age"},{"type":"FLOAT","name":"Weight"},{"type":"BOOLEAN","name":"IsMagic"}]

;(async() => {
  await dataset.create()
  await table.create(schema)

  table.createLoadJob('./location-test-data.json')
    .on('error', console.error)
    .on('finish', console.log)
})()

(node:7504) UnhandledPromiseRejectionWarning: TypeError: table.createLoadJob(...).on is not a function
    at /Users/stephen/dev/play/gissue-640/index.js:14:6

I think we just need to move the return to after the lines that do the streaming work.

[zamnuts]

@JustinBeckwith in the next "what" or so? :) i pushed up a draft, still have some testing to do...

We are going to be doing a semver major release here in the next or so.

@stephenplusplus IIRC table.createLoadJob the way you have it will return a promise of a stream. check my test cases in the OP.

[stephenplusplus]

Ah, I'm caught up now. This will return a stream, like your test case shows (sorry):

table.createLoadJob('./location-test-data.json', (err, job, apiResponse) => {
  // These are the expected arguments. No stream.
  console.log({err, job, apiResponse})
})
.on('data', console.log) // doesn't error

IIRC table.createLoadJob the way you have it will return a promise of a stream.

It actually resolves the normally-expected arguments, the Job instance and the POJO apiResponse:

table.createLoadJob('./location-test-data.json')
  .then(([job, apiResponse]) => {})

I don't believe there's a way to actually get the stream back where a Job or apiResponse was expected.

We never meant to allow a stream to be returned, or advertised it (outside of the TypeScript params, right?). So this is the gray area of breaking changes IMO, when an undocumented side-effect will break a user's code. (Documentation being these docs: https://googleapis.dev/nodejs/bigquery/latest/Table.html#createLoadJob, and not TypeScript IDE helpers)

But to be safe, and since we have one coming up anyway, sure, it's a breaking change. However, I don't think we need to do too much to fix this. Just not putting the return on the stream line, and instead below the stream logic should restore us to the original intentions of the function. Skip the idea that we need to return a stream at all.