BrighterCommand / BrighterCommand/Brighter

MsSqlQueueBuilder.GetExistsQuery formats both names into SQL string literals

Open
#4,345 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C#
Stars
2.5k
Forks
296
Avg merge
1d 11h
Merged PRs (30d)
21

Description

`src/Paramore.Brighter.MessagingGateway.MsSql/MsSqlQueueBuilder.cs:84`:

```csharp
public static string GetExistsQuery(string queueTableName, string schemaName = "dbo") =>
string.Format(QUEUE_EXISTS_SQL, queueTableName, schemaName);
```

and the template it formats into:

```sql
WHERE t.name = '{0}' AND s.name = '{1}'
```

Both names land inside SQL string literals with no escaping, so a name containing a single quote changes the statement. `GetDDL` and `GetIndexDDL` have the same shape into `CREATE TABLE [{0}]`, where the character that matters is `]` rather than `'`.

### Why this is worth fixing rather than guarding at each call site

`MsSqlQueueBuilder` is **public API** — deliberately so, because the MSSQL gateway had no provisioning path and callers were expected to run the DDL themselves. So the guard cannot live in a caller without every caller re-deriving it, and each one getting it slightly differently.

A worked example of getting it wrong: while moving the sample's provisioner into the gateway (#4343) I ported a guard of `^[A-Za-z_][A-Za-z0-9_]{0,127}$`, which looks careful and is wrong — a **bracketed** identifier legally holds hyphens, and this repository's own test suite names queue tables `queue_test_`. The existing suite caught it. That is the argument for one guard, in the builder, next to the `string.Format`.

### Suggested fix

- `GetExistsQuery`: bind the two names as parameters rather than formatting them, or at minimum escape `'` as `''`.
- `GetDDL` / `GetIndexDDL`: escape `]` as `]]`, which is SQL Server's own convention for a bracketed identifier, and reject a name over 128 characters.

A parameterised existence check also fixes a second, quieter problem: `GetExistsQuery` defaults `schemaName` to a literal `dbo`, while every statement the gateway issues against the queue is **unqualified** and so resolves through the caller's default schema. A login whose default schema is not `dbo` gets an existence check that disagrees with the runtime. #4343 sidesteps this by asking `SCHEMA_NAME()`, but `GetExistsQuery` is public and still carries the `dbo` default.

Raised in review of #4331; #4343 is the first production caller of this family of methods.

Contributor guide

Open the contributing guide

Research direction

Start in src/Paramore.Brighter.MessagingGateway.MsSql/MsSqlQueueBuilder.cs, focusing on GetExistsQuery, GetDDL, and GetIndexDDL, then inspect the existing tests and the production caller from #4343. Verify behavior for quoted names, bracketed identifiers, names over 128 characters, and schemas other than dbo; done means generated SQL remains valid and existence checks match the gateway's schema resolution.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
databases
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.