mosaic icon indicating copy to clipboard operation
mosaic copied to clipboard

Type mismatch between Field and Ref

Open willium opened this issue 9 months ago • 0 comments

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.

willium avatar Apr 29 '24 16:04 willium