diff --git a/apple/Packages/TableProCore/Sources/TableProMSSQLCore/MSSQLRowMatch.swift b/apple/Packages/TableProCore/Sources/TableProMSSQLCore/MSSQLRowMatch.swift new file mode 100644 index 0000000000..7ffccbc0ec --- /dev/null +++ b/apple/Packages/TableProCore/Sources/TableProMSSQLCore/MSSQLRowMatch.swift @@ -0,0 +1,21 @@ +import Foundation + +// Text is sent as nvarchar, and a char or varchar column read against it compares by Unicode rules even under a SQL +// collation, where `ß` equals `ss`. The column's own rules keep `Weßels` and `Wessels` apart, so a unique key can hold +// both and `=` alone matched both rows. Comparing the stored text by code point keeps the one row the value was read +// from, with trailing spaces padded as `=` pads them (measured on SQL Server 2022). Every character type gets it, since +// a result can read a varchar column through a cast to nvarchar. +public enum MSSQLRowMatch { + private static let characterTypes: Set = ["char", "varchar", "text", "nchar", "nvarchar", "ntext", "sysname"] + + public static func comparesText(typeName: String?) -> Bool { + guard let typeName else { return false } + let base = typeName.lowercased().prefix { $0 != "(" }.trimmingCharacters(in: .whitespaces) + return characterTypes.contains(base) + } + + // `value` is the placeholder or literal the row match already compares the column with. + public static func exactText(_ quotedColumn: String, equals value: String) -> String { + "CAST(\(quotedColumn) AS NVARCHAR(MAX)) COLLATE Latin1_General_100_BIN2 = \(value)" + } +} diff --git a/apple/Packages/TableProCore/Sources/TableProQuery/FilterSQLGenerator.swift b/apple/Packages/TableProCore/Sources/TableProQuery/FilterSQLGenerator.swift index c767d3b583..d1add99dd7 100644 --- a/apple/Packages/TableProCore/Sources/TableProQuery/FilterSQLGenerator.swift +++ b/apple/Packages/TableProCore/Sources/TableProQuery/FilterSQLGenerator.swift @@ -127,6 +127,11 @@ public struct FilterSQLGenerator: Sendable { "\(stringLiteralPrefix)'\(literalBody(value))'" } + // By scalar: a combining mark after the quote joins it into one Character, so `hasPrefix` misses it. + private func isStringLiteral(_ sql: String) -> Bool { + sql.unicodeScalars.starts(with: "\(stringLiteralPrefix)'".unicodeScalars) + } + // A number, a boolean or a bit has no case, and compared folded as text `5.50` would no longer equal `5.5`. // A pattern still folds: `contains TRUE` finds PostgreSQL's `true`. private func caseFolding(for filter: TableFilter, column: ColumnInfo?) -> PluginSQLCaseFolding { @@ -146,7 +151,7 @@ public struct FilterSQLGenerator: Sendable { private func comparisonCondition( _ column: String, _ operand: String, _ operatorText: String, _ literal: String, folding: PluginSQLCaseFolding ) -> String { - guard folding.foldsComparisonOperands, literal.hasPrefix("'") || literal.hasPrefix("N'") else { + guard folding.foldsComparisonOperands, isStringLiteral(literal) else { return "\(column) \(operatorText) \(literal)" } return "\(folding.fold(operand)) \(operatorText) \(folding.fold(literal))" @@ -201,7 +206,7 @@ public struct FilterSQLGenerator: Sendable { ) -> String { let keyword = negated ? "NOT IN" : "IN" let items = listItems(rawValue).map { escapeValue($0, kind: kind) } - let foldable = folding.foldsComparisonOperands && items.allSatisfy { $0.hasPrefix("'") || $0.hasPrefix("N'") } + let foldable = folding.foldsComparisonOperands && items.allSatisfy(isStringLiteral) let rendered = foldable ? items.map { folding.fold($0) } : items let subject = foldable ? folding.fold(operand) : column return "\(subject) \(keyword) (\(rendered.joined(separator: ", ")))" diff --git a/apple/Packages/TableProCore/Tests/TableProMSSQLCoreTests/MSSQLRowMatchTests.swift b/apple/Packages/TableProCore/Tests/TableProMSSQLCoreTests/MSSQLRowMatchTests.swift new file mode 100644 index 0000000000..5b8a2fc285 --- /dev/null +++ b/apple/Packages/TableProCore/Tests/TableProMSSQLCoreTests/MSSQLRowMatchTests.swift @@ -0,0 +1,34 @@ +import Foundation +@testable import TableProMSSQLCore +import Testing + +struct MSSQLRowMatchTests { + @Test("Character columns need the exact comparison", arguments: [ + "char", "varchar", "text", "VARCHAR", "varchar(20)", "char(4)", "varchar(max)", " varchar (10)", + "nchar", "nvarchar(50)", "ntext", "sysname" + ]) + func characterTypes(typeName: String) { + #expect(MSSQLRowMatch.comparesText(typeName: typeName)) + } + + // The value of a number, a date or a binary column is not the text a cast to nvarchar prints. + @Test("Other columns keep the plain comparison", arguments: [ + "varbinary(max)", "int", "decimal(10,2)", "datetime", "uniqueidentifier", "xml", "sql_variant", "", "textual" + ]) + func otherTypes(typeName: String) { + #expect(!MSSQLRowMatch.comparesText(typeName: typeName)) + } + + @Test("A column with no known type keeps the plain comparison") + func unknownType() { + #expect(!MSSQLRowMatch.comparesText(typeName: nil)) + } + + @Test("The exact comparison reads the stored text by code point") + func exactText() { + #expect(MSSQLRowMatch.exactText("[k]", equals: "?") + == "CAST([k] AS NVARCHAR(MAX)) COLLATE Latin1_General_100_BIN2 = ?") + #expect(MSSQLRowMatch.exactText("[k]", equals: "N'Weßels'") + == "CAST([k] AS NVARCHAR(MAX)) COLLATE Latin1_General_100_BIN2 = N'Weßels'") + } +} diff --git a/apple/Packages/TableProCore/Tests/TableProQueryTests/FilterSQLGeneratorTests.swift b/apple/Packages/TableProCore/Tests/TableProQueryTests/FilterSQLGeneratorTests.swift index 02618110fb..581fe6a8ca 100644 --- a/apple/Packages/TableProCore/Tests/TableProQueryTests/FilterSQLGeneratorTests.swift +++ b/apple/Packages/TableProCore/Tests/TableProQueryTests/FilterSQLGeneratorTests.swift @@ -522,6 +522,17 @@ struct FilterSQLGeneratorTests { == "WHERE CAST(\"f\" AS TEXT) ILIKE '%TRUE%' ESCAPE '!'") } + @Test("Match Case off still folds a value that opens with a combining mark") + func leadingCombiningMarkIsFolded() { + let column = ColumnInfo(name: "c", typeName: "text") + #expect(engineClause(.postgresql, column, "\u{0301}ABC", isCaseSensitive: false, dialect: castingDialect) + == "WHERE LOWER(\"c\") = LOWER('\u{0301}ABC')") + #expect(engineClause(.postgresql, column, "\u{0301}A, B", op: .in, isCaseSensitive: false, dialect: castingDialect) + == "WHERE LOWER(\"c\") IN (LOWER('\u{0301}A'), LOWER('B'))") + #expect(engineClause(.mssql, column, "\u{0301}ABC", isCaseSensitive: false, dialect: castingDialect) + .hasSuffix("= LOWER(N'\u{0301}ABC')")) + } + @Test("A character column keeps its own operand", arguments: ["character varying", "text", "citext", "bpchar", "name"]) func characterColumnIsNotCast(typeName: String) { let column = ColumnInfo(name: "c", typeName: typeName) @@ -557,6 +568,7 @@ struct FilterSQLGeneratorTests { #expect(FilterColumnKind.keyLiteral("05", column: nil, databaseType: .sqlite) == .text("05")) #expect(FilterColumnKind.keyLiteral("2.5", column: nil, databaseType: .sqlite) == .numberOrText("2.5")) } + @Test("SQL Server filters keep Unicode and backslash line endings") func sqlServerTextConstants() { let generator = FilterSQLGenerator(dialect: dialect, databaseType: .mssql) diff --git a/apple/Plugins/MSSQLDriverPlugin/MSSQLStatementGenerator.swift b/apple/Plugins/MSSQLDriverPlugin/MSSQLStatementGenerator.swift index 0da32fa699..a195f9ef45 100644 --- a/apple/Plugins/MSSQLDriverPlugin/MSSQLStatementGenerator.swift +++ b/apple/Plugins/MSSQLDriverPlugin/MSSQLStatementGenerator.swift @@ -169,12 +169,21 @@ struct MSSQLStatementGenerator { } else { parameters.append(value) conditions.append("\(compared) = ?") + if value.asBytes == nil, MSSQLRowMatch.comparesText(typeName: typeName(of: column)) { + parameters.append(value) + conditions.append(MSSQLRowMatch.exactText(Self.quote(column), equals: "?")) + } } } guard !conditions.isEmpty else { return nil } return (sql: conditions.joined(separator: " AND "), parameters: parameters) } + private func typeName(of column: String) -> String? { + context.columnTypeNames[column] + ?? context.columnTypeNames.first { $0.key.caseInsensitiveCompare(column) == .orderedSame }?.value + } + /// SQL Server refuses `=` on `ntext`, `text`, `image` and `xml` (Msg 402), `sql_variant` (206) and the spatial /// types (403). Cast to the type the value is bound as, each compares exactly (measured on SQL Server 2022). private func keylessExpression(for column: String, value: PluginCellValue) -> String { diff --git a/apple/TableProMobile/TableProMobile/Helpers/SQLBuilder.swift b/apple/TableProMobile/TableProMobile/Helpers/SQLBuilder.swift index e995ceb980..e9f7dca291 100644 --- a/apple/TableProMobile/TableProMobile/Helpers/SQLBuilder.swift +++ b/apple/TableProMobile/TableProMobile/Helpers/SQLBuilder.swift @@ -57,7 +57,8 @@ nonisolated enum SQLBuilder { return "SELECT * FROM \(quoted) \(pagination)" } - // Every pair of a composite key is matched: one column alone can return another tenant's row. + // Every pair of a composite key is matched: one column alone can return another tenant's row. A foreign key's + // value finds its parent by the column's collation, so `abc` finds `ABC`, and is never held to the exact text. static func buildSingleRowLookup( table: String, schema: String?, @@ -67,7 +68,8 @@ nonisolated enum SQLBuilder { driver: any DatabaseDriver ) -> String { let quotedTable = qualifiedIdentifier(table: table, schema: schema, for: type) - let predicate = key.isEmpty ? "1 = 0" : keyPredicate(key, columns: columns, type: type, driver: driver) + let predicate = key.isEmpty + ? "1 = 0" : keyPredicate(key, columns: columns, type: type, driver: driver, exactText: false) let pagination = paginationClause(orderBy: "", limit: 1, offset: 0, for: type) return "SELECT * FROM \(quotedTable) WHERE \(predicate) \(pagination)" } @@ -81,7 +83,8 @@ nonisolated enum SQLBuilder { columns: [ColumnInfo], type: DatabaseType, driver: any DatabaseDriver, - uniqueIn quotedTable: String? = nil + uniqueIn quotedTable: String? = nil, + exactText: Bool = true ) -> String { let predicate = key.map { pair in let column = quoteIdentifier(pair.column, for: type) @@ -90,7 +93,11 @@ nonisolated enum SQLBuilder { case .number(let value): return "\(column) = \(value)" case .text(let value): - return "\(column) = \(stringLiteral(value, for: type, driver: driver))" + let literal = stringLiteral(value, for: type, driver: driver) + guard exactText, type == .mssql, MSSQLRowMatch.comparesText(typeName: info?.typeName) else { + return "\(column) = \(literal)" + } + return "\(column) = \(literal) AND \(MSSQLRowMatch.exactText(column, equals: literal))" case .numberOrText(let value): // CAST is the text the grid read, so a REAL another spelling of the same number parses to stays out. return "\(column) IN (\(value), '\(value)') AND CAST(\(column) AS TEXT) = '\(value)'" diff --git a/apple/TableProMobile/TableProMobileTests/Helpers/SQLBuilderSingleRowLookupTests.swift b/apple/TableProMobile/TableProMobileTests/Helpers/SQLBuilderSingleRowLookupTests.swift index 03d3581c9a..9b0f21d34e 100644 --- a/apple/TableProMobile/TableProMobileTests/Helpers/SQLBuilderSingleRowLookupTests.swift +++ b/apple/TableProMobile/TableProMobileTests/Helpers/SQLBuilderSingleRowLookupTests.swift @@ -189,5 +189,37 @@ struct SQLBuilderSingleRowLookupTests { #expect(SQLBuilder.buildInsert(table: "t", schema: nil, type: .mssql, driver: sqlDriver, columns: ["k"], values: [value]) == "INSERT INTO [t] ([k]) VALUES (" + literal + ")") } + + // Under a SQL collation a varchar key holds both `Wessels` and `Weßels`, which compare equal against N'…' text. + @Test("A SQL Server text key also matches its exact stored text") + func sqlServerNarrowKeyMatchesExactly() { + let sqlDriver = MSSQLDriver(connection: DatabaseConnection(name: "test", type: .mssql), password: nil) + let exact = "[k] = N'Weßels' AND CAST([k] AS NVARCHAR(MAX)) COLLATE Latin1_General_100_BIN2 = N'Weßels'" + let narrow = [ColumnInfo(name: "k", typeName: "varchar(20)")] + #expect(SQLBuilder.buildDelete( + table: "t", schema: "dbo", type: .mssql, driver: sqlDriver, primaryKeys: [("k", "Weßels")], columns: narrow + ) == "DELETE FROM [dbo].[t] WHERE " + exact) + #expect(SQLBuilder.buildUpdate( + table: "t", schema: "dbo", type: .mssql, driver: sqlDriver, + changes: [("v", "x")], primaryKeys: [("k", "Weßels")], columns: narrow + ) == "UPDATE [dbo].[t] SET [v] = N'x' WHERE " + exact) + #expect(SQLBuilder.keyPredicate([("k", "Weßels")], columns: narrow, type: .mssql, driver: sqlDriver) == exact) + let unicode = [ColumnInfo(name: "k", typeName: "nvarchar(20)")] + #expect(SQLBuilder.buildDelete( + table: "t", schema: "dbo", type: .mssql, driver: sqlDriver, primaryKeys: [("k", "Weßels")], columns: unicode + ) == "DELETE FROM [dbo].[t] WHERE " + exact) + let number = [ColumnInfo(name: "k", typeName: "int")] + #expect(SQLBuilder.buildDelete( + table: "t", schema: "dbo", type: .mssql, driver: sqlDriver, primaryKeys: [("k", "5")], columns: number + ) == "DELETE FROM [dbo].[t] WHERE [k] = 5") + } + + // A foreign key's `abc` refers to a parent `ABC` under a case-insensitive collation. + @Test("A SQL Server foreign key lookup keeps the column's collation") + func sqlServerLookupKeepsCollation() { + let narrow = [ColumnInfo(name: "k", typeName: "varchar(20)")] + #expect(lookup([("k", "abc")], columns: narrow, type: .mssql) + == "SELECT * FROM [parent] WHERE [k] = N'abc' ORDER BY (SELECT NULL) OFFSET 0 ROWS FETCH NEXT 1 ROWS ONLY") + } } diff --git a/apple/changelog.d/4039.fixed.md b/apple/changelog.d/4039.fixed.md new file mode 100644 index 0000000000..5b0e17b864 --- /dev/null +++ b/apple/changelog.d/4039.fixed.md @@ -0,0 +1 @@ +SQL Server edits on a char or varchar key could also change a row whose key differs only by characters like ß and ss. diff --git a/apple/scripts/probes/check-mssql-string-literals.sh b/apple/scripts/probes/check-mssql-string-literals.sh index 198c2c0466..4a3031f03b 100755 --- a/apple/scripts/probes/check-mssql-string-literals.sh +++ b/apple/scripts/probes/check-mssql-string-literals.sh @@ -101,6 +101,35 @@ struct Check { } print("PASS: \(newline.debugDescription) values, row targeting, nested names and LIKE") } + // Under a SQL collation a varchar key holds both spellings, which compare equal against nvarchar text. A query + // that reads the key through a cast to nvarchar reports that type instead. + for (keyed, reported) in [(true, "varchar"), (false, "varchar"), (true, "nvarchar")] { + _ = try await driver.execute(query: "CREATE TABLE #narrow (k varchar(20) COLLATE SQL_Latin1_General_CP1_CI_AS" + + (keyed ? " PRIMARY KEY" : "") + ", v int); INSERT #narrow VALUES (N'Wessels', 0), (N'Weßels', 0)") + var context = PluginRowWriteContext() + context.columnTypeNames = ["k": reported, "v": "int"] + let row: [PluginCellValue] = [.text("Weßels"), .text("0")] + let changes = [ + PluginRowChange(rowIndex: 0, type: .update, cellChanges: [ + (columnIndex: 1, columnName: "v", oldValue: .text("0"), newValue: .text("1")) + ], originalRow: row), + PluginRowChange(rowIndex: 0, type: .delete, cellChanges: [], originalRow: [.text("Weßels"), .text("1")]), + ] + for change in changes { + let writes = try driver.generateRowWrites( + table: "#narrow", schema: nil, columns: ["k", "v"], primaryKeyColumns: keyed ? ["k"] : [], + changes: [change], insertedRowData: [:], deletedRowIndices: [0], insertedRowIndices: [], + context: context + ) ?? [] + for write in writes { + _ = try await driver.executeParameterized(query: write.statement, parameters: write.parameters) + } + try await expect("SELECT CONVERT(varchar(10), v) FROM #narrow WHERE k = 'Wessels'", "0") + } + try await expect("SELECT CONVERT(varchar(10), COUNT(*)) FROM #narrow", "1") + _ = try await driver.execute(query: "DROP TABLE #narrow") + } + print("PASS: varchar keys edit only the row with the exact stored text") print("PASS: \(checks) SQL Server checks through the shipped driver") } } diff --git a/core/crates/core/src/sql_syntax/history_recording.rs b/core/crates/core/src/sql_syntax/history_recording.rs index 7416967a16..a40516b942 100644 --- a/core/crates/core/src/sql_syntax/history_recording.rs +++ b/core/crates/core/src/sql_syntax/history_recording.rs @@ -123,8 +123,9 @@ fn runs_text_as_sql(grammar: SqlGrammar, statements: &[Vec>]) names_runner || statements.iter().any(|tokens| names_a_variable(grammar, tokens)) } -/// A T-SQL `@s` word, or MySQL's `@` before a name or a quoted name. A -/// `@@` system variable holds no text the server runs later. +/// A T-SQL `@s` word, or any MySQL `@` but the pair that opens a system +/// variable, which holds no text the server runs later. MySQL names a +/// user variable with a word, a number, a quoted name or a string. fn names_a_variable(grammar: SqlGrammar, tokens: &[StatementToken<'_>]) -> bool { tokens.iter().enumerate().any(|(at, token)| match (grammar, token) { (SqlGrammar::MsSql, StatementToken::Word(word)) => word.starts_with('@') && !word.starts_with("@@"), @@ -134,11 +135,7 @@ fn names_a_variable(grammar: SqlGrammar, tokens: &[StatementToken<'_>]) -> bool .and_then(|index| tokens.get(index)) .is_some_and(|token| token.is_punct('@')) }; - !system(at.checked_sub(1)) - && !system(Some(at + 1)) - && tokens - .get(at + 1) - .is_some_and(|next| next.name().is_some() || matches!(next, StatementToken::StringLiteral(_))) + !system(at.checked_sub(1)) && !system(Some(at + 1)) } _ => false, }) @@ -619,6 +616,18 @@ mod tests { SqlGrammar::MySql, "SELECT 'CREATE USER app IDENTIFIED BY ''Hunter2!''' INTO @s", ), + ( + SqlGrammar::MySql, + "SET @1 = 'CREATE USER app IDENTIFIED BY ''Hunter2!'''", + ), + ( + SqlGrammar::MySql, + "SET @.s = 'CREATE USER app IDENTIFIED BY ''Hunter2!'''", + ), + ( + SqlGrammar::MySql, + "SET @$s = 'CREATE USER app IDENTIFIED BY ''Hunter2!'''", + ), ( SqlGrammar::MsSql, "DECLARE @s nvarchar(max) = N'CREATE USER app WITH PASSWORD = ''Hunter2!'''", diff --git a/core/crates/core/src/sql_syntax/statement_access.rs b/core/crates/core/src/sql_syntax/statement_access.rs index 9e196a0603..cd8b32a664 100644 --- a/core/crates/core/src/sql_syntax/statement_access.rs +++ b/core/crates/core/src/sql_syntax/statement_access.rs @@ -386,23 +386,78 @@ fn writes(part: &[Code<'_>], grammar: SqlGrammar) -> bool { // `NEXT VALUE FOR` takes a number from a sequence. || (is(token.text, &["VALUE"]) && follows(index, &["NEXT"])) || calls_mutator(part, index, grammar) - || names_pragma_optimize(token, grammar)) + || names_pragma_optimize(part, index, grammar)) } - Kind::Quoted => calls_mutator(part, index, grammar) || names_pragma_optimize(token, grammar), - Kind::Literal => names_pragma_optimize(token, grammar), + Kind::Quoted => calls_mutator(part, index, grammar) || names_pragma_optimize(part, index, grammar), + Kind::Literal => names_pragma_optimize(part, index, grammar), _ => false, }) } /// SQLite runs ANALYZE wherever `pragma_optimize` names a table, called /// or not, and takes a quoted name or a string for a table name there. -fn names_pragma_optimize(token: &Code<'_>, grammar: SqlGrammar) -> bool { - let text = token.text; - let name = match token.kind { +/// Where an expression stands it is a value, a column or an alias. +fn names_pragma_optimize(part: &[Code<'_>], index: usize, grammar: SqlGrammar) -> bool { + let text = part[index].text; + let name = match part[index].kind { Kind::Literal => text.get(1..text.len().saturating_sub(1)).unwrap_or_default(), _ => text, }; - grammar == SqlGrammar::Sqlite && name.eq_ignore_ascii_case("pragma_optimize") + grammar == SqlGrammar::Sqlite && name.eq_ignore_ascii_case("pragma_optimize") && !in_expression(part, index) +} + +/// Words after which SQLite reads an expression or an alias, never a +/// table. FROM, JOIN, IN and a schema's `.` lead to a table instead. +const EXPRESSION_LEADS: [&str; 26] = [ + "SELECT", + "DISTINCT", + "ALL", + "WHERE", + "AND", + "OR", + "NOT", + "ON", + "HAVING", + "CASE", + "WHEN", + "THEN", + "ELSE", + "LIKE", + "GLOB", + "REGEXP", + "MATCH", + "IS", + "BETWEEN", + "ESCAPE", + "BY", + "AS", + "RETURNING", + "SET", + "LIMIT", + "OFFSET", +]; + +/// Whether `part[index]` stands where an expression does: after an +/// operator, a word in `EXPRESSION_LEADS`, or the `(` of a call or of a +/// list. A `,` or a `(` that may belong to a FROM list counts as a table. +fn in_expression(part: &[Code<'_>], index: usize) -> bool { + let Some(before) = index.checked_sub(1).and_then(|at| part.get(at)) else { + return false; + }; + match before.kind { + Kind::Equals => true, + Kind::Other => before.text != ".", + Kind::Word => is(before.text, &EXPRESSION_LEADS), + Kind::Open => index + .checked_sub(2) + .and_then(|at| part.get(at)) + .is_some_and(|opener| match opener.kind { + Kind::Word => !opener.is_word(&["FROM", "JOIN"]), + Kind::Comma | Kind::Open => false, + _ => true, + }), + _ => false, + } } /// A call of a built-in that changes state, its name quoted or not. @@ -1670,4 +1725,30 @@ mod tests { StatementAccess::Read ); } + + #[test] + fn a_pragma_name_as_a_value_column_or_alias_reads_on_sqlite() { + for sql in [ + "SELECT 'pragma_optimize'", + "SELECT name FROM sqlite_schema WHERE name = 'pragma_optimize'", + "SELECT name FROM sqlite_schema WHERE name IN ('pragma_optimize', 'x')", + "SELECT name FROM sqlite_schema WHERE name LIKE 'pragma_optimize'", + "SELECT pragma_optimize FROM settings", + "SELECT 1 AS pragma_optimize", + "SELECT lower('pragma_optimize')", + "SELECT 'a' || 'pragma_optimize'", + ] { + assert_eq!(access(sql, SqlGrammar::Sqlite), StatementAccess::Read, "{sql}"); + } + for sql in [ + "SELECT * FROM (pragma_optimize)", + "SELECT * FROM orders JOIN 'pragma_optimize'", + "SELECT * FROM orders AS o, pragma_optimize", + "SELECT 1 WHERE 1 IN pragma_optimize", + "SELECT * FROM main.'pragma_optimize'", + "SELECT * FROM orders WHERE id IN (SELECT arg FROM pragma_optimize)", + ] { + assert_eq!(access(sql, SqlGrammar::Sqlite), StatementAccess::Write, "{sql}"); + } + } } diff --git a/core/crates/drivers/clickhouse/src/client_factory.rs b/core/crates/drivers/clickhouse/src/client_factory.rs index aa1f66eff1..591f45b6fa 100644 --- a/core/crates/drivers/clickhouse/src/client_factory.rs +++ b/core/crates/drivers/clickhouse/src/client_factory.rs @@ -104,8 +104,8 @@ pub(crate) fn base_client( clickhouse::Client::with_http_client(builder.build(https)) } }; - // A typed fetch then reads RowBinary, which names no column types, so the login never parses a type name - // the server sent, however deep it nests, nor sizes a list by the column count the server claims. + // A typed fetch then reads RowBinary, which names no column types, so no type name the server sends is parsed, + // however deep it nests, nor a list sized by the column count the server claims. let mut client = client .with_url(service_url(scheme, target)) .with_validation(false) diff --git a/core/crates/drivers/clickhouse/src/endpoint.rs b/core/crates/drivers/clickhouse/src/endpoint.rs index 85ad5105f7..2c6a60e263 100644 --- a/core/crates/drivers/clickhouse/src/endpoint.rs +++ b/core/crates/drivers/clickhouse/src/endpoint.rs @@ -13,6 +13,14 @@ use crate::result_reader::drain; /// How long the server keeps an editor session's state between runs. const SESSION_TIMEOUT: Duration = Duration::from_secs(3600); +// The level goes out as text, which every JSON output setting writes as a string; a login at a readonly level of +// its own cannot change those settings for this query. +const IDENTITY_SQL: &str = "SELECT version(), currentDatabase(), toString(toUInt8(getSetting('readonly')))"; + +/// The login's answer is two short strings and a byte. A longer body is +/// not a ClickHouse server's, and is never held in memory whole. +const IDENTITY_LIMIT: usize = 64 << 10; + /// Everything needed to reach one server: the workspace's client, the /// sessionless control client that KILLs and resolves, and the /// transport behind both. @@ -125,14 +133,8 @@ impl ClickHouseEndpoint { /// refused password or a failed handshake shows up. pub(crate) async fn identify(&mut self) -> Result<(ServerInfo, String), DriverError> { let bound = self.liveness.connect_timeout + self.liveness.login_timeout; - let asking = self - .control - .query_raw( - "SELECT version() AS version, currentDatabase() AS database, \ - toUInt8(getSetting('readonly')) AS readonly", - ) - .fetch_one::(); - let identity = match tokio::time::timeout(bound, asking).await { + let asking = read_identity(&self.control); + let body = match tokio::time::timeout(bound, asking).await { Err(_) => { return Err(DriverError::Timeout { phase: TimeoutPhase::Login, @@ -145,14 +147,12 @@ impl ClickHouseEndpoint { }; return Err(self.explain(map_error_in(&error, context)).await); } - Ok(Ok(identity)) => identity, + Ok(Ok(body)) => body, }; - self.hold(identity.readonly); - let (major, minor, patch) = version_numbers(&identity.version); - Ok(( - ServerInfo::new("ClickHouse", identity.version, major, minor, patch), - identity.database, - )) + let (version, database, readonly) = identity_of(body)?; + self.hold(readonly); + let (major, minor, patch) = version_numbers(&version); + Ok((ServerInfo::new("ClickHouse", version, major, minor, patch), database)) } /// A stream that closed may be the transport's doing, such as an SSH @@ -181,12 +181,37 @@ impl ClickHouseEndpoint { } } -// Read as RowBinary, which carries no names, so the fields keep the order the login query selects them in. -#[derive(Debug, serde::Deserialize, clickhouse::Row)] -struct Identity { - version: String, - database: String, - readonly: u8, +/// The login answer's body, or None once it runs past `IDENTITY_LIMIT`. +/// A RowBinary read would buffer a string of whatever length the server +/// declares before it could refuse it. +async fn read_identity(control: &clickhouse::Client) -> Result>, clickhouse::error::Error> { + let mut cursor = control.query_raw(IDENTITY_SQL).fetch_bytes("JSONCompactEachRow")?; + let mut body = Vec::new(); + while let Some(chunk) = cursor.next().await? { + if body.len() + chunk.len() > IDENTITY_LIMIT { + return Ok(None); + } + body.extend_from_slice(&chunk); + } + Ok(Some(body)) +} + +/// The version, the current database and the login's `readonly` level. +fn identity_of(body: Option>) -> Result<(String, String, u8), DriverError> { + let body = body.ok_or_else(|| { + DriverError::Protocol(format!( + "the server's answer to the login ran past {} KiB", + IDENTITY_LIMIT >> 10 + )) + })?; + let unreadable = + |detail: String| DriverError::Protocol(format!("the server's answer to the login could not be read: {detail}")); + let (version, database, readonly): (String, String, String) = + serde_json::from_slice(&body).map_err(|error| unreadable(error.to_string()))?; + let readonly = readonly + .parse() + .map_err(|_| unreadable(format!("readonly level {readonly:?}")))?; + Ok((version, database, readonly)) } /// `24.8.4.13` is major 24, minor 8. @@ -228,7 +253,7 @@ mod tests { /// The login against a loopback server whose every answer is `body`, /// run where a runtime worker would run it, on a 2 MiB stack. - fn identify_against(body: Vec) -> Result<(), DriverError> { + fn identify_against(body: Vec) -> Result<(ServerInfo, String, u8), DriverError> { let worker = std::thread::Builder::new().stack_size(2 << 20); let handle = worker .spawn(move || { @@ -255,7 +280,10 @@ mod tests { let mut endpoint = ClickHouseEndpoint::new(&target, liveness, &ConnectEnvironment::new("test"), false) .expect("an endpoint"); - let identity = endpoint.identify().await.map(|_| ()); + let identity = endpoint + .identify() + .await + .map(|(info, database)| (info, database, endpoint.login_readonly)); answering.abort(); identity }) @@ -296,6 +324,29 @@ mod tests { assert!(outcome.is_err(), "{outcome:?}"); } + #[test] + fn a_login_answer_reads_as_the_version_the_database_and_the_readonly_level() { + let (info, database, readonly) = + identify_against(b"[\"24.8.4.13\",\"tablepro\",\"2\"]\n".to_vec()).expect("a login"); + + assert_eq!((info.major, info.minor, info.patch), (24, 8, 4)); + assert_eq!(database, "tablepro"); + assert_eq!(readonly, 2); + } + + #[test] + fn a_login_answer_past_64_kib_fails_the_login() { + let body = format!("[\"{}\",\"default\",0]\n", "9".repeat(1 << 20)); + + let outcome = identify_against(body.into_bytes()); + + assert!( + matches!(&outcome, Err(DriverError::Protocol(message)) if message.contains("64 KiB")), + "{:?}", + outcome.map(|(info, _, _)| info.version.len()) + ); + } + #[test] fn a_login_answer_counting_2_to_the_62_columns_fails_the_login() { let mut body = vec![0x80; 8]; diff --git a/core/crates/drivers/postgres/src/catalog/pg_column_row.rs b/core/crates/drivers/postgres/src/catalog/pg_column_row.rs index 7918695b7d..026dcda976 100644 --- a/core/crates/drivers/postgres/src/catalog/pg_column_row.rs +++ b/core/crates/drivers/postgres/src/catalog/pg_column_row.rs @@ -13,6 +13,9 @@ use crate::codec::TextArray; /// /// The argument is the compression projection, since /// `attcompression` arrived in PostgreSQL 14. +/// +/// Under the catalog path `format_type` drops a built-in's schema, but a +/// cast target runs under the session's path, where a lookalike can win. macro_rules! columns_sql { ($compression:literal) => { concat!( @@ -20,7 +23,7 @@ macro_rules! columns_sql { WITH RECURSIVE resolved AS ( SELECT a.attnum, a.atttypid AS type_oid, a.atttypmod AS typmod, 0 AS depth FROM pg_catalog.pg_attribute a - WHERE a.attrelid = $1::text::regclass::oid AND a.attnum > 0 AND NOT a.attisdropped + WHERE a.attrelid = $1::text::oid AND a.attnum > 0 AND NOT a.attisdropped UNION ALL SELECT r.attnum, t.typbasetype, CASE WHEN r.typmod = -1 THEN t.typtypmod ELSE r.typmod END, @@ -36,7 +39,9 @@ base AS ( ) SELECT a.attname::text, pg_catalog.format_type(a.atttypid, a.atttypmod)::text, - pg_catalog.format_type(a.atttypid, NULL)::text, + CASE WHEN atn.nspname = 'pg_catalog' + THEN 'pg_catalog.' || pg_catalog.quote_ident(at.typname::text) + ELSE pg_catalog.format_type(a.atttypid, NULL)::text END, b.type_oid::int8, b.typmod::int4, bt.typtype::text, @@ -62,13 +67,14 @@ SELECT a.attname::text, FROM pg_catalog.pg_attribute a JOIN base b ON b.attnum = a.attnum JOIN pg_catalog.pg_type at ON at.oid = a.atttypid +JOIN pg_catalog.pg_namespace atn ON atn.oid = at.typnamespace JOIN pg_catalog.pg_type bt ON bt.oid = b.type_oid LEFT JOIN pg_catalog.pg_type et ON et.oid = bt.typelem AND bt.typcategory = 'A' LEFT JOIN pg_catalog.pg_attrdef d ON d.adrelid = a.attrelid AND d.adnum = a.attnum LEFT JOIN pg_catalog.pg_constraint pk ON pk.conrelid = a.attrelid AND pk.contype = 'p' LEFT JOIN pg_catalog.pg_collation co ON co.oid = a.attcollation LEFT JOIN pg_catalog.pg_namespace cn ON cn.oid = co.collnamespace -WHERE a.attrelid = $1::text::regclass::oid AND a.attnum > 0 AND NOT a.attisdropped +WHERE a.attrelid = $1::text::oid AND a.attnum > 0 AND NOT a.attisdropped ORDER BY a.attnum" ) }; diff --git a/core/crates/drivers/postgres/src/catalog/table_definition.rs b/core/crates/drivers/postgres/src/catalog/table_definition.rs index 056bb6d06e..f57ef32e6a 100644 --- a/core/crates/drivers/postgres/src/catalog/table_definition.rs +++ b/core/crates/drivers/postgres/src/catalog/table_definition.rs @@ -13,7 +13,7 @@ SELECT c.conname::text, JOIN pg_catalog.pg_attribute a ON a.attrelid = c.conrelid AND a.attnum = k.attnum ORDER BY k.ord) FROM pg_catalog.pg_constraint c -WHERE c.conrelid = $1::text::regclass::oid AND c.contype = 'p'"; +WHERE c.conrelid = $1::text::oid AND c.contype = 'p'"; /// Foreign keys on other tables that point at this one. A partition of /// a partitioned table that points here holds a clone of its parent's @@ -35,7 +35,7 @@ SELECT c.conname::text, FROM pg_catalog.pg_constraint c JOIN pg_catalog.pg_class t ON t.oid = c.conrelid JOIN pg_catalog.pg_namespace n ON n.oid = t.relnamespace -WHERE c.confrelid = $1::text::regclass::oid AND c.contype = 'f' +WHERE c.confrelid = $1::text::oid AND c.contype = 'f' AND NOT EXISTS (SELECT 1 FROM pg_catalog.pg_constraint p WHERE p.oid = c.conparentid AND p.confrelid = c.confrelid) ORDER BY n.nspname, t.relname, c.conname"; diff --git a/core/crates/drivers/postgres/src/metadata/estimate.rs b/core/crates/drivers/postgres/src/metadata/estimate.rs index abb8ab3894..216a0a5c11 100644 --- a/core/crates/drivers/postgres/src/metadata/estimate.rs +++ b/core/crates/drivers/postgres/src/metadata/estimate.rs @@ -11,13 +11,13 @@ use super::{decode_error, text_rows}; /// window. pub(super) const ESTIMATE_SQL: &str = "\ WITH RECURSIVE tree AS ( - SELECT $1::text::regclass::oid AS oid + SELECT $1::text::oid AS oid UNION SELECT i.inhrelid FROM pg_catalog.pg_inherits i JOIN tree t ON i.inhparent = t.oid ) SELECT COALESCE(pg_catalog.sum(GREATEST(c.reltuples, 0)), 0)::float8, COALESCE(pg_catalog.bool_and(c.reltuples < 0::float4), true), - (pg_catalog.pg_relation_size($1::text::regclass) + (pg_catalog.pg_relation_size($1::text::oid) / pg_catalog.current_setting('block_size')::int8)::int8 FROM tree JOIN pg_catalog.pg_class c ON c.oid = tree.oid"; diff --git a/core/crates/drivers/postgres/src/metadata/foreign_keys.rs b/core/crates/drivers/postgres/src/metadata/foreign_keys.rs index 6365270e75..5c666ab153 100644 --- a/core/crates/drivers/postgres/src/metadata/foreign_keys.rs +++ b/core/crates/drivers/postgres/src/metadata/foreign_keys.rs @@ -27,7 +27,7 @@ SELECT c.conname::text, FROM pg_catalog.pg_constraint c JOIN pg_catalog.pg_class t ON t.oid = c.confrelid JOIN pg_catalog.pg_namespace n ON n.oid = t.relnamespace -WHERE c.conrelid = $1::text::regclass::oid AND c.contype = 'f' +WHERE c.conrelid = $1::text::oid AND c.contype = 'f' AND NOT EXISTS (SELECT 1 FROM pg_catalog.pg_constraint p WHERE p.oid = c.conparentid AND p.conrelid = c.conrelid) ORDER BY c.conname"; diff --git a/core/crates/drivers/postgres/src/metadata/identity.rs b/core/crates/drivers/postgres/src/metadata/identity.rs index 9c8234142e..0f6721a69d 100644 --- a/core/crates/drivers/postgres/src/metadata/identity.rs +++ b/core/crates/drivers/postgres/src/metadata/identity.rs @@ -38,7 +38,7 @@ SELECT c.relkind::text, WHERE k.ord <= best.indnkeyatts ORDER BY k.ord) FROM pg_catalog.pg_class c -WHERE c.oid = $1::text::regclass::oid"; +WHERE c.oid = $1::text::oid"; pub(crate) async fn row_identity(client: &Client, relation: &str) -> Result { let field = decode_error("pg_class"); diff --git a/core/crates/drivers/postgres/src/metadata/indexes.rs b/core/crates/drivers/postgres/src/metadata/indexes.rs index 7bb7cbe618..1c46d30b9c 100644 --- a/core/crates/drivers/postgres/src/metadata/indexes.rs +++ b/core/crates/drivers/postgres/src/metadata/indexes.rs @@ -21,7 +21,7 @@ FROM pg_catalog.pg_index i JOIN pg_catalog.pg_class ic ON ic.oid = i.indexrelid CROSS JOIN LATERAL pg_catalog.unnest(i.indkey::int2[]) WITH ORDINALITY AS k(attnum, ord) LEFT JOIN pg_catalog.pg_attribute a ON a.attrelid = i.indrelid AND a.attnum = k.attnum AND k.attnum > 0 -WHERE i.indrelid = $1::text::regclass::oid AND k.ord <= i.indnkeyatts +WHERE i.indrelid = $1::text::oid AND k.ord <= i.indnkeyatts ORDER BY ic.relname, k.ord"; pub(crate) async fn fetch_indexes(client: &Client, relation: &str) -> Result, DriverError> { @@ -68,7 +68,7 @@ SELECT ic.relname::text, FROM pg_catalog.pg_index i JOIN pg_catalog.pg_class ic ON ic.oid = i.indexrelid LEFT JOIN pg_catalog.pg_tablespace ts ON ts.oid = ic.reltablespace -WHERE i.indrelid = $1::text::regclass::oid +WHERE i.indrelid = $1::text::oid AND NOT EXISTS ( SELECT 1 FROM pg_catalog.pg_constraint c WHERE c.conindid = i.indexrelid AND c.conrelid = i.indrelid AND c.contype IN ('p', 'u', 'x')) diff --git a/core/crates/drivers/postgres/tests/integration/metadata.rs b/core/crates/drivers/postgres/tests/integration/metadata.rs index 15f4a5eb42..4769651aa2 100644 --- a/core/crates/drivers/postgres/tests/integration/metadata.rs +++ b/core/crates/drivers/postgres/tests/integration/metadata.rs @@ -1332,3 +1332,56 @@ async fn an_unqualified_user_table_keeps_its_identity_when_it_shares_a_catalog_n assert_eq!(columns.len(), 2); } } + +/// With `public` searched ahead of `pg_catalog`, a lookalike in `public` +/// shares a built-in type's name. A save casts to the column's own type. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[ignore = "requires docker"] +async fn a_save_casts_to_the_built_in_type_a_lookalike_shares_a_name_with() { + let fixture = start().await; + let initial = connect(&fixture, false).await; + let inet_check = trap("inet_check", "text", "boolean"); + setup( + initial.as_ref(), + &[ + "CREATE TABLE hosts (id int PRIMARY KEY, addr pg_catalog.inet)", + "INSERT INTO hosts VALUES (1, '10.0.0.1')", + inet_check.as_str(), + "CREATE DOMAIN public.inet AS pg_catalog.text CHECK (public.inet_check(VALUE))", + "ALTER ROLE CURRENT_USER SET search_path = public, pg_catalog", + ], + ) + .await; + initial.close().await.unwrap(); + + let connection = connect(&fixture, false).await; + let target = table("hosts"); + let columns = connection.fetch_columns(&target, within(CALL)).await.unwrap(); + let addr = columns.iter().find(|column| column.name == "addr").expect("a column"); + assert_eq!( + addr.column_type + .catalog + .as_ref() + .map(|catalog| catalog.cast_target.as_sql()), + Some("pg_catalog.inet") + ); + let identity = connection.row_identity(&target, within(CALL)).await.unwrap(); + let changes = ChangeSet { + changes: vec![RowChange::Update(RowUpdate { + key: vec![Value::Int(1)], + assignments: vec![(1, CellInput::Value(Value::Text("10.0.0.2".to_owned())))], + })], + }; + let dialect = connection.dialect(); + let plan = plan_save(dialect.as_ref(), &target, &columns, &identity, &changes).expect("a plan"); + + connection + .write_batch(&plan.batch, within(CALL)) + .await + .expect("the save"); + + assert_eq!( + text(&scalar(connection.as_ref(), "SELECT pg_catalog.host(addr) FROM hosts").await), + "10.0.0.2" + ); +}