From d7dce43da87e1edf90e4d05d44a317222ab70808 Mon Sep 17 00:00:00 2001 From: Mark Kincaid Date: Tue, 19 May 2026 14:42:46 -0700 Subject: [PATCH 1/8] Add guidance on datetime generation in stored procedures Establishes the rule that datetime values must be generated in application code and passed as parameters rather than using SYSUTCDATETIME()/GETUTCDATE() inline. Documents the four accepted exceptions found in the codebase: account revision date bumping, bulk operations with consistent timestamps, WHERE clause predicates, and nullable parameter fallbacks. --- docs/contributing/code-style/sql.md | 67 +++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/docs/contributing/code-style/sql.md b/docs/contributing/code-style/sql.md index bac95c5bc..7ddb43670 100644 --- a/docs/contributing/code-style/sql.md +++ b/docs/contributing/code-style/sql.md @@ -88,6 +88,10 @@ These standards should be applied across any T-SQL scripts that you write. `NVARCHAR(50)` not `NVARCHAR (50)`, `DATETIME2(7)` not `DATETIME2 (7)`) - **ID generation**: Use `CoreHelpers.GenerateComb()` in application code, not `NEWID()` in the database -- see [GUID generation](./csharp#guid-generation) +- **Datetime generation**: Generate datetime values in application code and pass them as parameters + (e.g., `@RevisionDate`), not using SQL functions (`SYSUTCDATETIME()`, `GETUTCDATE()`) — this keeps + timestamp generation consistent and testable. See the [accepted exceptions](#datetime-values) + documented in the stored procedures section. ### `SELECT` statements @@ -419,6 +423,69 @@ Use `SET NOCOUNT ON` to prevent the automatic return of row count messages, whic performance and ensures consistent behavior across different client applications that might handle these messages differently. +#### Datetime values + +As noted in the general standards, datetime values must be generated in application code and passed +as parameters. Do **not** use `SYSUTCDATETIME()` or `GETUTCDATE()` inline: + +```sql +-- Wrong +UPDATE + [dbo].[Entity] +SET + [RevisionDate] = GETUTCDATE() +WHERE + [Id] = @Id + +-- Correct +UPDATE + [dbo].[Entity] +SET + [RevisionDate] = @RevisionDate +WHERE + [Id] = @Id +``` + +**Accepted exceptions** + +There are specific patterns in the codebase where using a SQL datetime function is intentional and +correct: + +1. **Account revision date bumping** — The `User_BumpAccountRevisionDate*` family of procedures + exist solely to stamp `[AccountRevisionDate]` to the current UTC time atomically, without a + round-trip to application code. + +2. **Bulk operations requiring a consistent timestamp** — When a procedure must apply the same + timestamp across multiple rows or statements in one transaction, declare a local variable once + and reuse it throughout the procedure: + + ```sql + DECLARE @UtcNow DATETIME2(7) = SYSUTCDATETIME(); + + UPDATE + [dbo].[Cipher] + SET + [DeletedDate] = @UtcNow, + [RevisionDate] = @UtcNow + WHERE + [Id] IN (SELECT [Id] FROM @Ids) + ``` + +3. **`WHERE` clause predicates** — Comparing against the current time in a filter is acceptable + (e.g., deleting expired records, skipping rows whose date has not changed since yesterday): + + ```sql + WHERE + [ExpirationDate] < GETUTCDATE() + ``` + +4. **Nullable parameter fallbacks** — When a parameter is intentionally nullable and the procedure + should default to the current time when no value is supplied: + + ```sql + SET @Created = COALESCE(@Created, GETUTCDATE()) + ``` + #### `INSERT` statements - Column list in parentheses, one column per line From ee5e908c41d2e515fa47682df460498402e86356 Mon Sep 17 00:00:00 2001 From: Mark Kincaid Date: Wed, 15 Jul 2026 08:44:54 -0700 Subject: [PATCH 2/8] Refine T-SQL stored procedure naming, formatting, and deployment guidance Clarifies stored procedure naming with standard action verbs, moves parameter guidance to the basic structure section, adds a naming convention for full unabbreviated names, and notes EDD constraints on deployment scripts. --- docs/contributing/code-style/sql.md | 55 +++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 15 deletions(-) diff --git a/docs/contributing/code-style/sql.md b/docs/contributing/code-style/sql.md index 7ddb43670..5fdc6ef43 100644 --- a/docs/contributing/code-style/sql.md +++ b/docs/contributing/code-style/sql.md @@ -55,8 +55,6 @@ This separation of concerns means: - e.g. `UserView.sql`, `ApiKeyDetailsView.sql` - **Functions**: `{EntityName}{Purpose}.sql` - e.g. `UserCollectionDetails.sql` -- **User Defined Types**: `{TypeName}.sql` - - e.g. `GuidIdArray.sql` :::tip Versioning @@ -83,9 +81,11 @@ These standards should be applied across any T-SQL scripts that you write. makes code changes easily detectable - **Blank lines**: Separate sections of code with at least one blank line - **Commas**: Commas should be placed at the right end of the line -- **Parentheses**: Parentheses should be vertically aligned with spanning multiple lines +- **Parentheses**: Parentheses should be vertically aligned when spanning multiple lines - **Data type modifiers**: Omit the space between type name and opening parenthesis (e.g., `NVARCHAR(50)` not `NVARCHAR (50)`, `DATETIME2(7)` not `DATETIME2 (7)`) +- **Naming**: Use full, unabbreviated names throughout — object names, column names, parameters, and + descriptors (e.g., `OrganizationId` not `OrgId`) - **ID generation**: Use `CoreHelpers.GenerateComb()` in application code, not `NEWID()` in the database -- see [GUID generation](./csharp#guid-generation) - **Datetime generation**: Generate datetime values in application code and pass them as parameters @@ -276,19 +276,34 @@ LEFT JOIN ### Stored procedures -- **Stored Procedure Name**: `{EntityName}_{Action}` format (e.g., `[dbo].[User_ReadById]`) - - EntityName: The main table or concept (e.g. User, Organization, Cipher) - - Action: What the procedure does (e.g. Create, ReadById, DeleteMany) -- **Parameters**: Start with `@` and use PascalCase (e.g., `@UserId`, `@OrganizationId`) -- **OUTPUT parameters**: Explicitly declare with `OUTPUT` keyword +#### Naming + +Stored procedures follow the `{EntityName}_{Action}` format (e.g., `[dbo].[User_ReadById]`): + +- **EntityName**: The main table or concept the procedure operates on (e.g., `User`, `Organization`, + `Cipher`) +- **Action**: A verb from the standard list below, optionally followed by a short descriptor that + clarifies what the procedure does + +**Standard action verbs** + +| Verb | Description | +| ------------ | ----------------------- | +| `Create` | Insert a new record | +| `Read` | Select a single record | +| `ReadMany` | Select multiple records | +| `Update` | Modify a record | +| `UpdateMany` | Modify multiple records | +| `Delete` | Remove a record | +| `DeleteMany` | Remove multiple records | -:::tip Example of common CRUD operations +:::tip When an operation is more specific than a standard verb alone, append a short descriptor: -- **Create**: `{EntityName}_Create` procedures -- **Read**: `{EntityName}_ReadById`, `{EntityName}_ReadBy{Criteria}` procedures -- **Read Many**: `{EntityName}_ReadManyByIds`, `{EntityName}_ReadManyBy{Criteria}` procedures -- **Update**: `{EntityName}_Update` procedures -- **Delete**: `{EntityName}_DeleteById`, `{EntityName}_Delete` procedures +- `User_ReadById` — read filtered by a specific field +- `User_ReadManyByOrganizationId` — filtered bulk read +- `OrganizationUser_UpdateStatus` — update a specific field +- `OrganizationUser_UpdateManyRevoke` — bulk revoke +- `User_UpdateApplicationData` — update a named subset of fields ::: @@ -303,6 +318,8 @@ These are incorrect and should not be used as a reference. Always use `Read` or #### Basic structure +- **Parameters**: Start with `@` and use PascalCase (e.g., `@UserId`, `@OrganizationId`) +- **OUTPUT parameters**: Explicitly declare with `OUTPUT` keyword - Wrap the entire procedure body in `BEGIN`/`END` statements ```sql @@ -401,7 +418,7 @@ BEGIN WHERE [OrganizationId] = @OrganizationId AND [Status] = 2 -- 2 = Confirmed - AND [Type] = @Role + AND [Role] = @Role END ``` @@ -709,6 +726,14 @@ END CATCH; ## Deployment scripts +:::note Evolutionary database design + +Bitwarden follows [Evolutionary Database Design (EDD)](../database-migrations/edd). If a deployment +fails and server code is rolled back, database changes are **not** rolled back with it. This means +all migrations must support both the current release and the next release simultaneously. + +::: + There are specific ways migration scripts should be structured. We do so to adhere to the following guiding principles: From 8424ce0977758df82d9a07c5e915518830a479e2 Mon Sep 17 00:00:00 2001 From: Mark Kincaid Date: Wed, 15 Jul 2026 10:21:43 -0700 Subject: [PATCH 3/8] Add JSON and explicit transaction guidance to T-SQL style guide Expands user defined types guidance with OPENJSON examples for passing structured data as an alternative to new TVPs, clarifies parameter values should come from application code, and adds guidance against wrapping single statements in explicit transactions based on existing anti-patterns in the codebase. --- docs/contributing/code-style/sql.md | 159 ++++++++++++++++++++++++++-- 1 file changed, 148 insertions(+), 11 deletions(-) diff --git a/docs/contributing/code-style/sql.md b/docs/contributing/code-style/sql.md index 5fdc6ef43..c5bb87527 100644 --- a/docs/contributing/code-style/sql.md +++ b/docs/contributing/code-style/sql.md @@ -428,6 +428,9 @@ END - Align parameters with consistent indentation (4 spaces) - Default values on same line as parameter - `OUTPUT` parameters clearly marked +- Pass values in as parameters from application code rather than hard-coding them or generating them + with a SQL function inside the procedure (e.g., `GETUTCDATE()`) -- see + [accepted exceptions](#datetime-values) for cases where a SQL function is appropriate :::warning Default parameter values @@ -539,6 +542,88 @@ WHERE [Id] = @Id ``` +#### Explicit transactions + +Only wrap statements in an explicit `BEGIN TRANSACTION` / `COMMIT TRANSACTION` when a procedure +performs multiple statements that must all succeed or all fail together. A single `INSERT`, +`UPDATE`, or `DELETE` statement is already atomic on its own -- SQL Server implicitly wraps every +individual statement in a transaction, so adding an explicit one around it adds nothing except +unnecessary lock hold time and noise. When a transaction is needed, keep its scope as small as +possible -- only the statements that need to be atomic, not unrelated reads or `EXEC` calls that +don't need to roll back with them. + +:::warning Do not wrap a single statement in an explicit transaction + +Several `Delete` procedures in the codebase wrap a lone `DELETE` in an explicit transaction (e.g., +`OrganizationSponsorship_DeleteById`). These are incorrect and should not be used as a reference: + +```sql +-- Wrong -- the transaction wraps a single statement and adds nothing +CREATE PROCEDURE [dbo].[EntityName_DeleteById] + @Id UNIQUEIDENTIFIER +AS +BEGIN + SET NOCOUNT ON + + BEGIN TRANSACTION + + DELETE + FROM + [dbo].[EntityName] + WHERE + [Id] = @Id + + COMMIT TRANSACTION +END +``` + +```sql +-- Correct -- no explicit transaction needed +CREATE PROCEDURE [dbo].[EntityName_DeleteById] + @Id UNIQUEIDENTIFIER +AS +BEGIN + SET NOCOUNT ON + + DELETE + FROM + [dbo].[EntityName] + WHERE + [Id] = @Id +END +``` + +::: + +Use an explicit transaction when a procedure deletes (or otherwise modifies) rows across multiple +related tables that must be kept in sync -- e.g. deleting a parent record along with its dependent +child records: + +```sql +CREATE PROCEDURE [dbo].[EntityName_DeleteById] + @Id UNIQUEIDENTIFIER +AS +BEGIN + SET NOCOUNT ON + + BEGIN TRANSACTION + + DELETE + FROM + [dbo].[ChildEntity] + WHERE + [EntityNameId] = @Id + + DELETE + FROM + [dbo].[EntityName] + WHERE + [Id] = @Id + + COMMIT TRANSACTION +END +``` + ### Tables - **Table Name**: Singular form of the object name, PascalCase (e.g., `[dbo].[User]` not @@ -581,6 +666,17 @@ CREATE TABLE [dbo].[TableName] ); ``` +### Indexes + +- **Index Name**: `IX_{TableName}_{ColumnName(s)}` (e.g., `[IX_User_Email]`) + - The name should clearly indicate the table and the columns being indexed + +```sql +CREATE NONCLUSTERED INDEX [IX_OrganizationUser_UserIdOrganizationIdStatus] + ON [dbo].[OrganizationUser]([UserId] ASC, [OrganizationId] ASC, [Status] ASC) + INCLUDE ([AccessAll]) +``` + ### Views - **View Name**: @@ -654,7 +750,7 @@ WHERE ### User defined types New user defined types should not be created. The following existing types may be used as -table-valued parameters in stored procedures: +table-valued parameters in stored procedures for simple, scalar lists: - **`[dbo].[GuidIdArray]`** — a single-column table of `UNIQUEIDENTIFIER` values. Use when passing a list of IDs to a stored procedure (e.g., bulk reads or deletes). @@ -665,17 +761,59 @@ table-valued parameters in stored procedures: - **`[dbo].[EmailArray]`** — a single-column table of `NVARCHAR(256)` email addresses. Use when passing a list of emails to a stored procedure. -### Indexes +For anything beyond these scalar list shapes -- multi-column rows, or a shape that may need new +properties over time -- serialize the data as JSON in application code and pass it as a single +`NVARCHAR(MAX)` parameter, rather than creating a new TVP. -- **Index Name**: `IX_{TableName}_{ColumnName(s)}` (e.g., `[IX_User_Email]`) - - The name should clearly indicate the table and the columns being indexed +#### Passing structured data as JSON + +Use `OPENJSON` with an explicit `WITH` clause to shred a JSON array of objects into a typed table. +This is the preferred pattern for bulk `INSERT`/`UPDATE` operations that need more than one column +per row: ```sql -CREATE NONCLUSTERED INDEX [IX_OrganizationUser_UserIdOrganizationIdStatus] - ON [dbo].[OrganizationUser]([UserId] ASC, [OrganizationId] ASC, [Status] ASC) - INCLUDE ([AccessAll]) +CREATE PROCEDURE [dbo].[EntityName_CreateMany] + @EntityNameJson NVARCHAR(MAX) +AS +BEGIN + SET NOCOUNT ON + + INSERT INTO [dbo].[EntityName] + ( + [Id], + [Name], + [CreationDate], + [RevisionDate] + ) + SELECT + [Id], + [Name], + [CreationDate], + [RevisionDate] + FROM + OPENJSON(@EntityNameJson) + WITH ( + [Id] UNIQUEIDENTIFIER '$.Id', + [Name] NVARCHAR(256) '$.Name', + [CreationDate] DATETIME2(7) '$.CreationDate', + [RevisionDate] DATETIME2(7) '$.RevisionDate' + ) +END ``` +In application code, serialize the collection with `JsonSerializer.Serialize()` and pass the result +as the parameter value; Dapper maps it to the `NVARCHAR(MAX)` parameter like any other string. + +:::tip When to use JSON vs. an existing TVP + +- Use the existing TVPs (`GuidIdArray`, `TwoGuidIdArray`, `EmailArray`) for simple, single- or + two-column lists of scalar values. +- Use a JSON parameter when each row needs more than two columns, or when the row shape may need to + gain properties over time -- adding a property to a JSON payload doesn't require a schema change, + unlike adding a column to a TVP. + +::: + ## Error handling - Use `SET NOCOUNT ON` in stored procedures @@ -896,7 +1034,7 @@ GO #### Creating or modifying a view -We recommend using the `CREATE OR ALTER` syntax for adding or modifying a view. +Use the `CREATE OR ALTER` syntax for adding or modifying a view. ```sql CREATE OR ALTER VIEW [dbo].[{view_name}] @@ -910,7 +1048,7 @@ GO #### Deleting a view -When deleting a view, use `IF EXISTS` to avoid an error if the table doesn't exist. +When deleting a view, use `IF EXISTS` to avoid an error if the view doesn't exist. ```sql DROP IF EXISTS [dbo].[{view_name}] @@ -934,8 +1072,7 @@ GO #### Creating or modifying a function or stored procedure -We recommend using the `CREATE OR ALTER` syntax for adding or modifying a function or stored -procedure. +Use the `CREATE OR ALTER` syntax for adding or modifying a function or stored procedure. ```sql CREATE OR ALTER {PROCEDURE|FUNCTION} [dbo].[{sproc_or_func_name}] From b790da895ea76d81cc998e7c82865840dab80133 Mon Sep 17 00:00:00 2001 From: Mark Kincaid Date: Wed, 15 Jul 2026 11:02:32 -0700 Subject: [PATCH 4/8] Remove specific procedure reference from transaction warning --- docs/contributing/code-style/sql.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/contributing/code-style/sql.md b/docs/contributing/code-style/sql.md index c5bb87527..6d9f722ad 100644 --- a/docs/contributing/code-style/sql.md +++ b/docs/contributing/code-style/sql.md @@ -554,8 +554,8 @@ don't need to roll back with them. :::warning Do not wrap a single statement in an explicit transaction -Several `Delete` procedures in the codebase wrap a lone `DELETE` in an explicit transaction (e.g., -`OrganizationSponsorship_DeleteById`). These are incorrect and should not be used as a reference: +Several `Delete` procedures in the codebase wrap a lone `DELETE` in an explicit transaction. These +should not be used as a reference: ```sql -- Wrong -- the transaction wraps a single statement and adds nothing From 203fa008b1500fea5dfebccef9ac0db5fa572baa Mon Sep 17 00:00:00 2001 From: Mark Kincaid Date: Wed, 22 Jul 2026 16:22:56 -0700 Subject: [PATCH 5/8] Add section regarding check constraints --- docs/contributing/code-style/sql.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/contributing/code-style/sql.md b/docs/contributing/code-style/sql.md index 6d9f722ad..e5846c2a5 100644 --- a/docs/contributing/code-style/sql.md +++ b/docs/contributing/code-style/sql.md @@ -633,6 +633,14 @@ END - **Foreign Keys**: `FK_{TableName}_{ReferencedTable}` (e.g., FK_Device_User) - **Default Constraints**: `DF_{TableName}_{ColumnName}` (e.g., [DF_Organization_UseScim]) +:::warning Do not create `CHECK` constraints + +Only primary keys, foreign keys, unique constraints, and default constraints are permitted on +tables. Do not create `CHECK` constraints -- they encode business logic in the database, which goes +against our policy of keeping business logic in the application layer. + +::: + #### Column definitions - **Alignment**: Column names, data types, and nullability vertically aligned using spaces From 842d643cc693af57ece6aca485c990df702603cf Mon Sep 17 00:00:00 2001 From: Mark Kincaid Date: Thu, 23 Jul 2026 08:34:12 -0700 Subject: [PATCH 6/8] Revert "Add section regarding check constraints" This reverts commit 203fa008b1500fea5dfebccef9ac0db5fa572baa. --- docs/contributing/code-style/sql.md | 8 -------- 1 file changed, 8 deletions(-) diff --git a/docs/contributing/code-style/sql.md b/docs/contributing/code-style/sql.md index e5846c2a5..6d9f722ad 100644 --- a/docs/contributing/code-style/sql.md +++ b/docs/contributing/code-style/sql.md @@ -633,14 +633,6 @@ END - **Foreign Keys**: `FK_{TableName}_{ReferencedTable}` (e.g., FK_Device_User) - **Default Constraints**: `DF_{TableName}_{ColumnName}` (e.g., [DF_Organization_UseScim]) -:::warning Do not create `CHECK` constraints - -Only primary keys, foreign keys, unique constraints, and default constraints are permitted on -tables. Do not create `CHECK` constraints -- they encode business logic in the database, which goes -against our policy of keeping business logic in the application layer. - -::: - #### Column definitions - **Alignment**: Column names, data types, and nullability vertically aligned using spaces From fefcf566d87bc265970a5c8cdcd5bee2d150a91c Mon Sep 17 00:00:00 2001 From: Mark Kincaid Date: Mon, 10 Aug 2026 16:24:51 -0700 Subject: [PATCH 7/8] Updated file based on recommendations --- docs/contributing/code-style/sql.md | 147 ++++++++++++++++++---------- 1 file changed, 94 insertions(+), 53 deletions(-) diff --git a/docs/contributing/code-style/sql.md b/docs/contributing/code-style/sql.md index 2e3619e22..75d9b51df 100644 --- a/docs/contributing/code-style/sql.md +++ b/docs/contributing/code-style/sql.md @@ -43,7 +43,8 @@ This separation of concerns means: - `Stored Procedures/` - General stored procedures - `Tables/` - Core tables - `Views/` - General views - - `User Defined Types/` - Custom data types + - `User Defined Types/` - Custom data types (no new types should be added; see + [User defined types](#user-defined-types)) ### File naming conventions @@ -85,7 +86,8 @@ These standards should be applied across any T-SQL scripts that you write. - **Data type modifiers**: Omit the space between type name and opening parenthesis (e.g., `NVARCHAR(50)` not `NVARCHAR (50)`, `DATETIME2(7)` not `DATETIME2 (7)`) - **Naming**: Use full, unabbreviated names throughout — object names, column names, parameters, and - descriptors (e.g., `OrganizationId` not `OrgId`) + descriptors (e.g., `OrganizationId` not `OrgId`). Conventional short forms are exempt (`Id`, the + `IX_`/`PK_`/`FK_` prefixes, short-lived local variables like `@UtcNow`). - **ID generation**: Use `CoreHelpers.GenerateComb()` in application code, not `NEWID()` in the database -- see [GUID generation](./csharp#guid-generation) - **Datetime generation**: Generate datetime values in application code and pass them as parameters @@ -229,7 +231,7 @@ WHERE - Put `UNION ALL` on its own line, with a blank line above and below it ```sql -;WITH OrgUsers AS +;WITH OrganizationUsers AS ( -- Active users: direct UserId match SELECT @@ -269,7 +271,7 @@ SELECT OU.[OrganizationId], CASE WHEN PR.[OrganizationId] IS NULL THEN 0 ELSE 1 END AS [IsProvider] FROM - OrgUsers OU + OrganizationUsers OU LEFT JOIN Providers PR ON PR.[OrganizationId] = OU.[OrganizationId] ``` @@ -283,13 +285,16 @@ Stored procedures follow the `{EntityName}_{Action}` format (e.g., `[dbo].[User_ - **EntityName**: The main table or concept the procedure operates on (e.g., `User`, `Organization`, `Cipher`) - **Action**: A verb from the standard list below, optionally followed by a short descriptor that - clarifies what the procedure does + clarifies what the procedure does. `Read` and `ReadMany` are almost always paired with a + descriptor (e.g., `ReadById`, `ReadByOrganizationId`) since a bare `Read`/`ReadMany` rarely + conveys which record(s) are selected. **Standard action verbs** | Verb | Description | | ------------ | ----------------------- | | `Create` | Insert a new record | +| `CreateMany` | Insert multiple records | | `Read` | Select a single record | | `ReadMany` | Select multiple records | | `Update` | Modify a record | @@ -297,13 +302,16 @@ Stored procedures follow the `{EntityName}_{Action}` format (e.g., `[dbo].[User_ | `Delete` | Remove a record | | `DeleteMany` | Remove multiple records | -:::tip When an operation is more specific than a standard verb alone, append a short descriptor: +:::tip Appending a descriptor + +When an operation is more specific than a standard verb alone, append a short descriptor to clarify +what it does: - `User_ReadById` — read filtered by a specific field -- `User_ReadManyByOrganizationId` — filtered bulk read -- `OrganizationUser_UpdateStatus` — update a specific field +- `OrganizationIntegration_ReadManyByOrganizationId` — filtered bulk read +- `User_UpdateRenewalReminderDate` — update a specific field - `OrganizationUser_UpdateManyRevoke` — bulk revoke -- `User_UpdateApplicationData` — update a named subset of fields +- `OrganizationReport_UpdateApplicationData` — update a named subset of fields ::: @@ -422,6 +430,9 @@ BEGIN END ``` +`[Role]` is a placeholder column name for this generic example; the real codebase column for this +pattern is `[Type]`. + #### Parameter declaration - One parameter per line @@ -429,7 +440,7 @@ END - Default values on same line as parameter - `OUTPUT` parameters clearly marked - Pass values in as parameters from application code rather than hard-coding them or generating them - with a SQL function inside the procedure (e.g., `GETUTCDATE()`) -- see + with a SQL function inside the procedure (e.g., `GETUTCDATE()`) — see [accepted exceptions](#datetime-values) for cases where a SQL function is appropriate :::warning Default parameter values @@ -472,12 +483,14 @@ There are specific patterns in the codebase where using a SQL datetime function correct: 1. **Account revision date bumping** — The `User_BumpAccountRevisionDate*` family of procedures - exist solely to stamp `[AccountRevisionDate]` to the current UTC time atomically, without a + exists solely to stamp `[AccountRevisionDate]` to the current UTC time atomically, without a round-trip to application code. -2. **Bulk operations requiring a consistent timestamp** — When a procedure must apply the same - timestamp across multiple rows or statements in one transaction, declare a local variable once - and reuse it throughout the procedure: +2. **Bulk operations spanning multiple statements** — SQL Server evaluates `GETUTCDATE()`/ + `SYSUTCDATETIME()` once per statement, so a single statement never needs this exception — every + row it touches already gets the same value. When a procedure executes multiple statements that + must share the exact same timestamp, declare a local variable once and reuse it throughout the + procedure: ```sql DECLARE @UtcNow DATETIME2(7) = SYSUTCDATETIME(); @@ -489,21 +502,28 @@ correct: [RevisionDate] = @UtcNow WHERE [Id] IN (SELECT [Id] FROM @Ids) + + UPDATE + [dbo].[Folder] + SET + [RevisionDate] = @UtcNow + WHERE + [UserId] = @UserId ``` 3. **`WHERE` clause predicates** — Comparing against the current time in a filter is acceptable - (e.g., deleting expired records, skipping rows whose date has not changed since yesterday): + (e.g., deleting expired records): ```sql WHERE - [ExpirationDate] < GETUTCDATE() + [ExpirationDate] < SYSUTCDATETIME() ``` 4. **Nullable parameter fallbacks** — When a parameter is intentionally nullable and the procedure should default to the current time when no value is supplied: ```sql - SET @Created = COALESCE(@Created, GETUTCDATE()) + SET @Created = COALESCE(@Created, SYSUTCDATETIME()) ``` #### `INSERT` statements @@ -546,11 +566,11 @@ WHERE Only wrap statements in an explicit `BEGIN TRANSACTION` / `COMMIT TRANSACTION` when a procedure performs multiple statements that must all succeed or all fail together. A single `INSERT`, -`UPDATE`, or `DELETE` statement is already atomic on its own -- SQL Server implicitly wraps every -individual statement in a transaction, so adding an explicit one around it adds nothing except -unnecessary lock hold time and noise. When a transaction is needed, keep its scope as small as -possible -- only the statements that need to be atomic, not unrelated reads or `EXEC` calls that -don't need to roll back with them. +`UPDATE`, or `DELETE` statement is already atomic on its own — SQL Server implicitly wraps every +individual statement in a transaction, so adding an explicit one around it adds nothing but noise +and the risk of an orphaned open transaction if the statement errors before `COMMIT` is reached. +When a transaction is needed, keep its scope as small as possible — only the statements that need to +be atomic, not unrelated reads or `EXEC` calls that don't need to roll back with them. :::warning Do not wrap a single statement in an explicit transaction @@ -558,7 +578,7 @@ Several `Delete` procedures in the codebase wrap a lone `DELETE` in an explicit should not be used as a reference: ```sql --- Wrong -- the transaction wraps a single statement and adds nothing +-- Wrong: the transaction wraps a single statement and adds nothing CREATE PROCEDURE [dbo].[EntityName_DeleteById] @Id UNIQUEIDENTIFIER AS @@ -578,7 +598,7 @@ END ``` ```sql --- Correct -- no explicit transaction needed +-- Correct: no explicit transaction needed CREATE PROCEDURE [dbo].[EntityName_DeleteById] @Id UNIQUEIDENTIFIER AS @@ -596,8 +616,10 @@ END ::: Use an explicit transaction when a procedure deletes (or otherwise modifies) rows across multiple -related tables that must be kept in sync -- e.g. deleting a parent record along with its dependent -child records: +related tables that must be kept in sync — e.g. deleting a parent record along with its dependent +child records. Set `XACT_ABORT ON` and wrap the transaction in `TRY`/`CATCH` (see +[Error handling](#error-handling)) so a mid-transaction error rolls back everything instead of +committing a partial change: ```sql CREATE PROCEDURE [dbo].[EntityName_DeleteById] @@ -605,22 +627,31 @@ CREATE PROCEDURE [dbo].[EntityName_DeleteById] AS BEGIN SET NOCOUNT ON + SET XACT_ABORT ON - BEGIN TRANSACTION + BEGIN TRY + BEGIN TRANSACTION - DELETE - FROM - [dbo].[ChildEntity] - WHERE - [EntityNameId] = @Id + DELETE + FROM + [dbo].[ChildEntity] + WHERE + [EntityNameId] = @Id - DELETE - FROM - [dbo].[EntityName] - WHERE - [Id] = @Id + DELETE + FROM + [dbo].[EntityName] + WHERE + [Id] = @Id - COMMIT TRANSACTION + COMMIT TRANSACTION + END TRY + BEGIN CATCH + IF @@TRANCOUNT > 0 + ROLLBACK TRANSACTION + + THROW + END CATCH END ``` @@ -672,9 +703,8 @@ CREATE TABLE [dbo].[TableName] - The name should clearly indicate the table and the columns being indexed ```sql -CREATE NONCLUSTERED INDEX [IX_OrganizationUser_UserIdOrganizationIdStatus] - ON [dbo].[OrganizationUser]([UserId] ASC, [OrganizationId] ASC, [Status] ASC) - INCLUDE ([AccessAll]) +CREATE NONCLUSTERED INDEX [IX_OrganizationUser_UserIdOrganizationIdStatusV2] + ON [dbo].[OrganizationUser]([UserId] ASC, [OrganizationId] ASC, [Status] ASC) ``` ### Views @@ -761,8 +791,12 @@ table-valued parameters in stored procedures for simple, scalar lists: - **`[dbo].[EmailArray]`** — a single-column table of `NVARCHAR(256)` email addresses. Use when passing a list of emails to a stored procedure. -For anything beyond these scalar list shapes -- multi-column rows, or a shape that may need new -properties over time -- serialize the data as JSON in application code and pass it as a single +A small number of multi-column TVPs predate this guidance and remain in active use (e.g. +`[dbo].[CollectionAccessSelectionType]`, `[dbo].[OrganizationSponsorshipType]`) — continue using +them in procedures that already depend on them, but don't follow them as a pattern for new work. + +For anything beyond the scalar list shapes above — multi-column rows, or a shape that may need new +properties over time — serialize the data as JSON in application code and pass it as a single `NVARCHAR(MAX)` parameter, rather than creating a new TVP. #### Passing structured data as JSON @@ -779,8 +813,7 @@ BEGIN SET NOCOUNT ON INSERT INTO [dbo].[EntityName] - ( - [Id], + ( [Id], [Name], [CreationDate], [RevisionDate] @@ -793,7 +826,7 @@ BEGIN FROM OPENJSON(@EntityNameJson) WITH ( - [Id] UNIQUEIDENTIFIER '$.Id', + [Id] UNIQUEIDENTIFIER '$.Id', [Name] NVARCHAR(256) '$.Name', [CreationDate] DATETIME2(7) '$.CreationDate', [RevisionDate] DATETIME2(7) '$.RevisionDate' @@ -804,12 +837,21 @@ END In application code, serialize the collection with `JsonSerializer.Serialize()` and pass the result as the parameter value; Dapper maps it to the `NVARCHAR(MAX)` parameter like any other string. +:::warning `OPENJSON` paths are lax and case-sensitive by default + +`WITH` clause paths are lax unless prefixed with `strict`: a missing or misspelled property yields +`NULL` instead of an error. JSON property names are also case-sensitive, so a camelCase +serialization policy will silently break a path like `'$.Id'`. Use `strict $.Id` for columns that +must always be present, and confirm the application's JSON casing matches the paths used here. + +::: + :::tip When to use JSON vs. an existing TVP - Use the existing TVPs (`GuidIdArray`, `TwoGuidIdArray`, `EmailArray`) for simple, single- or two-column lists of scalar values. - Use a JSON parameter when each row needs more than two columns, or when the row shape may need to - gain properties over time -- adding a property to a JSON payload doesn't require a schema change, + gain properties over time — adding a property to a JSON payload doesn't require a schema change, unlike adding a column to a TVP. ::: @@ -914,7 +956,7 @@ GO When deleting a table, use `IF EXISTS` to avoid an error if the table doesn't exist. ```sql -DROP IF EXISTS [dbo].[{table_name}] +DROP TABLE IF EXISTS [dbo].[{table_name}] GO ``` @@ -1099,7 +1141,7 @@ GO When deleting a view, use `IF EXISTS` to avoid an error if the view doesn't exist. ```sql -DROP IF EXISTS [dbo].[{view_name}] +DROP VIEW IF EXISTS [dbo].[{view_name}] GO ``` @@ -1133,7 +1175,7 @@ GO When deleting a function or stored procedure, use `IF EXISTS` to avoid an error if it doesn't exist. ```sql -DROP IF EXISTS [dbo].[{sproc_or_func_name}] +DROP {PROCEDURE|FUNCTION} IF EXISTS [dbo].[{sproc_or_func_name}] GO ``` @@ -1152,9 +1194,8 @@ heavy-read tables and the locks can cause exceptionally high CPU, wait times and in Azure SQL. ```sql -CREATE NONCLUSTERED INDEX [IX_OrganizationUser_UserIdOrganizationIdStatus] - ON [dbo].[OrganizationUser]([UserId] ASC, [OrganizationId] ASC, [Status] ASC) - INCLUDE ([AccessAll]) +CREATE NONCLUSTERED INDEX [IX_OrganizationUser_UserIdOrganizationIdStatusV2] + ON [dbo].[OrganizationUser]([UserId] ASC, [OrganizationId] ASC, [Status] ASC) ``` #### Modifying Existing Indexes From 34070c559a768c68ed96c600e43eb35c6bdb4af7 Mon Sep 17 00:00:00 2001 From: Mark Kincaid Date: Wed, 12 Aug 2026 16:35:13 -0700 Subject: [PATCH 8/8] Updated PR feedback. --- docs/contributing/code-style/sql.md | 157 ++++++++-------------------- 1 file changed, 43 insertions(+), 114 deletions(-) diff --git a/docs/contributing/code-style/sql.md b/docs/contributing/code-style/sql.md index 75d9b51df..e9322ba8d 100644 --- a/docs/contributing/code-style/sql.md +++ b/docs/contributing/code-style/sql.md @@ -85,15 +85,14 @@ These standards should be applied across any T-SQL scripts that you write. - **Parentheses**: Parentheses should be vertically aligned when spanning multiple lines - **Data type modifiers**: Omit the space between type name and opening parenthesis (e.g., `NVARCHAR(50)` not `NVARCHAR (50)`, `DATETIME2(7)` not `DATETIME2 (7)`) -- **Naming**: Use full, unabbreviated names throughout — object names, column names, parameters, and - descriptors (e.g., `OrganizationId` not `OrgId`). Conventional short forms are exempt (`Id`, the - `IX_`/`PK_`/`FK_` prefixes, short-lived local variables like `@UtcNow`). +- **Naming**: Use full, unabbreviated names throughout -- object names, column names, parameters, + and descriptors (e.g., `OrganizationId` not `OrgId`). Conventional short forms are exempt (`Id`, + the `IX_`/`PK_`/`FK_` prefixes, short-lived local variables like `@UtcNow`). - **ID generation**: Use `CoreHelpers.GenerateComb()` in application code, not `NEWID()` in the database -- see [GUID generation](./csharp#guid-generation) - **Datetime generation**: Generate datetime values in application code and pass them as parameters - (e.g., `@RevisionDate`), not using SQL functions (`SYSUTCDATETIME()`, `GETUTCDATE()`) — this keeps - timestamp generation consistent and testable. See the [accepted exceptions](#datetime-values) - documented in the stored procedures section. + (e.g., `@RevisionDate`) instead of computing them in SQL (`SYSUTCDATETIME()`, `GETUTCDATE()`) -- + see [Datetime values](#datetime-values) for why. ### `SELECT` statements @@ -163,7 +162,7 @@ WHERE ``` - For bulk operations where the recordset may be large (e.g., all users in an organization), prefer - an `INNER JOIN` on the TVP — this gives the query optimizer full flexibility to choose an + an `INNER JOIN` on the TVP -- this gives the query optimizer full flexibility to choose an efficient join strategy (hash join, merge join) rather than defaulting to nested loops, which is the typical result of an `IN` subquery: @@ -307,11 +306,11 @@ Stored procedures follow the `{EntityName}_{Action}` format (e.g., `[dbo].[User_ When an operation is more specific than a standard verb alone, append a short descriptor to clarify what it does: -- `User_ReadById` — read filtered by a specific field -- `OrganizationIntegration_ReadManyByOrganizationId` — filtered bulk read -- `User_UpdateRenewalReminderDate` — update a specific field -- `OrganizationUser_UpdateManyRevoke` — bulk revoke -- `OrganizationReport_UpdateApplicationData` — update a named subset of fields +- `User_ReadById` -- read filtered by a specific field +- `OrganizationIntegration_ReadManyByOrganizationId` -- filtered bulk read +- `User_UpdateRenewalReminderDate` -- update a specific field +- `OrganizationUser_UpdateManyRevoke` -- bulk revoke +- `OrganizationReport_UpdateApplicationData` -- update a named subset of fields ::: @@ -430,9 +429,6 @@ BEGIN END ``` -`[Role]` is a placeholder column name for this generic example; the real codebase column for this -pattern is `[Type]`. - #### Parameter declaration - One parameter per line @@ -440,8 +436,7 @@ pattern is `[Type]`. - Default values on same line as parameter - `OUTPUT` parameters clearly marked - Pass values in as parameters from application code rather than hard-coding them or generating them - with a SQL function inside the procedure (e.g., `GETUTCDATE()`) — see - [accepted exceptions](#datetime-values) for cases where a SQL function is appropriate + with a SQL function inside the procedure (e.g., `GETUTCDATE()`) :::warning Default parameter values @@ -456,8 +451,11 @@ these messages differently. #### Datetime values -As noted in the general standards, datetime values must be generated in application code and passed -as parameters. Do **not** use `SYSUTCDATETIME()` or `GETUTCDATE()` inline: +Datetime values must be generated in application code and passed as parameters, not computed inline +with `SYSUTCDATETIME()` or `GETUTCDATE()`. Deciding what "now" is belongs to application logic, not +the database. Generating the value once also keeps it atomic for the whole operation -- every row or +table the operation touches gets the same timestamp, instead of each statement computing its own +slightly different one: ```sql -- Wrong @@ -477,55 +475,6 @@ WHERE [Id] = @Id ``` -**Accepted exceptions** - -There are specific patterns in the codebase where using a SQL datetime function is intentional and -correct: - -1. **Account revision date bumping** — The `User_BumpAccountRevisionDate*` family of procedures - exists solely to stamp `[AccountRevisionDate]` to the current UTC time atomically, without a - round-trip to application code. - -2. **Bulk operations spanning multiple statements** — SQL Server evaluates `GETUTCDATE()`/ - `SYSUTCDATETIME()` once per statement, so a single statement never needs this exception — every - row it touches already gets the same value. When a procedure executes multiple statements that - must share the exact same timestamp, declare a local variable once and reuse it throughout the - procedure: - - ```sql - DECLARE @UtcNow DATETIME2(7) = SYSUTCDATETIME(); - - UPDATE - [dbo].[Cipher] - SET - [DeletedDate] = @UtcNow, - [RevisionDate] = @UtcNow - WHERE - [Id] IN (SELECT [Id] FROM @Ids) - - UPDATE - [dbo].[Folder] - SET - [RevisionDate] = @UtcNow - WHERE - [UserId] = @UserId - ``` - -3. **`WHERE` clause predicates** — Comparing against the current time in a filter is acceptable - (e.g., deleting expired records): - - ```sql - WHERE - [ExpirationDate] < SYSUTCDATETIME() - ``` - -4. **Nullable parameter fallbacks** — When a parameter is intentionally nullable and the procedure - should default to the current time when no value is supplied: - - ```sql - SET @Created = COALESCE(@Created, SYSUTCDATETIME()) - ``` - #### `INSERT` statements - Column list in parentheses, one column per line @@ -566,11 +515,11 @@ WHERE Only wrap statements in an explicit `BEGIN TRANSACTION` / `COMMIT TRANSACTION` when a procedure performs multiple statements that must all succeed or all fail together. A single `INSERT`, -`UPDATE`, or `DELETE` statement is already atomic on its own — SQL Server implicitly wraps every +`UPDATE`, or `DELETE` statement is already atomic on its own -- SQL Server implicitly wraps every individual statement in a transaction, so adding an explicit one around it adds nothing but noise and the risk of an orphaned open transaction if the statement errors before `COMMIT` is reached. -When a transaction is needed, keep its scope as small as possible — only the statements that need to -be atomic, not unrelated reads or `EXEC` calls that don't need to roll back with them. +When a transaction is needed, keep its scope as small as possible -- only the statements that need +to be atomic, not unrelated reads or `EXEC` calls that don't need to roll back with them. :::warning Do not wrap a single statement in an explicit transaction @@ -578,45 +527,29 @@ Several `Delete` procedures in the codebase wrap a lone `DELETE` in an explicit should not be used as a reference: ```sql --- Wrong: the transaction wraps a single statement and adds nothing -CREATE PROCEDURE [dbo].[EntityName_DeleteById] - @Id UNIQUEIDENTIFIER -AS -BEGIN - SET NOCOUNT ON - - BEGIN TRANSACTION - - DELETE - FROM - [dbo].[EntityName] - WHERE - [Id] = @Id +-- Wrong +BEGIN TRANSACTION - COMMIT TRANSACTION -END -``` +DELETE +FROM + [dbo].[EntityName] +WHERE + [Id] = @Id -```sql --- Correct: no explicit transaction needed -CREATE PROCEDURE [dbo].[EntityName_DeleteById] - @Id UNIQUEIDENTIFIER -AS -BEGIN - SET NOCOUNT ON +COMMIT TRANSACTION - DELETE - FROM - [dbo].[EntityName] - WHERE - [Id] = @Id -END +-- Correct +DELETE +FROM + [dbo].[EntityName] +WHERE + [Id] = @Id ``` ::: Use an explicit transaction when a procedure deletes (or otherwise modifies) rows across multiple -related tables that must be kept in sync — e.g. deleting a parent record along with its dependent +related tables that must be kept in sync -- e.g. deleting a parent record along with its dependent child records. Set `XACT_ABORT ON` and wrap the transaction in `TRY`/`CATCH` (see [Error handling](#error-handling)) so a mid-transaction error rolls back everything instead of committing a partial change: @@ -678,7 +611,7 @@ END `NVARCHAR(50)` not `NVARCHAR (50)`, `DATETIME2(7)` not `DATETIME2 (7)`) - **Nullability**: Explicitly specify `NOT NULL` or `NULL` - **Datetime column naming**: Datetime columns must end with `Date` (e.g., `CreationDate`, - `RevisionDate`, `ExpirationDate`) — do not use `At` suffixes (e.g., `CreatedAt`, `UpdatedAt`) + `RevisionDate`, `ExpirationDate`) -- do not use `At` suffixes (e.g., `CreatedAt`, `UpdatedAt`) - **Standard Columns**: Most tables include: - `[Id] UNIQUEIDENTIFIER NOT NULL` - Primary key - `[CreationDate] DATETIME2(7) NOT NULL` - Record creation timestamp @@ -782,21 +715,17 @@ WHERE New user defined types should not be created. The following existing types may be used as table-valued parameters in stored procedures for simple, scalar lists: -- **`[dbo].[GuidIdArray]`** — a single-column table of `UNIQUEIDENTIFIER` values. Use when passing a - list of IDs to a stored procedure (e.g., bulk reads or deletes). +- **`[dbo].[GuidIdArray]`** -- a single-column table of `UNIQUEIDENTIFIER` values. Use when passing + a list of IDs to a stored procedure (e.g., bulk reads or deletes). -- **`[dbo].[TwoGuidIdArray]`** — a two-column table of `UNIQUEIDENTIFIER` pairs (`Id1`, `Id2`). Use +- **`[dbo].[TwoGuidIdArray]`** -- a two-column table of `UNIQUEIDENTIFIER` pairs (`Id1`, `Id2`). Use when an operation requires two related IDs per row (e.g., user ID + organization ID). -- **`[dbo].[EmailArray]`** — a single-column table of `NVARCHAR(256)` email addresses. Use when +- **`[dbo].[EmailArray]`** -- a single-column table of `NVARCHAR(256)` email addresses. Use when passing a list of emails to a stored procedure. -A small number of multi-column TVPs predate this guidance and remain in active use (e.g. -`[dbo].[CollectionAccessSelectionType]`, `[dbo].[OrganizationSponsorshipType]`) — continue using -them in procedures that already depend on them, but don't follow them as a pattern for new work. - -For anything beyond the scalar list shapes above — multi-column rows, or a shape that may need new -properties over time — serialize the data as JSON in application code and pass it as a single +For anything beyond the scalar list shapes above -- multi-column rows, or a shape that may need new +properties over time -- serialize the data as JSON in application code and pass it as a single `NVARCHAR(MAX)` parameter, rather than creating a new TVP. #### Passing structured data as JSON @@ -851,7 +780,7 @@ must always be present, and confirm the application's JSON casing matches the pa - Use the existing TVPs (`GuidIdArray`, `TwoGuidIdArray`, `EmailArray`) for simple, single- or two-column lists of scalar values. - Use a JSON parameter when each row needs more than two columns, or when the row shape may need to - gain properties over time — adding a property to a JSON payload doesn't require a schema change, + gain properties over time -- adding a property to a JSON payload doesn't require a schema change, unlike adding a column to a TVP. :::