diff --git a/src/Weasel.Core.Tests/IdentifierValidationTests.cs b/src/Weasel.Core.Tests/IdentifierValidationTests.cs new file mode 100644 index 00000000..bea3c202 --- /dev/null +++ b/src/Weasel.Core.Tests/IdentifierValidationTests.cs @@ -0,0 +1,81 @@ +using Shouldly; +using Xunit; + +namespace Weasel.Core.Tests; + +public class IdentifierValidationTests +{ + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void null_empty_and_all_whitespace_are_rejected(string? name) + { + IdentifierValidation.FindProblem(name, "\"").ShouldBe("it is null, empty, or entirely whitespace"); + } + + [Theory] + [InlineData("us ers")] + [InlineData("us\ters")] + [InlineData("us\ners")] + [InlineData("us\rers")] + public void all_whitespace_characters_are_rejected_not_just_the_space(string name) + { + IdentifierValidation.FindProblem(name, "\"").ShouldBe("it contains whitespace"); + } + + /// + /// The semicolon and the single quote are unsafe for every provider, so they are checked whatever + /// the caller passes as its own unsafe set: a ';' starts a new statement, and object names reach + /// string literals on all of them via the existence checks and introspection queries. + /// + [Theory] + [InlineData("us;ers", "it contains a semicolon")] + [InlineData("us'ers", "it contains a single quote")] + public void the_universal_characters_are_rejected_without_being_asked_for(string name, string expected) + { + IdentifierValidation.FindProblem(name, "").ShouldBe(expected); + } + + [Theory] + [InlineData("us\"ers", "\"", "it contains a double quote")] + [InlineData("us`ers", "`", "it contains a backtick")] + [InlineData("us]ers", "[]", "it contains a closing square bracket")] + [InlineData("[users]", "[]", "it contains an opening square bracket")] + [InlineData("users\\", "\\", "it contains a backslash")] + [InlineData("us~ers", "~", "it contains the character '~'")] + public void the_provider_specific_characters_are_named_in_the_reason( + string name, + string unsafeCharacters, + string expected + ) + { + IdentifierValidation.FindProblem(name, unsafeCharacters).ShouldBe(expected); + } + + /// + /// A character one provider delimits with is not necessarily unsafe for another -- a backtick is + /// nothing special to PostgreSQL or SQL Server -- so the set has to stay per-provider. + /// + [Theory] + [InlineData("us`ers", "\"")] + [InlineData("us\"ers", "`")] + [InlineData("us]ers", "\"")] + public void characters_outside_the_supplied_set_pass(string name, string unsafeCharacters) + { + IdentifierValidation.FindProblem(name, unsafeCharacters).ShouldBeNull(); + } + + [Theory] + [InlineData("mt_doc_user")] + [InlineData("users$1")] + [InlineData("_leading_underscore")] + [InlineData("MixedCaseName")] + [InlineData("naïve_café")] + [InlineData("table-with-dashes")] + [InlineData("mt_doc_target.p_tenant_one")] + public void ordinary_names_pass(string name) + { + IdentifierValidation.FindProblem(name, "\"[]`").ShouldBeNull(); + } +} diff --git a/src/Weasel.Core/IdentifierValidation.cs b/src/Weasel.Core/IdentifierValidation.cs new file mode 100644 index 00000000..8480063c --- /dev/null +++ b/src/Weasel.Core/IdentifierValidation.cs @@ -0,0 +1,85 @@ +namespace Weasel.Core; + +/// +/// The identifier checks every provider needs, factored out so each provider's +/// only has to supply what is specific to it: the +/// characters its own dialect delimits with, its length limit, and the exception type it has always +/// thrown (weasel#416). +/// +/// +/// +/// is the only identifier check in the stack -- +/// and its provider-specific subclasses do no validation of their own -- +/// and the migration path runs every schema object's name through it +/// (DatabaseBase.ApplyAllConfiguredChangesToDatabaseAsync and +/// DatabaseBase.generateOrUpdateFeature). So it has to reject the characters that let a name +/// escape the statement it is written into. +/// +/// +/// Two of those are the same for everyone. A ; ends the statement and starts another. A +/// ' closes a string literal, and object names do reach string literals on every provider -- +/// the existence checks and introspection queries interpolate them (SQL Server's +/// IF OBJECT_ID('...'), Oracle's WHERE table_name = '...' inside an anonymous PL/SQL +/// block, SQLite's pragma_table_info('...')). Whitespace is rejected in full rather than just +/// the literal space character, so that a newline cannot introduce a -- comment into an +/// unquoted name. +/// +/// +/// The rest is per-provider, because the character that closes an identifier is not: SQL Server +/// delimits with [...] as well as "...", MySQL with backticks, Oracle and SQLite with +/// ". Weasel's quoting helpers do not double an embedded delimiter (SQLite's +/// SchemaUtils.QuoteName does, but only quotes at all for keywords, spaces, dashes and +/// leading digits), so a name carrying one does not stay inside its own quotes. +/// +/// +public static class IdentifierValidation +{ + /// + /// Returns why is unsafe to write into DDL, phrased to follow "because", + /// or null when it passes. Length is deliberately not checked here -- the limit and the + /// exception that reports it differ per provider. + /// + /// The identifier to check. Null, empty and all-whitespace are rejected. + /// + /// The characters that are unsafe for this provider on top of the universal ones: its identifier + /// delimiters, plus anything else that can break out of the context a name lands in -- MySQL's + /// backslash escape, for instance. + /// + public static string? FindProblem(string? name, string unsafeCharacters) + { + if (string.IsNullOrWhiteSpace(name)) + { + return "it is null, empty, or entirely whitespace"; + } + + foreach (var c in name) + { + if (char.IsWhiteSpace(c)) + { + return "it contains whitespace"; + } + + if (c is ';' or '\'' || unsafeCharacters.Contains(c)) + { + return $"it contains {Describe(c)}"; + } + } + + return null; + } + + private static string Describe(char c) + { + return c switch + { + ';' => "a semicolon", + '\'' => "a single quote", + '"' => "a double quote", + '`' => "a backtick", + '[' => "an opening square bracket", + ']' => "a closing square bracket", + '\\' => "a backslash", + _ => $"the character '{c}'" + }; + } +} diff --git a/src/Weasel.MySql.Tests/MySqlMigratorTests.cs b/src/Weasel.MySql.Tests/MySqlMigratorTests.cs index 0b592c4c..5da92451 100644 --- a/src/Weasel.MySql.Tests/MySqlMigratorTests.cs +++ b/src/Weasel.MySql.Tests/MySqlMigratorTests.cs @@ -28,6 +28,87 @@ public void create_table_returns_mysql_table() table.Identifier.ShouldBe(identifier); } + /// + /// weasel#416. AssertValidIdentifier is the only identifier check in the stack, and this provider's + /// checked length only until now, so it has to reject the characters that let a name escape the + /// statement it is written into. MySQL delimits identifiers with backticks -- and with " + /// under ANSI_QUOTES -- a ' closes a string literal, a \ escapes the character after + /// it inside one, and a ; starts a new statement. + /// + [Theory] + [InlineData("users`", "a trailing backtick")] + [InlineData("`users`", "a fully backtick-wrapped name")] + [InlineData("us`ers", "an embedded backtick")] + [InlineData("users`; drop table users; --", "a backtick-and-semicolon payload")] + [InlineData("us\"ers", "an embedded double quote")] + [InlineData("us'ers", "an embedded single quote")] + [InlineData("users\\", "a trailing backslash")] + [InlineData("users;", "a trailing semicolon")] + [InlineData("us;ers", "an embedded semicolon")] + public void assert_identifier_rejects_quote_backtick_backslash_and_semicolon(string name, string description) + { + var migrator = new MySqlMigrator(); + + Should.Throw( + () => migrator.AssertValidIdentifier(name), + $"Expected {description} to be rejected"); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData("us ers")] + [InlineData("us\ters")] + [InlineData("us\ners")] + [InlineData("us\rers")] + [InlineData("users\n-- the rest of this statement is now a comment")] + public void assert_identifier_rejects_null_empty_and_whitespace(string? name) + { + var migrator = new MySqlMigrator(); + + Should.Throw(() => migrator.AssertValidIdentifier(name!)); + } + + [Fact] + public void assert_identifier_rejects_names_past_the_length_limit() + { + var migrator = new MySqlMigrator(); + + Should.NotThrow(() => migrator.AssertValidIdentifier(new string('a', 64))); + Should.Throw(() => migrator.AssertValidIdentifier(new string('a', 65))); + } + + [Fact] + public void invalid_identifier_message_says_which_rule_was_broken() + { + var migrator = new MySqlMigrator(); + + var ex = Should.Throw(() => migrator.AssertValidIdentifier("us`ers")); + + ex.Message.ShouldContain("us`ers"); + ex.Message.ShouldContain("backtick"); + } + + /// + /// Names Weasel and its consumers actually generate must keep working -- the tightening is aimed at + /// a handful of characters, not at narrowing the identifier grammar. + /// + [Theory] + [InlineData("mt_doc_user")] + [InlineData("mt_stream")] + [InlineData("mt_doc_user_hilo")] + [InlineData("users$1")] + [InlineData("_leading_underscore")] + [InlineData("MixedCaseName")] + [InlineData("naïve_café")] + [InlineData("table-with-dashes")] + [InlineData("mt_doc_target.p_tenant_one")] + public void assert_identifier_still_accepts_ordinary_names(string name) + { + Should.NotThrow(() => new MySqlMigrator().AssertValidIdentifier(name)); + } + [Fact] public async Task can_ensure_database_that_does_not_exist() { diff --git a/src/Weasel.MySql/MySqlMigrator.cs b/src/Weasel.MySql/MySqlMigrator.cs index 5b07baf4..4ebd4fdf 100644 --- a/src/Weasel.MySql/MySqlMigrator.cs +++ b/src/Weasel.MySql/MySqlMigrator.cs @@ -107,12 +107,39 @@ public override string ToExecuteScriptLine(string scriptName) return $"source {scriptName}"; } + /// + /// The characters that are unsafe in a MySQL identifier beyond the universal ones: the backtick + /// MySQL delimits identifiers with ( wraps every name in + /// backticks and does not double an embedded one), the double quote that delimits identifiers under + /// ANSI_QUOTES and string literals otherwise, and the backslash, which is an escape character + /// inside MySQL string literals unless NO_BACKSLASH_ESCAPES is set -- a trailing one would + /// otherwise swallow the closing quote of the literal a name is written into. + /// + private const string UnsafeIdentifierCharacters = "`\"\\"; + + /// + /// MySQL's identifier length limit. + /// + public int MaxIdentifierLength { get; set; } = 64; + + /// + /// Validates a database object name before it is written into DDL. See + /// for why each rule is here; before weasel#416 this checked + /// length only, and threw on a null name. + /// + /// The name cannot be safely written into DDL. public override void AssertValidIdentifier(string name) { - // MySQL identifiers can be up to 64 characters - if (name.Length > 64) + var problem = IdentifierValidation.FindProblem(name, UnsafeIdentifierCharacters); + if (problem != null) + { + throw new ArgumentException($"MySQL identifier '{name}' is not valid because {problem}."); + } + + if (name.Length > MaxIdentifierLength) { - throw new ArgumentException($"MySQL identifier '{name}' exceeds the 64 character limit."); + throw new ArgumentException( + $"MySQL identifier '{name}' exceeds the {MaxIdentifierLength} character limit."); } } diff --git a/src/Weasel.Oracle.Tests/OracleMigratorTests.cs b/src/Weasel.Oracle.Tests/OracleMigratorTests.cs index cc649b00..b1487e2a 100644 --- a/src/Weasel.Oracle.Tests/OracleMigratorTests.cs +++ b/src/Weasel.Oracle.Tests/OracleMigratorTests.cs @@ -28,6 +28,88 @@ public void create_table_returns_oracle_table() table.Identifier.ShouldBe(identifier); } + /// + /// weasel#416. AssertValidIdentifier is the only identifier check in the stack, and this provider's + /// checked length only until now, so it has to reject the characters that let a name escape the + /// statement it is written into. A " closes a quoted identifier and a ; starts a new + /// statement. The ' matters especially here: Weasel wraps Oracle DDL in an anonymous PL/SQL + /// block and runs it via EXECUTE IMMEDIATE, so the name is written inside a string literal. + /// + [Theory] + [InlineData("USERS\"", "a trailing double quote")] + [InlineData("\"USERS", "a leading double quote")] + [InlineData("US\"ERS", "an embedded double quote")] + [InlineData("\"USERS\"", "a fully quote-wrapped name")] + [InlineData("USERS\"; DROP TABLE USERS; --", "a quote-and-semicolon payload")] + [InlineData("USERS'", "a trailing single quote")] + [InlineData("US'ERS", "an embedded single quote")] + [InlineData("USERS'; EXECUTE IMMEDIATE 'DROP TABLE USERS", "a literal-breaking payload")] + [InlineData("USERS;", "a trailing semicolon")] + [InlineData("US;ERS", "an embedded semicolon")] + public void assert_identifier_rejects_quote_and_semicolon(string name, string description) + { + var migrator = new OracleMigrator(); + + Should.Throw( + () => migrator.AssertValidIdentifier(name), + $"Expected {description} to be rejected"); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData("US ERS")] + [InlineData("US\tERS")] + [InlineData("US\nERS")] + [InlineData("US\rERS")] + [InlineData("USERS\n-- the rest of this statement is now a comment")] + public void assert_identifier_rejects_null_empty_and_whitespace(string? name) + { + var migrator = new OracleMigrator(); + + Should.Throw(() => migrator.AssertValidIdentifier(name!)); + } + + [Fact] + public void assert_identifier_rejects_names_past_the_length_limit() + { + var migrator = new OracleMigrator(); + + Should.NotThrow(() => migrator.AssertValidIdentifier(new string('A', 128))); + Should.Throw(() => migrator.AssertValidIdentifier(new string('A', 129))); + } + + [Fact] + public void invalid_identifier_message_says_which_rule_was_broken() + { + var migrator = new OracleMigrator(); + + var ex = Should.Throw(() => migrator.AssertValidIdentifier("US\"ERS")); + + ex.Message.ShouldContain("US\"ERS"); + ex.Message.ShouldContain("double quote"); + } + + /// + /// Names Weasel and its consumers actually generate must keep working -- the tightening is aimed at + /// a handful of characters, not at narrowing the identifier grammar. + /// + [Theory] + [InlineData("MT_DOC_USER")] + [InlineData("MT_STREAM")] + [InlineData("mt_doc_user_hilo")] + [InlineData("USERS$1")] + [InlineData("_LEADING_UNDERSCORE")] + [InlineData("MixedCaseName")] + [InlineData("naïve_café")] + [InlineData("TABLE-WITH-DASHES")] + [InlineData("MT_DOC_TARGET.P_TENANT_ONE")] + public void assert_identifier_still_accepts_ordinary_names(string name) + { + Should.NotThrow(() => new OracleMigrator().AssertValidIdentifier(name)); + } + [Fact] public async Task ensure_database_is_idempotent() { diff --git a/src/Weasel.Oracle/OracleMigrator.cs b/src/Weasel.Oracle/OracleMigrator.cs index 623f1840..e15442d6 100644 --- a/src/Weasel.Oracle/OracleMigrator.cs +++ b/src/Weasel.Oracle/OracleMigrator.cs @@ -116,11 +116,40 @@ public override string ToExecuteScriptLine(string scriptName) return $"@{scriptName}"; } + /// + /// The character Oracle delimits identifiers with. A " closes a quoted identifier, and + /// does not double an embedded one. + /// + private const string UnsafeIdentifierCharacters = "\""; + + /// + /// Oracle's identifier length limit (12.2 and later). + /// + public int MaxIdentifierLength { get; set; } = 128; + + /// + /// Validates a database object name before it is written into DDL. See + /// for why each rule is here; before weasel#416 this checked + /// length only, and threw on a null name. + /// + /// + /// The single quote matters more on Oracle than elsewhere: Weasel wraps Oracle DDL in an anonymous + /// PL/SQL block and runs it through EXECUTE IMMEDIATE, so the whole statement -- object name + /// included -- is written inside a string literal. + /// + /// The name cannot be safely written into DDL. public override void AssertValidIdentifier(string name) { - if (name.Length > 128) + var problem = IdentifierValidation.FindProblem(name, UnsafeIdentifierCharacters); + if (problem != null) + { + throw new InvalidOperationException($"Oracle identifier '{name}' is not valid because {problem}."); + } + + if (name.Length > MaxIdentifierLength) { - throw new InvalidOperationException($"Oracle identifiers cannot exceed 128 characters. '{name}' is {name.Length} characters."); + throw new InvalidOperationException( + $"Oracle identifiers cannot exceed {MaxIdentifierLength} characters. '{name}' is {name.Length} characters."); } } diff --git a/src/Weasel.SqlServer.Tests/SqlServerMigratorTests.cs b/src/Weasel.SqlServer.Tests/SqlServerMigratorTests.cs index 43d23bb9..d710df71 100644 --- a/src/Weasel.SqlServer.Tests/SqlServerMigratorTests.cs +++ b/src/Weasel.SqlServer.Tests/SqlServerMigratorTests.cs @@ -29,6 +29,93 @@ public void create_table_returns_sql_server_table() table.Identifier.ShouldBe(identifier); } + /// + /// weasel#416. AssertValidIdentifier is the only identifier check in the stack, and this provider's + /// was an empty method body until now, so it has to reject the characters that let a name escape the + /// statement it is written into. SQL Server delimits identifiers with [...] as well as + /// "...", so ] matters alongside "; a ' closes a string literal, which is + /// where names land in the existence checks (IF OBJECT_ID('...')); a ; starts a new + /// statement. + /// + [Theory] + [InlineData("users\"", "a trailing double quote")] + [InlineData("\"users", "a leading double quote")] + [InlineData("us\"ers", "an embedded double quote")] + [InlineData("\"users\"", "a fully quote-wrapped name")] + [InlineData("users]", "a bracket that closes a delimited identifier")] + [InlineData("[users]", "a fully bracket-wrapped name")] + [InlineData("us]ers", "an embedded closing bracket")] + [InlineData("users]; drop table users; --", "a bracket-and-semicolon payload")] + [InlineData("users'", "a trailing single quote")] + [InlineData("us'ers", "an embedded single quote")] + [InlineData("users'); drop table users; --", "a quote-and-semicolon payload")] + [InlineData("users;", "a trailing semicolon")] + [InlineData("us;ers", "an embedded semicolon")] + public void assert_identifier_rejects_quote_bracket_and_semicolon(string name, string description) + { + var migrator = new SqlServerMigrator(); + + Should.Throw( + () => migrator.AssertValidIdentifier(name), + $"Expected {description} to be rejected"); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData("us ers")] + [InlineData("us\ters")] + [InlineData("us\ners")] + [InlineData("us\rers")] + [InlineData("users\n-- the rest of this statement is now a comment")] + public void assert_identifier_rejects_null_empty_and_whitespace(string? name) + { + var migrator = new SqlServerMigrator(); + + Should.Throw(() => migrator.AssertValidIdentifier(name!)); + } + + [Fact] + public void assert_identifier_rejects_names_past_the_sysname_limit() + { + var migrator = new SqlServerMigrator(); + + Should.NotThrow(() => migrator.AssertValidIdentifier(new string('a', 128))); + Should.Throw(() => migrator.AssertValidIdentifier(new string('a', 129))); + } + + [Fact] + public void invalid_identifier_message_says_which_rule_was_broken() + { + var migrator = new SqlServerMigrator(); + + var ex = Should.Throw(() => migrator.AssertValidIdentifier("us]ers")); + + ex.Message.ShouldContain("us]ers"); + ex.Message.ShouldContain("closing square bracket"); + } + + /// + /// Names Weasel and its consumers actually generate must keep working -- the tightening is aimed at + /// a handful of characters, not at narrowing the identifier grammar. + /// + [Theory] + [InlineData("mt_doc_user")] + [InlineData("mt_stream")] + [InlineData("mt_doc_user_hilo")] + [InlineData("users$1")] + [InlineData("_leading_underscore")] + [InlineData("MixedCaseName")] + [InlineData("naïve_café")] + [InlineData("table-with-dashes")] + [InlineData("#temp_table")] + [InlineData("mt_doc_target.p_tenant_one")] + public void assert_identifier_still_accepts_ordinary_names(string name) + { + Should.NotThrow(() => new SqlServerMigrator().AssertValidIdentifier(name)); + } + [Fact] public async Task can_ensure_database_that_does_not_exist() { diff --git a/src/Weasel.SqlServer/SqlServerMigrator.cs b/src/Weasel.SqlServer/SqlServerMigrator.cs index b6cedaea..9299f438 100644 --- a/src/Weasel.SqlServer/SqlServerMigrator.cs +++ b/src/Weasel.SqlServer/SqlServerMigrator.cs @@ -137,9 +137,40 @@ public override string ToExecuteScriptLine(string scriptName) return $":r {scriptName}"; } + /// + /// The characters SQL Server delimits identifiers with. ] is what closes a + /// [...] delimited identifier and " what closes a quoted one (SQL Server accepts both, + /// the latter under QUOTED_IDENTIFIER ON); [ is rejected alongside ] so that an + /// already-bracketed name is caught rather than being bracketed a second time. + /// + private const string UnsafeIdentifierCharacters = "[]\""; + + /// + /// SQL Server's identifier length limit -- sysname is nvarchar(128), so anything + /// longer is rejected by the server itself. + /// + public int MaxIdentifierLength { get; set; } = 128; + + /// + /// Validates a database object name before it is written into DDL. See + /// for why each rule is here; this method had no body at all + /// before weasel#416. + /// + /// The name cannot be safely written into DDL. public override void AssertValidIdentifier(string name) { - // Nothing yet + var problem = IdentifierValidation.FindProblem(name, UnsafeIdentifierCharacters); + if (problem != null) + { + throw new InvalidOperationException( + $"SQL Server identifier '{name}' is not valid because {problem}."); + } + + if (name.Length > MaxIdentifierLength) + { + throw new InvalidOperationException( + $"SQL Server identifiers cannot exceed {MaxIdentifierLength} characters. '{name}' is {name.Length} characters."); + } } private static async Task createSchemas( diff --git a/src/Weasel.Sqlite.Tests/SqliteMigratorTests.cs b/src/Weasel.Sqlite.Tests/SqliteMigratorTests.cs index 0ee07f02..57bc3874 100644 --- a/src/Weasel.Sqlite.Tests/SqliteMigratorTests.cs +++ b/src/Weasel.Sqlite.Tests/SqliteMigratorTests.cs @@ -105,6 +105,82 @@ public void assert_valid_identifier_rejects_too_long() Should.Throw(() => migrator.AssertValidIdentifier(longName)); } + /// + /// weasel#416. AssertValidIdentifier is the only identifier check in the stack, so it has to reject + /// the characters that let a name escape the statement it is written into. A " closes a + /// quoted identifier -- SchemaUtils.QuoteName doubles an embedded quote but only quotes at all for + /// keywords, spaces, dashes and leading digits, so an ordinary-looking name carrying one is written + /// out raw -- a ' closes the string literal the introspection path interpolates names into + /// (pragma_table_info('...')), and a ; starts a new statement. + /// + [Theory] + [InlineData("users\"", "a trailing double quote")] + [InlineData("\"users", "a leading double quote")] + [InlineData("us\"ers", "an embedded double quote")] + [InlineData("\"users\"", "a fully quote-wrapped name")] + [InlineData("users\"; drop table users; --", "a quote-and-semicolon payload")] + [InlineData("users'", "a trailing single quote")] + [InlineData("us'ers", "an embedded single quote")] + [InlineData("users'; drop table users; --", "a literal-breaking payload")] + [InlineData("users;", "a trailing semicolon")] + [InlineData("us;ers", "an embedded semicolon")] + public void assert_identifier_rejects_quote_and_semicolon(string name, string description) + { + var migrator = new SqliteMigrator(); + + Should.Throw( + () => migrator.AssertValidIdentifier(name), + $"Expected {description} to be rejected"); + } + + /// + /// Only null and all-whitespace names used to be checked, so a name could still carry an interior + /// newline and smuggle a '--' comment into the statement. + /// + [Theory] + [InlineData(null)] + [InlineData("us ers")] + [InlineData("us\ters")] + [InlineData("us\ners")] + [InlineData("us\rers")] + [InlineData("users\n-- the rest of this statement is now a comment")] + public void assert_identifier_rejects_null_and_interior_whitespace(string? name) + { + var migrator = new SqliteMigrator(); + + Should.Throw(() => migrator.AssertValidIdentifier(name!)); + } + + [Fact] + public void invalid_identifier_message_says_which_rule_was_broken() + { + var migrator = new SqliteMigrator(); + + var ex = Should.Throw(() => migrator.AssertValidIdentifier("us\"ers")); + + ex.Message.ShouldContain("us\"ers"); + ex.Message.ShouldContain("double quote"); + } + + /// + /// Names Weasel and its consumers actually generate must keep working -- the tightening is aimed at + /// a handful of characters, not at narrowing the identifier grammar. + /// + [Theory] + [InlineData("mt_doc_user")] + [InlineData("mt_stream")] + [InlineData("mt_doc_user_hilo")] + [InlineData("users$1")] + [InlineData("_leading_underscore")] + [InlineData("MixedCaseName")] + [InlineData("naïve_café")] + [InlineData("table-with-dashes")] + [InlineData("mt_doc_target.p_tenant_one")] + public void assert_identifier_still_accepts_ordinary_names(string name) + { + Should.NotThrow(() => new SqliteMigrator().AssertValidIdentifier(name)); + } + [Fact] public async Task ensure_database_exists_is_noop_for_memory() { diff --git a/src/Weasel.Sqlite/SqliteMigrator.cs b/src/Weasel.Sqlite/SqliteMigrator.cs index a77b3b36..1332245e 100644 --- a/src/Weasel.Sqlite/SqliteMigrator.cs +++ b/src/Weasel.Sqlite/SqliteMigrator.cs @@ -84,19 +84,42 @@ public override string ToExecuteScriptLine(string scriptName) return $".read {scriptName}"; } + /// + /// The character SQLite delimits identifiers with. does double + /// an embedded ", but only quotes at all for reserved keywords, spaces, dashes and leading + /// digits -- an ordinary-looking name carrying a quote is written out raw. + /// + private const string UnsafeIdentifierCharacters = "\""; + + /// + /// SQLite itself allows identifiers up to 1073741824 characters, which is not a useful limit; this + /// is the practical one Weasel enforces, in line with the other providers. + /// + public int MaxIdentifierLength { get; set; } = 255; + + /// + /// Validates a database object name before it is written into DDL. See + /// for why each rule is here; before weasel#416 this checked + /// null/whitespace and length only. + /// + /// + /// The single quote matters here because SQLite's introspection path interpolates the table name + /// into string literals -- WHERE name = '...' against sqlite_master and + /// pragma_table_info('...') in Table.FetchExisting. + /// + /// The name cannot be safely written into DDL. public override void AssertValidIdentifier(string name) { - if (string.IsNullOrWhiteSpace(name)) + var problem = IdentifierValidation.FindProblem(name, UnsafeIdentifierCharacters); + if (problem != null) { - throw new InvalidOperationException($"SQLite identifier cannot be empty or whitespace"); + throw new InvalidOperationException($"SQLite identifier '{name}' is not valid because {problem}."); } - // SQLite is quite permissive with identifier names, but we should check for basic issues - // SQLite allows up to 1073741824 characters, but that's impractical - // We'll use a reasonable limit similar to other databases - if (name.Length > 255) + if (name.Length > MaxIdentifierLength) { - throw new InvalidOperationException($"SQLite identifier '{name}' is too long ({name.Length} characters). Maximum recommended length is 255."); + throw new InvalidOperationException( + $"SQLite identifier '{name}' is too long ({name.Length} characters). Maximum recommended length is {MaxIdentifierLength}."); } }