expose failed operations as callback(err)

@stephenplusplus

Some upstream API methods exist, which can return a code 200, however, a portion of the request failed-- partial failure. We've been returning these partial failures to users separate from the err property of a callback(err). This is confusing, and we should just do it like this:

methodThatCanPartiallyFail('...', function (err, apiResponse) {
  if (err) {
    // err.code = 'INSERT_ERROR'
    // err.errors = [insert errors]
  }
})

Methods that need to be updated

bigquery/table#insert
bigtable/table#mutate
vision#detect

Back to you, @c0b...

am playing around streaming data into tables, with this nodejs-docs-samples code,

https://github.com/GoogleCloudPlatform/nodejs-docs-samples/blob/master/bigquery/tables.js#L194-L207

a table created with name:integer,value:string as schema but inserted name as string, the program runs ok says 1 row inserted but nothing show up in bigquery console, until I print the apiResponse I found the error, but it turns out the callback function's first err is a null, where it shouldn't be; if user has to check apiResponse anyway, that makes the first err check not meaningful

  table.insert(rows, function (err, insertErrors, apiResponse) {
    if (err) {          // why this branch not triggered
      return callback(err);
    }
    ...

$ bq mk new_dataset.new_table name:integer,value:string

$ node ./bigquery/tables.js insert new_dataset newtable '[{ "name": "abc" }]'
Inserted 1 row(s)!
{ datasetId: 'new_dataset',
  tableId: 'newtable',
  rows: [ { name: 'abc' } ],
  callback: [Function],
  err: null,
  insertErrors: 
   [ { errors: 
        [ { message: 'Cannot convert value to integer (bad value).',
            reason: 'invalid' } ],
       row: { name: 'abc' } } ],
  apiResponse: 
   { kind: 'bigquery#tableDataInsertAllResponse',
     insertErrors: 
      [ { index: 0,
          errors: 
           [ { reason: 'invalid',
               location: 'name',
               debugInfo: 'generic::invalid_argument: Cannot convert value to integer (bad value).',
               message: 'Cannot convert value to integer (bad value).' } ] } ] } }

The insertErrors is for this purpose, because you might be inserting several rows, most of which were successfully inserted, but only a handful that may have failed. The err is generally reserved for API errors (404, 503, etc), i.e. complete failures. If we used err for a partial failure, the user's instinct would likely be that the entire request failed.

I can see how this can be confusing, so other opinions and ideas for how we should handle this are welcome.

@c0b

@c0b did the explanation make anything clearer, or was that information you already knew? And any thoughts on if we should re-think our entire approach, or maybe solve it some other way?

that is the workaround I've also found; while I feel any error should be treated as an Error, to be passed to callback(err, ...), that would let users of this library feel more consistent to other Nodejs callbacks, a contract among many other libraries

when err is null, it means all rows inserted successful
only when err is not null, user need to check if 4xx bad request, or insertError means partially fail, user would retry for the failed rows, or whatever

That sounds good to me. We already populate err.errors[] with any errors the server returns, so we would just plop the insertErrors in there.

table.insert(rows, function (err, apiResponse) {
  if (err) {
    // err.code = 501 OR 'INSERT_ERROR'
    // err.errors = [server OR insert errors]
  }
})