Bring the other four providers' AssertValidIdentifier in line with PostgreSQL (weasel#416) - #420
Merged
jeremydmiller merged 5 commits intoJul 31, 2026
Conversation
…el#416) b4ffbe4 hardened PostgresqlMigrator.AssertValidIdentifier and left the other four providers as they were: SqlServerMigrator's body was "// Nothing yet", Oracle and MySql checked length only, Sqlite null/whitespace and length. Bringing all four into line means writing the same loop four times, so it goes here instead. IdentifierValidation.FindProblem takes the name and the characters that are unsafe for that provider specifically, and returns why the name was rejected -- phrased to follow "because" -- or null. The provider keeps what is genuinely its own: the delimiter set, the length limit, and the exception type it has always thrown. Two rules are universal and so are not passed in: * ';' ends the statement and starts another. * '\'' closes a string literal, and object names do reach literals on every provider -- SQL Server's IF OBJECT_ID('...'), Oracle's WHERE table_name = '...' inside the anonymous PL/SQL block it wraps DDL in, SQLite's pragma_table_info('...'). MySQL parameterises its introspection, so there the rule is uniformity rather than a live hole. Whitespace is rejected in full rather than just the literal space character, as on PostgreSQL, so a newline cannot introduce a '--' comment into an unquoted name. PostgresqlMigrator is deliberately not refactored onto this. Its rules are the same minus the single quote; folding it in would be a no-behaviour-change edit to code shipped in 9.23.0, and this change is already touching four providers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…l#416)
AssertValidIdentifier was an empty method body -- "// Nothing yet" -- and it is the
only identifier check in the stack: DbObjectName and SqlServerObjectName do not
validate, and DatabaseBase runs every schema object's name through the migrator on
the way to DDL. So this provider accepted any name at all.
It now rejects, via IdentifierValidation.FindProblem:
* ']', which closes a [...] delimited identifier, and '[' with it so that an
already-bracketed name is caught rather than bracketed a second time.
* '"', which closes a quoted identifier under QUOTED_IDENTIFIER ON. SchemaUtils
.QuoteName brackets reserved keywords only and doubles nothing.
* '\'', which closes a string literal. Object names reach literals here in the
existence checks and introspection -- Table.WriteCreateStatement's
IF OBJECT_ID('{Identifier}'), StoredProcedure's sys.objects.name = '{...}',
TableColumn's OBJECT_ID('{parent.Identifier}').
* ';', whitespace anywhere, and null/empty.
A length limit is new too: sysname is nvarchar(128), so 128 passes and 129 does
not. MaxIdentifierLength is settable for parity with PostgresqlMigrator
.NameDataLength.
InvalidOperationException rather than a new typed exception -- the provider has no
existing identifier exception to extend, and adding one across four providers is a
wider API change than this needs to be.
Behaviour change for downstream consumers: a name with a space in it used to reach
DDL and would have worked when bracketed, and no longer does. Nothing in the
suites generates such a name. Full Weasel.SqlServer suite green against the
azure-sql-edge container (376 passed, 8 pre-existing skips), plus
Weasel.EntityFrameworkCore (106 passed, 1 skip) and Weasel.CommandLine (23) as the
nearest downstream consumers.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ntifier (weasel#416) The check was length only, and it threw NullReferenceException on a null name rather than saying anything useful. It is the only identifier check in the stack -- DbObjectName and OracleObjectName do not validate -- so '"', '\'' and ';' all went through to DDL. Now rejected, via IdentifierValidation.FindProblem: '"' (closes a quoted identifier; SchemaUtils.QuoteName quotes reserved keywords only and doubles nothing), '\'', ';', whitespace anywhere, and null/empty. The length limit is unchanged at 128, now settable via MaxIdentifierLength. The single quote matters more here than elsewhere: Weasel wraps Oracle DDL in an anonymous PL/SQL block and executes it with EXECUTE IMMEDIATE, so the whole statement -- object name included -- sits inside a string literal, and the existence checks interpolate names into WHERE table_name = '...' besides. InvalidOperationException is kept as the exception type, so existing callers that catch it still do. Full Weasel.Oracle suite green against the oracle-free container (215 passed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ValidIdentifier (weasel#416) The check was length only, and it threw NullReferenceException on a null name. As on the other providers it is the only identifier check in the stack, so a name carrying a delimiter reached DDL intact. Now rejected, via IdentifierValidation.FindProblem: * '`', the delimiter. SchemaUtils.QuoteName wraps every name in backticks and does not double an embedded one, so a name carrying one does not stay inside its own quotes. * '"', which delimits identifiers under ANSI_QUOTES and string literals otherwise. * '\\', an escape character inside MySQL string literals unless NO_BACKSLASH_ESCAPES is set -- a trailing one swallows the closing quote of the literal a name is written into. * '\'', ';', whitespace anywhere, and null/empty. MySQL is the one provider of the four whose introspection parameterises the table name (Table.FetchExisting binds it), so the single quote here is uniformity with the others rather than a live hole. The 64 character limit is unchanged, now settable via MaxIdentifierLength. ArgumentException is kept as the exception type, so existing callers that catch it still do. Full Weasel.MySql suite green against the mysql:8.0 container (227 passed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tValidIdentifier (weasel#416)
The check covered null/all-whitespace and length, which left '"', '\'', ';' and
interior whitespace -- a name could carry a newline and smuggle a '--' comment into
the statement it was written into.
Now rejected, via IdentifierValidation.FindProblem: '"', '\'', ';', whitespace
anywhere, and null/empty. The 255 character limit is unchanged, now settable via
MaxIdentifierLength.
SQLite's SchemaUtils.QuoteName is the only one of the four that doubles an embedded
quote, but it only quotes at all for reserved keywords, spaces, dashes and leading
digits -- so an ordinary-looking name carrying a quote is written out raw. The
single quote matters because the introspection path interpolates the table name
into literals: WHERE name = '...' against sqlite_master, and
pragma_table_info('...') in Table.FetchExisting.
InvalidOperationException is kept as the exception type; the existing
assert_valid_identifier_rejects_empty and _rejects_too_long tests pin it and still
pass unchanged.
Full Weasel.Sqlite suite green (387 passed, no container needed).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #416, which was closed after #419 (
b4ffbe4) hardened PostgreSQL alone. That commit's own notes listed the rest as deliberately not ridden along; this is that work.AssertValidIdentifieris the only identifier check in the stack on every provider —DbObjectNameand its provider subclasses do no validation, andDatabaseBaseruns every schema object's name through the migrator on the migration path (DatabaseBase.cs:330, :544). What the four providers did with that responsibility before this:// Nothing yet— empty method bodyNone rejected a delimiter or a
;.What each now rejects
Universal (in
IdentifierValidation, the shared helper):;,', whitespace anywhere, null/empty. The single quote is there because object names genuinely reach string literals on these providers — SQL Server'sIF OBJECT_ID('…')andsys.objects.name = '…', Oracle'sWHERE table_name = '…'inside the anonymous PL/SQL block it wraps DDL in, SQLite'spragma_table_info('…'). MySQL parameterises its introspection, so there it is uniformity rather than a live hole. Whitespace is rejected in full rather than just the space character, as on PostgreSQL, so a newline can't introduce a--comment.Per provider, its own delimiters:
](closes a[…]identifier) and[with it, so an already-bracketed name is caught rather than bracketed twice;"forQUOTED_IDENTIFIER ON. New length limit of 128, thesysnamebound."(identifier delimiter underANSI_QUOTES, string delimiter otherwise);\, which escapes the next character inside a literal unlessNO_BACKSLASH_ESCAPESis set, so a trailing one swallows the literal's closing quote.".Weasel's quoting helpers do not double an embedded delimiter, so a name carrying one does not stay inside its own quotes. SQLite's
SchemaUtils.QuoteNamedoes double it, but only quotes at all for keywords, spaces, dashes and leading digits — an ordinary-looking name carrying a quote is written out raw.Shape
One PR, five commits: the shared helper, then one per provider. The rule set is one decision and reviews better in one place, but each provider's commit stands alone and is revertable on its own if one of them turns out to bite a downstream consumer.
Two judgement calls worth flagging:
InvalidOperationExceptionfor SQL Server/Oracle/SQLite,ArgumentExceptionfor MySQL — so existingcatchsites and the existing SQLite tests are unaffected. The message now names the rule that was broken. PostgreSQL's typedPostgresqlIdentifierInvalidExceptionwith itsReasonproperty is the nicer shape; matching it across four providers is a wider API change than this needs, and can follow.PostgresqlMigratoris not refactored onto the helper. Its rules are the same minus the single quote, so folding it in would be a no-behaviour-change edit to code that shipped in 9.23.0, on top of a change already touching four providers.Behaviour change
This tightens what four providers accept, so it is a breaking change for anyone whose schema object names contain these characters. The one plausible case is a SQL Server name with a space in it (
Order Details), which used to reach DDL and would have worked bracketed. Nothing in any suite generates such a name, andassert_identifier_still_accepts_ordinary_namespinsmt_*names,$, leading underscores, mixed case, non-ASCII letters, dashes, dotted partition suffixes and (SQL Server)#tempnames as still valid.Regression pass
Full suite per provider, containers from
docker compose up, all on net9.0:The last two are the nearest downstream consumers of all four providers in-repo.
Closes the identifier half of #416 for the remaining providers.
🤖 Generated with Claude Code