mosaic
mosaic copied to clipboard
Type mismatch between Field and Ref
I've using Mosaic from within TypeScript so I authored some type declarations.
I don't stand by their correctness, but in the process of doing so, I noticed a type mismatch that might lead to bugs.
Coordinator calls a client's fields()
method, and expects the client to return an array of objects with roughly this type:
export interface Field {
table: string;
column: string;
stats?: Set<keyof typeof Stats> | (keyof typeof Stats)[];
}
As you can see here: https://github.com/uwdata/mosaic/blob/04d68e14d5e0efb1ea2c480b04b6aedb2784367b/packages/core/src/Coordinator.js#L138-L141
and here: https://github.com/uwdata/mosaic/blob/04d68e14d5e0efb1ea2c480b04b6aedb2784367b/packages/core/src/util/field-info.js#L26-L34
and here: https://github.com/uwdata/mosaic/blob/04d68e14d5e0efb1ea2c480b04b6aedb2784367b/packages/core/src/util/field-info.js#L71-L80
However, in some of the clients, like Table, the fields method implementation looks like this: https://github.com/uwdata/mosaic/blob/04d68e14d5e0efb1ea2c480b04b6aedb2784367b/packages/inputs/src/Table.js#L90-L92
which uses this method on Ref that returns a new instance of Ref: https://github.com/uwdata/mosaic/blob/04d68e14d5e0efb1ea2c480b04b6aedb2784367b/packages/sql/src/ref.js#L94-L100
However, Ref does not require that the table
and column
properties are non-null, as you can see in the toString()
method:
https://github.com/uwdata/mosaic/blob/04d68e14d5e0efb1ea2c480b04b6aedb2784367b/packages/sql/src/ref.js#L27-L36
So, in cases like this where we use a Ref as if it implemented Field without any checks, we might end up using null as if it were a string.