spanner: Unable to insert multiple rows in a single table.insert/transaction.insert call when some rows lack a value for nullable columns

[jscinoz]

In transaction-request.js, there is the following (L687-695):

mutation[method] = {
  table: table,
  columns: Object.keys(keyVals[0]),
  values: keyVals.map(function(keyVal) {
    return Object.keys(keyVal).map(function(key) {
      return codec.encode(keyVal[key]);
    });
  })
};

There are at least two bugs here:

1. When an array is provided to table.insert(), if some objects do not have a given key (which is perfectly valid if the column said key corresponds to is nullable), the array within values for this object will not be the same length as the columns array, and indices in columns will no longer correspond to the correct value in the values array for this row/object. This results in (best case) misleading error messages from Spanner about incorrect data types. Worse still, if it so happens that the value in the values array for this row and incorrect column index is the same type as the value for the correct column index, the insert request will succeed, and end up storing data in the wrong column entirely.

2. If a consumer of this library works around the issue above by populating missing nullable keys with null, as the code above relies on the order of values returned by Object.keys, the newly added values will be at the end of the array returned by Object.keys (which returns keys in insertion order), and again, the indices between the columns array, and the values array for affected rows will no longer align, with the same end result as what occurs with issue 1 above.

This could be fixed by iterating over all rows (rather than naively assuming all rows have identical keys to the first row) and collecting all unique keys into a single array which is subsequently sorted lexically, to build the value for columns. Likewise, the value for values would be constructed by mapping over columns (rather than Object.keys(keyVal)) and pulling out the corresponding value from keyVal to pass to codec.encode.

[stephenplusplus]

Thanks for reporting, @jscinoz! Would you be willing to send a PR?

[jscinoz]

Sure thing, I will implement the fix as described above and send through a PR today.

[jscinoz]

PR created: #2417

[vkedia]

@swcloud @bjwatson This would be a beta blocking issue since this can cause incorrect data to be stored in spanner.

[stephenplusplus]

I'm working on it now.

[vkedia]

Thanks!

[stephenplusplus]

New PR in #2426.