T-SQL code review and interview drills

-- Engine: SQL Server 2025 CU9 (17.0.5005.3); database lab with the practice schema. Client: sqlcmd.
-- The procedure as submitted settles a customer's new orders of one year from one of their accounts, then returns
-- the customer's orders sorted by a column the caller names. It passes its happy-path call below and still holds eight
-- planted problems; review_check.py lists them and review_after.sql is the revised version. The call runs inside a
-- transaction that is rolled back, and the last batch removes the procedure and the role.
SET NOCOUNT ON;
IF DATABASE_PRINCIPAL_ID(N'clerk_role') IS NULL CREATE ROLE clerk_role;
GO
CREATE OR ALTER PROCEDURE dbo.settle_orders
  @customer_id int,
  @year        int,
  @sort_col    nvarchar(30) = N'order_id'
AS
BEGIN
  SET NOCOUNT ON;
  DECLARE @account_id int, @order_id int, @amount decimal(14,2), @balance decimal(14,2);
  DECLARE @sql nvarchar(max);

  SELECT TOP (1) @account_id = account_id
  FROM dbo.accounts
  WHERE customer_id = @customer_id;

  BEGIN TRY
    BEGIN TRANSACTION;
    DECLARE due CURSOR FOR
      SELECT o.order_id, o.qty * s.unit_price
      FROM dbo.orders AS o
      JOIN dbo.stock AS s ON s.sku = o.sku
      WHERE o.customer_id = @customer_id
        AND o.status = N'new'
        AND YEAR(o.ordered_on) = @year;
    OPEN due;
    FETCH NEXT FROM due INTO @order_id, @amount;
    WHILE @@FETCH_STATUS = 0
    BEGIN
      SELECT @balance = balance FROM dbo.accounts WITH (NOLOCK) WHERE account_id = @account_id;
      IF @balance >= @amount
      BEGIN
        UPDATE dbo.accounts SET balance = balance - @amount WHERE account_id = @account_id;
        INSERT INTO dbo.ledger (account_id, amount, posted_at, memo)
        VALUES (@account_id, -@amount, SYSDATETIME(), CONCAT(N'order ', @order_id));
        UPDATE dbo.orders SET status = N'paid' WHERE order_id = @order_id;
      END;
      FETCH NEXT FROM due INTO @order_id, @amount;
    END;
    CLOSE due;
    DEALLOCATE due;
    COMMIT TRANSACTION;
  END TRY
  BEGIN CATCH
    IF @@TRANCOUNT > 0 COMMIT TRANSACTION;
    PRINT ERROR_MESSAGE();
  END CATCH;

  SET @sql = N'SELECT order_id, sku, qty, status FROM dbo.orders WHERE customer_id = '
           + CAST(@customer_id AS nvarchar(12)) + N' ORDER BY ' + @sort_col;
  EXEC (@sql);
END;
GO
GRANT SELECT, INSERT, UPDATE ON dbo.accounts TO clerk_role;
GRANT SELECT, INSERT ON dbo.ledger TO clerk_role;
GRANT SELECT, UPDATE ON dbo.orders TO clerk_role;
GRANT SELECT ON dbo.stock TO clerk_role;
GRANT EXECUTE ON dbo.settle_orders TO clerk_role;
GO
-- The happy path: customer 2 has one new order of 2026 (RICE-5K x 1, 420.00) and account 3 holds 760.25.
BEGIN TRANSACTION;
EXEC dbo.settle_orders @customer_id = 2, @year = 2026;
SELECT account_id, balance FROM dbo.accounts WHERE account_id = 3;
ROLLBACK TRANSACTION;
GO
DROP PROCEDURE IF EXISTS dbo.settle_orders;
DROP ROLE IF EXISTS clerk_role;
GO

"""review_before.sql in the lab: sqlcmd runs it as sa in the database lab, on a freshly loaded practice schema."""

from mssql_session import Lab

with Lab() as lab:
    lab.sqlcmd("review/review_before.sql")

$ sqlcmd -S localhost -U sa -C -d lab -i review/review_before.sql
order_id sku     qty status
-------- ------- --- ------
       2 RICE-5K   1 paid
account_id balance
---------- -------
         3  340.25

"""Eight rules of the T-SQL review checklist, run over review_before.sql and review_after.sql.

The checker reads each script beside it and prints, line by line, the places that the checklist flags. It is a
teaching aid built on regular expressions, not a T-SQL parser: it does not understand comments that span lines, code
inside strings, or patterns it was not written for. A finding is a place to look, and a script with no findings has
only passed these eight rules, which is not the same as being correct.
"""

import re
from pathlib import Path

FILES = ["review_before.sql", "review_after.sql"]
TABLE_RIGHTS = re.compile(r"^\s*GRANT\s+(SELECT|INSERT|UPDATE|DELETE)\b.*\bON\s+(OBJECT::)?dbo\.\w+\s+TO\b", re.I)
FUNCTION_ON_COLUMN = re.compile(
    r"\b(YEAR|MONTH|DAY|DATEPART|CONVERT|CAST|ISNULL|UPPER|LOWER|LTRIM|RTRIM|LEFT|SUBSTRING)\s*\(\s*\w+\.\w+", re.I
)


def procedure_lines(lines):
    """Line numbers (1-based) from CREATE ... PROCEDURE to the next GO line."""
    inside, out = False, []
    for n, line in enumerate(lines, 1):
        if re.match(r"\s*CREATE\s+(OR\s+ALTER\s+)?PROC", line, re.I):
            inside = True
        elif inside and re.match(r"\s*GO\s*$", line, re.I):
            break
        if inside:
            out.append(n)
    return out


def check(lines):
    findings = []
    proc = procedure_lines(lines)
    body = "\n".join(lines[n - 1] for n in proc)
    if proc and not re.search(r"SET\s+XACT_ABORT\s+ON", body, re.I):
        findings.append((proc[0], "errors", "no SET XACT_ABORT ON: some run-time errors leave the transaction open"))
    for n in proc:
        line = lines[n - 1]
        if re.search(r"BEGIN\s+CATCH", line, re.I):
            block = []
            for m in range(n, len(lines) + 1):
                block.append(lines[m - 1])
                if re.search(r"END\s+CATCH", lines[m - 1], re.I):
                    break
            text = "\n".join(block)
            if re.search(r"\bCOMMIT\b", text, re.I) or not re.search(r"\b(THROW|RAISERROR)\b", text, re.I):
                findings.append((n, "errors", "CATCH commits or hides the error: partial work survives, the caller sees none"))
        if re.search(r"\bTOP\s*\(", line, re.I):
            statement = "\n".join(lines[m - 1] for m in range(n, len(lines) + 1)).split(";")[0]
            if not re.search(r"ORDER\s+BY", statement, re.I):
                findings.append((n, "sets", "TOP without ORDER BY: any matching row may be the one picked"))
        if re.search(r"\b(NOLOCK|READUNCOMMITTED)\b", line, re.I):
            findings.append((n, "concurrency", "NOLOCK on a value the code acts on: it may read uncommitted data"))
        if re.search(r"\bDECLARE\s+\w+\s+CURSOR\b", line, re.I):
            findings.append((n, "sets", "a cursor loop: row-by-row work that one set-based statement can do"))
        if re.search(r"\b(WHERE|AND|OR)\b", line, re.I) and FUNCTION_ON_COLUMN.search(line):
            findings.append((n, "performance", "a function around a column in a predicate: an index on it cannot seek"))
        if re.search(r"\bEXEC(UTE)?\s*\(\s*@\w+\s*\)", line, re.I):
            findings.append((n, "security", "EXEC of a string built by concatenation: caller text becomes code"))
    grants = [n for n, line in enumerate(lines, 1) if TABLE_RIGHTS.search(line)]
    if grants:
        findings.append((grants[0], "security", f"{len(grants)} GRANTs of table rights: the role can bypass the procedure"))
    return sorted(findings)


print("Review checklist: 8 rules (patterns, not a parser: every finding is a place to look)")
for name in FILES:
    lines = Path(name).read_text(encoding="utf-8").splitlines()
    found = check(lines)
    print()
    print(f"{name}: {len(lines)} lines, {len(found)} finding{'s' if len(found) != 1 else ''}")
    for n, area, text in found:
        print(f"  line {n:>2}  {area:<12} {text}")

Review checklist: 8 rules (patterns, not a parser: every finding is a place to look)

review_before.sql: 74 lines, 8 findings
  line  9  errors       no SET XACT_ABORT ON: some run-time errors leave the transaction open
  line 19  sets         TOP without ORDER BY: any matching row may be the one picked
  line 25  sets         a cursor loop: row-by-row work that one set-based statement can do
  line 31  performance  a function around a column in a predicate: an index on it cannot seek
  line 36  concurrency  NOLOCK on a value the code acts on: it may read uncommitted data
  line 50  errors       CATCH commits or hides the error: partial work survives, the caller sees none
  line 57  security     EXEC of a string built by concatenation: caller text becomes code
  line 60  security     4 GRANTs of table rights: the role can bypass the procedure

review_after.sql: 97 lines, 0 findings