Bring the other four providers' AssertValidIdentifier in line with PostgreSQL (weasel#416)

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");
+    }
+
+    /// <summary>
+    ///     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.
+    /// </summary>
+    [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);
+    }
+
+    /// <summary>
+    ///     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.
+    /// </summary>
+    [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();
+    }
+}

src/Weasel.Core/IdentifierValidation.cs

@@ -0,0 +1,85 @@
+namespace Weasel.Core;
+
+/// <summary>
+///     The identifier checks every provider needs, factored out so each provider's
+///     <see cref="Migrator.AssertValidIdentifier" /> 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).
+/// </summary>
+/// <remarks>
+///     <para>
+///         <see cref="Migrator.AssertValidIdentifier" /> is the only identifier check in the stack --
+///         <see cref="DbObjectName" /> and its provider-specific subclasses do no validation of their own --
+///         and the migration path runs every schema object's name through it
+///         (<c>DatabaseBase.ApplyAllConfiguredChangesToDatabaseAsync</c> and
+///         <c>DatabaseBase.generateOrUpdateFeature</c>). So it has to reject the characters that let a name
+///         escape the statement it is written into.
+///     </para>
+///     <para>
+///         Two of those are the same for everyone. A <c>;</c> ends the statement and starts another. A
+///         <c>'</c> 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
+///         <c>IF OBJECT_ID('...')</c>, Oracle's <c>WHERE table_name = '...'</c> inside an anonymous PL/SQL
+///         block, SQLite's <c>pragma_table_info('...')</c>). Whitespace is rejected in full rather than just
+///         the literal space character, so that a newline cannot introduce a <c>--</c> comment into an
+///         unquoted name.
+///     </para>
+///     <para>
+///         The rest is per-provider, because the character that closes an identifier is not: SQL Server
+///         delimits with <c>[...]</c> as well as <c>"..."</c>, MySQL with backticks, Oracle and SQLite with
+///         <c>"</c>. Weasel's quoting helpers do not double an embedded delimiter (SQLite's
+///         <c>SchemaUtils.QuoteName</c> 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.
+///     </para>
+/// </remarks>
+public static class IdentifierValidation
+{
+    /// <summary>
+    ///     Returns why <paramref name="name" /> is unsafe to write into DDL, phrased to follow "because",
+    ///     or <c>null</c> when it passes. Length is deliberately not checked here -- the limit and the
+    ///     exception that reports it differ per provider.
+    /// </summary>
+    /// <param name="name">The identifier to check. Null, empty and all-whitespace are rejected.</param>
+    /// <param name="unsafeCharacters">
+    ///     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.
+    /// </param>
+    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}'"
+        };
+    }
+}

src/Weasel.MySql.Tests/MySqlMigratorTests.cs

@@ -28,6 +28,87 @@ public void create_table_returns_mysql_table()
         table.Identifier.ShouldBe(identifier);
     }
 
+    /// <summary>
+    ///     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 <c>"</c>
+    ///     under ANSI_QUOTES -- a <c>'</c> closes a string literal, a <c>\</c> escapes the character after
+    ///     it inside one, and a <c>;</c> starts a new statement.
+    /// </summary>
+    [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<ArgumentException>(
+            () => 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<ArgumentException>(() => 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<ArgumentException>(() => 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<ArgumentException>(() => migrator.AssertValidIdentifier("us`ers"));
+
+        ex.Message.ShouldContain("us`ers");
+        ex.Message.ShouldContain("backtick");
+    }
+
+    /// <summary>
+    ///     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.
+    /// </summary>
+    [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()
     {

src/Weasel.MySql/MySqlMigrator.cs

@@ -107,12 +107,39 @@ public override string ToExecuteScriptLine(string scriptName)
         return $"source {scriptName}";
     }
 
+    /// <summary>
+    ///     The characters that are unsafe in a MySQL identifier beyond the universal ones: the backtick
+    ///     MySQL delimits identifiers with (<see cref="SchemaUtils.QuoteName" /> wraps every name in
+    ///     backticks and does not double an embedded one), the double quote that delimits identifiers under
+    ///     <c>ANSI_QUOTES</c> and string literals otherwise, and the backslash, which is an escape character
+    ///     inside MySQL string literals unless <c>NO_BACKSLASH_ESCAPES</c> is set -- a trailing one would
+    ///     otherwise swallow the closing quote of the literal a name is written into.
+    /// </summary>
+    private const string UnsafeIdentifierCharacters = "`\"\\";
+
+    /// <summary>
+    ///     MySQL's identifier length limit.
+    /// </summary>
+    public int MaxIdentifierLength { get; set; } = 64;
+
+    /// <summary>
+    ///     Validates a database object name before it is written into DDL. See
+    ///     <see cref="IdentifierValidation" /> for why each rule is here; before weasel#416 this checked
+    ///     length only, and threw <see cref="NullReferenceException" /> on a null name.
+    /// </summary>
+    /// <exception cref="ArgumentException">The name cannot be safely written into DDL.</exception>
     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.");
         }
     }
 

src/Weasel.Oracle.Tests/OracleMigratorTests.cs

@@ -28,6 +28,88 @@ public void create_table_returns_oracle_table()
         table.Identifier.ShouldBe(identifier);
     }
 
+    /// <summary>
+    ///     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 <c>"</c> closes a quoted identifier and a <c>;</c> starts a new
+    ///     statement. The <c>'</c> 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.
+    /// </summary>
+    [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<InvalidOperationException>(
+            () => 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<InvalidOperationException>(() => 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<InvalidOperationException>(() => 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<InvalidOperationException>(() => migrator.AssertValidIdentifier("US\"ERS"));
+
+        ex.Message.ShouldContain("US\"ERS");
+        ex.Message.ShouldContain("double quote");
+    }
+
+    /// <summary>
+    ///     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.
+    /// </summary>
+    [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()
     {