planetscale/database-js

NULL types are mishandled

mattrobenolt opened this issue · 2 comments

It seems, due to a quirk in how a protobuf is translated into JSON, the NULL type is left out from the field because it's integer value is 0: https://github.com/vitessio/vitess/blob/main/proto/query.proto#L111-L112

What happens here is an HTTP response that looks like this for a query such as select null, 1.

"result": {
    "fields": [
      {
        "name": "null"
      },
      {
        "name": ":vtg1",
        "type": "INT64"
      }
    ],
    "rows": [
      {
        "lengths": [
          "-1",
          "1"
        ],
        "values": "MQ=="
      }
    ]
  }
}

The quirk here is that type is entirely left off of the fields, resulting in us decoding the row incorrectly.

The equivalent of after decoded by the driver is:

{
  headers: [ 'null', ':vtg1' ],
  types: { null: undefined, ':vtg1': 'INT64' },
  fields: [ { name: 'null' }, { name: ':vtg1', type: 'INT64' } ],
  rows: [ { null: null, ':vtg1': '1' } ],
  rowsAffected: null,
  insertId: null,
  size: 1,
  statement: 'select null, 1',
  time: 194
}

As we can see, the null column has an undefined type, rather than something such as "NULL" as I'd expect.

Even worse, but albeit an extremely edge case, on a query like select null, we actually throw an error:

DOMException [InvalidCharacterError]: The string to be decoded is not correctly encoded.
    at new DOMException (node:internal/per_context/domexception:53:5)
    at __node_internal_ (node:internal/util:505:10)
    at atob (node:buffer:1288:11)
    at decodeRow (file:///xxx/node_modules/@planetscale/database/dist/index.js:161:20)
    at parseObjectRow (file:///xxx/node_modules/@planetscale/database/dist/index.js:149:17)
    at file:///xxx/node_modules/@planetscale/database/dist/index.js:158:88
    at Array.map (<anonymous>)
    at parse (file:///xxx/node_modules/@planetscale/database/dist/index.js:158:17)
    at Connection.execute (file:///xxx/node_modules/@planetscale/database/dist/index.js:86:31)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

The HTTP response we get is:

  "result": {
    "fields": [
      {
        "name": "null"
      }
    ],
    "rows": [
      {
        "lengths": [
          "-1"
        ]
      }
    ]
  }

My assumption here again is a similar quirk, and that values is just a missing key entirely since it'd be an empty string representing no data. It's stemming from within atob, so didn't check exactly what's going on, but my assumption is we're passing undefined in there, rather than anything else.

My hunch here though is we need to explicitly handle the NULL type better, and can probably fix both issues together.

To add to this, I'd argue that for our API, we should stub in a type: 'NULL' just for our response for our Field type. I'm not a typescript person by any means, but Field.type is declared as a string and wouldn't expect that to be an undefined anyways, so I'd probably suggest we stub in NULL as a type since it'd be expected to have something there.

So I think something like: https://github.com/planetscale/database-js/blob/main/src/index.ts#L307

Change to atob(row.values || '') will catch the exception being thrown. I'm not entirely sure if this is considered good typescript or not.

Gonna try to PR for both of these cases.