Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat (client): client-side support for int8 #665

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions clients/typescript/src/client/conversions/input.ts
Original file line number Diff line number Diff line change
Expand Up @@ -331,8 +331,7 @@ function isObject(v: any): boolean {
}

function isFilterObject(value: any): boolean {
// if it is an object it can only be a timestamp or a filter object
// because those are the only objects we support in where clauses
// if it is an object it can only be a data object or a filter object
return isObject(value) && !isDataObject(value)
}

Expand Down
5 changes: 5 additions & 0 deletions clients/typescript/src/client/conversions/sqlite.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ export function fromSqlite(v: any, pgType: PgType): any {
) {
// it's a serialised NaN
return NaN
} else if (pgType === PgBasicType.PG_INT8) {
// always return BigInts for PG_INT8 values
// because some drivers (e.g. wa-sqlite) return a regular JS number if the value fits into a JS number
// but we know that it should be a BigInt based on the column type
return BigInt(v)
} else {
return v
}
Expand Down
43 changes: 42 additions & 1 deletion clients/typescript/src/client/model/builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,34 @@ import { InvalidArgumentError } from '../validation/errors/invalidArgumentError'
import * as z from 'zod'
import { IShapeManager } from './shapes'
import Log from 'loglevel'
import { ExtendedTableSchema } from './schema'
import { PgBasicType } from '../conversions/types'
import { HKT } from '../util/hkt'

const squelPostgres = squel.useFlavour('postgres')
squelPostgres.registerValueHandler('bigint', function (bigint) {
return bigint.toString()
})

type AnyFindInput = FindInput<any, any, any, any, any>

export class Builder {
constructor(
private _tableName: string,
private _fields: string[],
private shapeManager: IShapeManager
private shapeManager: IShapeManager,
private _tableDescription: ExtendedTableSchema<
any,
any,
any,
any,
any,
any,
any,
any,
any,
HKT
>
) {}

create(i: CreateInput<any, any, any>): QueryBuilder {
Expand Down Expand Up @@ -196,10 +214,26 @@ export class Builder {
const fields = identificationFields
.filter((f) => this._fields.includes(f))
.concat(selectedFields)
.map((f) => this.castBigIntToText(f))

return q.fields(fields)
}

/**
* Casts a field to TEXT if it is of type BigInt
* because not all adapters deal well with BigInts
* (e.g. better-sqlite3 requires BigInt support to be enabled
* but then all integers are returned as BigInt...)
* The DAL will convert the string into a BigInt in the `fromSqlite` function from `../conversions/sqlite.ts`.
*/
private castBigIntToText(field: string) {
const pgType = this._tableDescription.fields.get(field)
if (pgType === PgBasicType.PG_INT8) {
return `cast(${field} as TEXT) AS ${field}`
}
return field
}

private addOrderBy(i: AnyFindInput, q: PostgresSelect): PostgresSelect {
if (typeof i.orderBy === 'undefined') return q
const orderByArray = Array.isArray(i.orderBy) ? i.orderBy : [i.orderBy]
Expand Down Expand Up @@ -243,6 +277,13 @@ export class Builder {
query: T
): T {
return this._fields.reduce((query, field) => {
// if field is of type BigInt cast the result to TEXT
// because not all adapters deal well with BigInts
// the DAL will convert the string into a BigInt in the `fromSqlite` function from `../conversions/sqlite.ts`.
const pgType = this._tableDescription.fields.get(field)
if (pgType === PgBasicType.PG_INT8) {
return query.returning(`cast(${field} as TEXT) AS ${field}`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This string should be returned from a function if it's used in multiple places like this. If, say, we add quoting for these field identifiers later, it'll be easier to make the change once and not worry about missing some occurrences that will be left unquoted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to introduce a local getField() function that returns the field?
something like:

private returnAllFields<T extends QueryBuilder & ReturningMixin>(
    query: T
  ): T {
    const getField = (field: string) => field 
    return this._fields.reduce((query, field) => {
      // if field is of type BigInt cast the result to TEXT
      // because not all adapters deal well with BigInts
      // the DAL will convert the string into a BigInt in the `fromSqlite` function from `../conversions/sqlite.ts`.
      const pgType = this._tableDescription.fields.get(field)
      if (pgType === PgBasicType.PG_INT8) {
        return query.returning(`cast(${getField()} as TEXT) AS ${getField()}`)
      }
      return query.returning(getField())
    }, query)
  }
}

I don't really see the value of adding this now as we merely use the field without any changes.
I do see the value once we do more than just using the field name, e.g. add quoting, but now it is literally the identity function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was only referring to the act of interpolating a variable into a string which is then passsed as SQL for execution by SQLite. Depending on the context, this can lead to SQL injection attacks or merely client-side fatal errors.

What I'm saying is that we should properly quote all identifiers and values when interpolating them into strings of SQL. It doesn't take much effort for a particular field name to break our client and as a consequence the app that depends on it. Here's a quick example:

$ sqlite3
SQLite version 3.36.0 2021-06-18 18:36:39
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
sqlite> create table foo(cast integer);
sqlite> insert into foo values (1), (2), (3);
sqlite> select * from foo;
1
2
3
sqlite> select cast(cast as text) from foo;
Error: near "as": syntax error
sqlite> select cast("cast" as text) from foo;
1
2
3

Perhaps the value in the field variable is already quoted? In that case, disregard this comment.

Copy link
Contributor Author

@kevin-dp kevin-dp Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, the arguments we pass to the function are already quoted, e.g.:

castIfNeeded(col, `"${col}"`)

}
return query.returning(field)
}, query)
}
Expand Down
9 changes: 7 additions & 2 deletions clients/typescript/src/client/model/table.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,17 @@ export class Table<
) {
this._fields = this._dbDescription.getFields(tableName)
const fieldNames = this._dbDescription.getFieldNames(tableName)
this._builder = new Builder(tableName, fieldNames, shapeManager)
const tableDescription = this._dbDescription.getTableDescription(tableName)
this._builder = new Builder(
tableName,
fieldNames,
shapeManager,
tableDescription
)
this._executor = new Executor(adapter, notifier, this._fields)
this._shapeManager = shapeManager
this._qualifiedTableName = new QualifiedTablename('main', tableName)
this._tables = new Map()
const tableDescription = this._dbDescription.getTableDescription(tableName)
this._schema = tableDescription.modelSchema
this.createSchema = //transformCreateSchema(
omitCountFromSelectAndIncludeSchema(tableDescription.createSchema) //, fields)
Expand Down
13 changes: 11 additions & 2 deletions clients/typescript/src/migrators/triggers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,12 @@ type ForeignKey = {

type ColumnName = string
type SQLiteType = string
type ColumnTypes = Record<ColumnName, SQLiteType>
type PgType = string
type ColumnType = {
sqliteType: SQLiteType
pgType: PgType
}
type ColumnTypes = Record<ColumnName, ColumnType>

export type Table = {
tableName: string
Expand Down Expand Up @@ -220,6 +225,7 @@ export function generateTriggers(tables: Tables): Statement[] {
* Joins the column names and values into a string of pairs of the form `'col1', val1, 'col2', val2, ...`
* that can be used to build a JSON object in a SQLite `json_object` function call.
* Values of type REAL are cast to text to avoid a bug in SQLite's `json_object` function (see below).
* Similarly, values of type INT8 (i.e. BigInts) are cast to text because JSON does not support BigInts.
*
* NOTE: There is a bug with SQLite's `json_object` function up to version 3.41.2
* that causes it to return an invalid JSON object if some value is +Infinity or -Infinity.
Expand Down Expand Up @@ -268,7 +274,10 @@ function joinColsForJSON(
// casts the value to TEXT if it is of type REAL
// to work around the bug in SQLite's `json_object` function
const castIfNeeded = (col: string, targettedCol: string) => {
if (colTypes[col] === 'REAL') {
const tpes = colTypes[col]
const sqliteType = tpes.sqliteType
const pgType = tpes.pgType
if (sqliteType === 'REAL' || pgType === 'INT8' || pgType === 'BIGINT') {
return `cast(${targettedCol} as TEXT)`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing this for the first time now, it's surprising we're not using a disciplined approach for interpolating fargments into SQL strings. I know it's out of scope for this PR but I wanted to hear your opinion on it: shouldn't we audit all table column references that are interpolated into strings like this and replace them with a centralized function that can quote all identifiers uniformly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could indeed change all places were we quote explicitly by calling a quote function that does the quoting. That improves maintainability if at some later point in time we want to change the way we quote identifiers but i don't think we will ever change the quoting as it seems the only way to quote identifiers is to use double quotes.

} else {
return targettedCol
Expand Down
2 changes: 1 addition & 1 deletion clients/typescript/src/satellite/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1130,6 +1130,7 @@ function deserializeColumnData(
switch (columnType) {
case PgBasicType.PG_CHAR:
case PgDateType.PG_DATE:
case PgBasicType.PG_INT8:
case PgBasicType.PG_TEXT:
case PgDateType.PG_TIME:
case PgDateType.PG_TIMESTAMP:
Expand All @@ -1142,7 +1143,6 @@ function deserializeColumnData(
case PgBasicType.PG_INT:
case PgBasicType.PG_INT2:
case PgBasicType.PG_INT4:
case PgBasicType.PG_INT8:
case PgBasicType.PG_INTEGER:
return Number(typeDecoder.text(column))
case PgBasicType.PG_FLOAT4:
Expand Down
3 changes: 3 additions & 0 deletions clients/typescript/src/satellite/oplog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,9 @@ function deserialiseRow(str: string, rel: Pick<Relation, 'columns'>): Rec {
return Number(value)
}
}
if (columnType === 'INT8' || columnType === 'BIGINT') {
return BigInt(value)
}
return value
})
}
Expand Down
8 changes: 7 additions & 1 deletion clients/typescript/src/satellite/process.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1471,7 +1471,13 @@ export function generateTriggersForTable(tbl: MigrationTable): Statement[] {
}
}),
columnTypes: Object.fromEntries(
tbl.columns.map((col) => [col.name, col.sqliteType.toUpperCase()])
tbl.columns.map((col) => [
col.name,
{
sqliteType: col.sqliteType.toUpperCase(),
pgType: col.pgType!.name.toUpperCase(),
},
])
),
}
const fullTableName = table.namespace + '.' + table.tableName
Expand Down
8 changes: 6 additions & 2 deletions clients/typescript/src/util/types.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type Long from 'long'
import {
SatOpMigrate_Column,
SatOpMigrate_PgColumnType,
SatOpMigrate_Table,
SatOpMigrate_Type,
SatRelation_RelationType,
Expand Down Expand Up @@ -130,9 +131,12 @@ export type DataChange = {
tags: Tag[]
}

// The properties are omitted from columns because they are not currently used.
export type SatOpMigrate_Col = Omit<SatOpMigrate_Column, '$type' | 'pgType'> & {
pgType: Omit<SatOpMigrate_PgColumnType, '$type'> | undefined
}

export type MigrationTable = Omit<SatOpMigrate_Table, '$type' | 'columns'> & {
columns: Omit<SatOpMigrate_Column, '$type' | 'pgType'>[]
columns: SatOpMigrate_Col[]
}

export type SchemaChange = {
Expand Down
3 changes: 2 additions & 1 deletion clients/typescript/test/client/conversions/input.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ await tbl.sync()
function setupDB() {
db.exec('DROP TABLE IF EXISTS DataTypes')
db.exec(
"CREATE TABLE DataTypes('id' int PRIMARY KEY, 'date' varchar, 'time' varchar, 'timetz' varchar, 'timestamp' varchar, 'timestamptz' varchar, 'bool' int, 'uuid' varchar, 'int2' int2, 'int4' int4, 'float8' real, 'relatedId' int);"
"CREATE TABLE DataTypes('id' int PRIMARY KEY, 'date' varchar, 'time' varchar, 'timetz' varchar, 'timestamp' varchar, 'timestamptz' varchar, 'bool' int, 'uuid' varchar, 'int2' int2, 'int4' int4, 'int8' int8, 'float8' real, 'relatedId' int);"
)

db.exec('DROP TABLE IF EXISTS Dummy')
Expand Down Expand Up @@ -232,6 +232,7 @@ const dateNulls = {
bool: null,
int2: null,
int4: null,
int8: null,
float8: null,
uuid: null,
}
Expand Down
23 changes: 22 additions & 1 deletion clients/typescript/test/client/conversions/sqlite.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ await tbl.sync()
function setupDB() {
db.exec('DROP TABLE IF EXISTS DataTypes')
db.exec(
"CREATE TABLE DataTypes('id' int PRIMARY KEY, 'date' varchar, 'time' varchar, 'timetz' varchar, 'timestamp' varchar, 'timestamptz' varchar, 'bool' int, 'uuid' varchar, 'int2' int2, 'int4' int4, 'float8' real, 'relatedId' int);"
"CREATE TABLE DataTypes('id' int PRIMARY KEY, 'date' varchar, 'time' varchar, 'timetz' varchar, 'timestamp' varchar, 'timestamptz' varchar, 'bool' int, 'uuid' varchar, 'int2' int2, 'int4' int4, 'int8' int8, 'float8' real, 'relatedId' int);"
)
}

Expand Down Expand Up @@ -211,3 +211,24 @@ test.serial('floats are converted correctly to SQLite', async (t) => {
{ id: 4, float8: -Infinity },
])
})

test.serial('BigInts are converted correctly to SQLite', async (t) => {
//db.defaultSafeIntegers(true) // enables BigInt support
const bigInt = 9_223_372_036_854_775_807n
await tbl.create({
data: {
id: 1,
int8: bigInt,
},
})

const rawRes = await electric.db.raw({
sql: 'SELECT id, cast(int8 as TEXT) AS int8 FROM DataTypes WHERE id = ?',
args: [1],
})
// because we are executing a raw query,
// the returned BigInt for the `id`
// is not converted into a regular number
t.deepEqual(rawRes, [{ id: 1, int8: bigInt.toString() }])
//db.defaultSafeIntegers(false) // disables BigInt support
})
Loading
Loading