From f50d1d2e7a488804e8e18ca1d34ea9a52e55b669 Mon Sep 17 00:00:00 2001 From: Ralf Kistner Date: Wed, 25 Sep 2024 12:11:00 +0200 Subject: [PATCH 1/3] Warn on table/column identifier case issues. --- packages/sync-rules/src/SqlDataQuery.ts | 1 + packages/sync-rules/src/SqlParameterQuery.ts | 4 ++ packages/sync-rules/src/sql_filters.ts | 58 +++++++++++++++++- .../sync-rules/test/src/data_queries.test.ts | 60 +++++++++++++++++++ .../test/src/parameter_queries.test.ts | 45 +++++++++++++- 5 files changed, 166 insertions(+), 2 deletions(-) diff --git a/packages/sync-rules/src/SqlDataQuery.ts b/packages/sync-rules/src/SqlDataQuery.ts index c1176bc9b..dc5e494f2 100644 --- a/packages/sync-rules/src/SqlDataQuery.ts +++ b/packages/sync-rules/src/SqlDataQuery.ts @@ -76,6 +76,7 @@ export class SqlDataQuery { sql, schema: querySchema }); + tools.checkSpecificNameCase(tableRef); const filter = tools.compileWhereClause(where); const inputParameterNames = filter.inputParameters!.map((p) => p.key); diff --git a/packages/sync-rules/src/SqlParameterQuery.ts b/packages/sync-rules/src/SqlParameterQuery.ts index f13352177..db22937a9 100644 --- a/packages/sync-rules/src/SqlParameterQuery.ts +++ b/packages/sync-rules/src/SqlParameterQuery.ts @@ -96,6 +96,7 @@ export class SqlParameterQuery { supports_parameter_expressions: true, schema: querySchema }); + tools.checkSpecificNameCase(tableRef); const where = q.where; const filter = tools.compileWhereClause(where); @@ -118,6 +119,9 @@ export class SqlParameterQuery { for (let column of q.columns ?? []) { const name = tools.getSpecificOutputName(column); + if (column.alias != null) { + tools.checkSpecificNameCase(column.alias); + } if (tools.isTableRef(column.expr)) { rows.lookup_columns.push(column); const extractor = tools.compileRowValueExtractor(column.expr); diff --git a/packages/sync-rules/src/sql_filters.ts b/packages/sync-rules/src/sql_filters.ts index 67c4b1603..e963655f7 100644 --- a/packages/sync-rules/src/sql_filters.ts +++ b/packages/sync-rules/src/sql_filters.ts @@ -1,4 +1,4 @@ -import { Expr, ExprRef, NodeLocation, SelectedColumn } from 'pgsql-ast-parser'; +import { Expr, ExprRef, Name, NodeLocation, QName, QNameAliased, SelectedColumn, parse } from 'pgsql-ast-parser'; import { nil } from 'pgsql-ast-parser/src/utils.js'; import { ExpressionType, TYPE_NONE } from './ExpressionType.js'; import { SqlRuleError } from './errors.js'; @@ -182,6 +182,7 @@ export class SqlTools { const value = staticValue(expr); return staticValueClause(value); } else if (expr.type == 'ref') { + this.checkRefCase(expr); const column = expr.name; if (column == '*') { return this.error('* not supported here', expr); @@ -512,6 +513,61 @@ export class SqlTools { } } + public checkRefCase(ref: ExprRef) { + if (ref.table != null) { + this.checkSpecificNameCase(ref.table); + } + this.checkColumnNameCase(ref); + } + + private checkColumnNameCase(expr: ExprRef) { + if (expr.name.toLowerCase() != expr.name) { + // name is not lower case, must be quoted + return; + } + + let location = expr._location; + if (location == null) { + return; + } + const tableLocation = expr.table?._location; + if (tableLocation != null) { + // exp._location contains the entire expression. + // We use this to remove the "table" part. + location = { start: tableLocation.end + 1, end: location.end }; + } + const source = this.sql.substring(location.start, location.end); + if (source.toLowerCase() != source) { + // source is not lower case, while parsed is lower-case + this.warn(`Unquoted identifiers are converted to lower-case. Use "${source}" instead.`, location); + } + } + + /** + * Check the case of a table name or any alias. + */ + public checkSpecificNameCase(expr: Name | QName | QNameAliased) { + if ((expr as QNameAliased).alias != null || (expr as QName).schema != null) { + // We cannot properly distinguish alias and schema from the name itself, + // without building our own complete parser, so we ignore this for now. + return; + } + if (expr.name.toLowerCase() != expr.name) { + // name is not lower case, which means it is already quoted + return; + } + + const location = expr._location; + if (location == null) { + return; + } + const source = this.sql.substring(location.start, location.end); + if (source.toLowerCase() != source) { + // source is not lower case + this.warn(`Unquoted identifiers are converted to lower-case. Use "${source}" instead.`, location); + } + } + private checkRef(table: string, ref: ExprRef) { if (this.schema) { const type = this.schema.getType(table, ref.name); diff --git a/packages/sync-rules/test/src/data_queries.test.ts b/packages/sync-rules/test/src/data_queries.test.ts index 134585cd4..b83fa46ab 100644 --- a/packages/sync-rules/test/src/data_queries.test.ts +++ b/packages/sync-rules/test/src/data_queries.test.ts @@ -194,4 +194,64 @@ describe('data queries', () => { const query = SqlDataQuery.fromSql('mybucket', ['org_id'], sql); expect(query.errors[0].message).toMatch(/Parameter match expression is not allowed here/); }); + + test('case-sensitive queries (1)', () => { + const sql = 'SELECT * FROM Assets'; + const query = SqlDataQuery.fromSql('mybucket', [], sql); + expect(query.errors).toMatchObject([ + { message: `Unquoted identifiers are converted to lower-case. Use "Assets" instead.` } + ]); + }); + + test('case-sensitive queries (2)', () => { + const sql = 'SELECT *, Name FROM assets'; + const query = SqlDataQuery.fromSql('mybucket', [], sql); + expect(query.errors).toMatchObject([ + { message: `Unquoted identifiers are converted to lower-case. Use "Name" instead.` } + ]); + }); + + test('case-sensitive queries (3)', () => { + const sql = 'SELECT * FROM assets WHERE Archived = False'; + const query = SqlDataQuery.fromSql('mybucket', [], sql); + expect(query.errors).toMatchObject([ + { message: `Unquoted identifiers are converted to lower-case. Use "Archived" instead.` } + ]); + }); + + test.skip('case-sensitive queries (4)', () => { + // Cannot validate table alias yet + const sql = 'SELECT * FROM assets as myAssets'; + const query = SqlDataQuery.fromSql('mybucket', [], sql); + expect(query.errors).toMatchObject([ + { message: `Unquoted identifiers are converted to lower-case. Use "myAssets" instead.` } + ]); + }); + + test.skip('case-sensitive queries (5)', () => { + // Cannot validate table alias yet + const sql = 'SELECT * FROM assets myAssets'; + const query = SqlDataQuery.fromSql('mybucket', [], sql); + expect(query.errors).toMatchObject([ + { message: `Unquoted identifiers are converted to lower-case. Use "myAssets" instead.` } + ]); + }); + + test.skip('case-sensitive queries (6)', () => { + // Cannot validate anything with a schema yet + const sql = 'SELECT * FROM public.ASSETS'; + const query = SqlDataQuery.fromSql('mybucket', [], sql); + expect(query.errors).toMatchObject([ + { message: `Unquoted identifiers are converted to lower-case. Use "ASSETS" instead.` } + ]); + }); + + test.skip('case-sensitive queries (7)', () => { + // Cannot validate schema yet + const sql = 'SELECT * FROM PUBLIC.assets'; + const query = SqlDataQuery.fromSql('mybucket', [], sql); + expect(query.errors).toMatchObject([ + { message: `Unquoted identifiers are converted to lower-case. Use "PUBLIC" instead.` } + ]); + }); }); diff --git a/packages/sync-rules/test/src/parameter_queries.test.ts b/packages/sync-rules/test/src/parameter_queries.test.ts index 7e6a0ee34..04f24f8f0 100644 --- a/packages/sync-rules/test/src/parameter_queries.test.ts +++ b/packages/sync-rules/test/src/parameter_queries.test.ts @@ -376,7 +376,10 @@ describe('parameter queries', () => { // Postgres and/or SQLite. const sql = 'SELECT users.userId AS user_id FROM users WHERE users.userId = token_parameters.user_id'; const query = SqlParameterQuery.fromSql('mybucket', sql) as SqlParameterQuery; - expect(query.errors).toEqual([]); + expect(query.errors).toMatchObject([ + { message: `Unquoted identifiers are converted to lower-case. Use "userId" instead.` }, + { message: `Unquoted identifiers are converted to lower-case. Use "userId" instead.` } + ]); query.id = '1'; expect(query.evaluateParameterRow({ userId: 'user1' })).toEqual([]); @@ -389,6 +392,46 @@ describe('parameter queries', () => { ]); }); + test('case-sensitive parameter queries (3)', () => { + const sql = 'SELECT user_id FROM users WHERE Users.user_id = token_parameters.user_id'; + const query = SqlParameterQuery.fromSql('mybucket', sql) as SqlParameterQuery; + expect(query.errors).toMatchObject([ + { message: `Unquoted identifiers are converted to lower-case. Use "Users" instead.` } + ]); + }); + + test('case-sensitive parameter queries (4)', () => { + const sql = 'SELECT Users.user_id FROM users WHERE user_id = token_parameters.user_id'; + const query = SqlParameterQuery.fromSql('mybucket', sql) as SqlParameterQuery; + expect(query.errors).toMatchObject([ + { message: `Unquoted identifiers are converted to lower-case. Use "Users" instead.` } + ]); + }); + + test('case-sensitive parameter queries (5)', () => { + const sql = 'SELECT user_id FROM Users WHERE user_id = token_parameters.user_id'; + const query = SqlParameterQuery.fromSql('mybucket', sql) as SqlParameterQuery; + expect(query.errors).toMatchObject([ + { message: `Unquoted identifiers are converted to lower-case. Use "Users" instead.` } + ]); + }); + + test('case-sensitive parameter queries (6)', () => { + const sql = 'SELECT userId FROM users'; + const query = SqlParameterQuery.fromSql('mybucket', sql) as SqlParameterQuery; + expect(query.errors).toMatchObject([ + { message: `Unquoted identifiers are converted to lower-case. Use "userId" instead.` } + ]); + }); + + test('case-sensitive parameter queries (7)', () => { + const sql = 'SELECT user_id as userId FROM users'; + const query = SqlParameterQuery.fromSql('mybucket', sql) as SqlParameterQuery; + expect(query.errors).toMatchObject([ + { message: `Unquoted identifiers are converted to lower-case. Use "userId" instead.` } + ]); + }); + test('dynamic global parameter query', () => { const sql = "SELECT workspaces.id AS workspace_id FROM workspaces WHERE visibility = 'public'"; const query = SqlParameterQuery.fromSql('mybucket', sql) as SqlParameterQuery; From 34d30b87f933164002ba5237272e361e2fca3bdd Mon Sep 17 00:00:00 2001 From: Ralf Kistner Date: Wed, 25 Sep 2024 12:22:57 +0200 Subject: [PATCH 2/3] Add check for static parameter queries. --- packages/sync-rules/src/StaticSqlParameterQuery.ts | 3 +++ .../sync-rules/test/src/static_parameter_queries.test.ts | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/packages/sync-rules/src/StaticSqlParameterQuery.ts b/packages/sync-rules/src/StaticSqlParameterQuery.ts index 290e4e06f..841208988 100644 --- a/packages/sync-rules/src/StaticSqlParameterQuery.ts +++ b/packages/sync-rules/src/StaticSqlParameterQuery.ts @@ -39,6 +39,9 @@ export class StaticSqlParameterQuery { } for (let column of columns) { + if (column.alias != null) { + tools.checkSpecificNameCase(column.alias); + } const name = tools.getSpecificOutputName(column); const extractor = tools.compileParameterValueExtractor(column.expr); if (isClauseError(extractor)) { diff --git a/packages/sync-rules/test/src/static_parameter_queries.test.ts b/packages/sync-rules/test/src/static_parameter_queries.test.ts index d030b6f25..eb24d2014 100644 --- a/packages/sync-rules/test/src/static_parameter_queries.test.ts +++ b/packages/sync-rules/test/src/static_parameter_queries.test.ts @@ -82,6 +82,14 @@ describe('static parameter queries', () => { expect(query.getStaticBucketIds(normalizeTokenParameters({ user_id: 'user1' }))).toEqual(['mybucket["user1"]']); }); + test('case-sensitive queries (1)', () => { + const sql = 'SELECT request.user_id() as USER_ID'; + const query = SqlParameterQuery.fromSql('mybucket', sql) as SqlParameterQuery; + expect(query.errors).toMatchObject([ + { message: `Unquoted identifiers are converted to lower-case. Use "USER_ID" instead.` } + ]); + }); + describe('dangerous queries', function () { function testDangerousQuery(sql: string) { test(sql, function () { From 9e78ff18a7ab6aa2ef90f4b3f23fae319d0f478a Mon Sep 17 00:00:00 2001 From: Ralf Kistner Date: Wed, 25 Sep 2024 12:25:59 +0200 Subject: [PATCH 3/3] Add changeset. --- .changeset/short-bats-sin.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/short-bats-sin.md diff --git a/.changeset/short-bats-sin.md b/.changeset/short-bats-sin.md new file mode 100644 index 000000000..a4d78cd5a --- /dev/null +++ b/.changeset/short-bats-sin.md @@ -0,0 +1,5 @@ +--- +'@powersync/service-sync-rules': minor +--- + +Warn when identifiers are automatically convererted to lower case.