PL/SQL code review and interview drills

-- Engine: Oracle AI Database 26ai Free, Release 23.26.3.0.0; user LAB in FREEPDB1. Client: SQL*Plus or SQLcl.
-- The package ledger_post as it arrives for a code review. post_batch moves one feed batch from feed_lines into the
-- ledger and adds each line to its account's balance; lines_posted reports how many lines were posted; open_report
-- opens a batch's ledger entries in the order a caller asks for; day_total adds up one day's entries. It compiles,
-- and the author's test at the end succeeds. Run schema_oracle.sql of practice-schema-and-dialect-basics first.
-- post_batch commits on its own, so the cleanup at the end deletes what the test posted and drops what the script made.
SET SERVEROUTPUT ON

DROP TABLE IF EXISTS feed_lines PURGE;
DROP INDEX IF EXISTS ledger_posted_ix;

CREATE TABLE feed_lines (
  batch_id   VARCHAR2(20)  NOT NULL,
  line_no    NUMBER(5)     NOT NULL,
  account_id NUMBER(10)    NOT NULL,
  amount     NUMBER(14, 2) NOT NULL,
  posted_at  DATE          NOT NULL,
  memo       VARCHAR2(60),
  CONSTRAINT feed_lines_pk PRIMARY KEY (batch_id, line_no)
);

CREATE INDEX ledger_posted_ix ON ledger (posted_at);

CREATE OR REPLACE PACKAGE ledger_post IS
  PROCEDURE post_batch (p_batch IN VARCHAR2);
  FUNCTION lines_posted RETURN PLS_INTEGER;
  PROCEDURE open_report (p_batch IN VARCHAR2, p_sort IN VARCHAR2, p_rows OUT SYS_REFCURSOR);
  FUNCTION day_total (p_day IN DATE) RETURN NUMBER;
END ledger_post;
/

CREATE OR REPLACE PACKAGE BODY ledger_post IS
  g_posted PLS_INTEGER := 0;

  PROCEDURE post_batch (p_batch IN VARCHAR2) IS
    CURSOR c_lines IS
      SELECT account_id, amount, posted_at, memo
      FROM feed_lines
      WHERE batch_id = p_batch
      ORDER BY line_no;
    TYPE t_lines IS TABLE OF c_lines%ROWTYPE INDEX BY PLS_INTEGER;
    v_lines t_lines;
  BEGIN
    OPEN c_lines;
    LOOP
      FETCH c_lines BULK COLLECT INTO v_lines LIMIT 100;
      EXIT WHEN c_lines%NOTFOUND;
      FORALL i IN 1 .. v_lines.COUNT
        INSERT INTO ledger (account_id, amount, posted_at, memo)
        VALUES (v_lines(i).account_id, v_lines(i).amount, v_lines(i).posted_at, v_lines(i).memo);
      FOR i IN 1 .. v_lines.COUNT LOOP
        UPDATE accounts
        SET balance = balance + v_lines(i).amount
        WHERE account_id = v_lines(i).account_id;
      END LOOP;
      g_posted := g_posted + v_lines.COUNT;
      COMMIT;
    END LOOP;
    CLOSE c_lines;
  EXCEPTION
    WHEN OTHERS THEN NULL;
  END post_batch;

  FUNCTION lines_posted RETURN PLS_INTEGER IS
  BEGIN
    RETURN g_posted;
  END lines_posted;

  PROCEDURE open_report (p_batch IN VARCHAR2, p_sort IN VARCHAR2, p_rows OUT SYS_REFCURSOR) IS
  BEGIN
    OPEN p_rows FOR
      'SELECT entry_id, account_id, amount, memo FROM ledger WHERE memo LIKE ''' || p_batch
      || ' line %'' ORDER BY ' || p_sort;
  END open_report;

  FUNCTION day_total (p_day IN DATE) RETURN NUMBER IS
    v_total NUMBER;
  BEGIN
    SELECT NVL(SUM(amount), 0)
    INTO v_total
    FROM ledger
    WHERE TRUNC(posted_at) = TRUNC(p_day);
    RETURN v_total;
  END day_total;
END ledger_post;
/

-- The author's test: a generated batch of 200 lines, 100 each on accounts 1 and 2, then the count it checks.
INSERT INTO feed_lines (batch_id, line_no, account_id, amount, posted_at, memo)
  SELECT 'b-0412', LEVEL, MOD(LEVEL, 2) + 1, 10.00, DATE '2031-04-12', 'b-0412 line ' || LEVEL
  FROM dual
  CONNECT BY LEVEL <= 200;
COMMIT;

BEGIN
  ledger_post.post_batch('b-0412');
  DBMS_OUTPUT.PUT_LINE('lines_posted = ' || ledger_post.lines_posted);
END;
/
-- Expected: lines_posted = 200

SELECT COUNT(*) AS ledger_rows FROM ledger WHERE memo LIKE 'b-0412 line %';
-- Expected: 200, the number the test compares, so the test succeeds.

-- Cleanup: post_batch committed, so its rows are removed by hand and the balances put back.
DELETE FROM ledger WHERE memo LIKE 'b-0412 line %';
UPDATE accounts SET balance = balance - 1000.00 WHERE account_id IN (1, 2);
COMMIT;
DROP PACKAGE ledger_post;
DROP INDEX ledger_posted_ix;
DROP TABLE feed_lines PURGE;

"""review_before.sql in the lab: SQL*Plus runs it as LAB on a freshly loaded practice schema."""

from oracle_session import Lab

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

$ sqlplus -s lab@//localhost/FREEPDB1 @review/review_before.sql
Table dropped.


Index dropped.


Table created.


Index created.


Package created.


Package body created.


200 rows created.


Commit complete.

lines_posted = 200

PL/SQL procedure successfully completed.


LEDGER_ROWS
-----------
        200


200 rows deleted.


2 rows updated.


Commit complete.


Package dropped.


Index dropped.


Table dropped.

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

The checker reads each script beside it and prints the lines that its eight rules flag, in the checklist's order. It
is a teaching aid built on regular expressions, not a PL/SQL parser: it does not follow comments that span lines,
text inside quotes or code it was not written for. A flagged line is a place to read closely, and a script with no
findings has passed these eight rules only, which is not the same as being correct.
"""

import re
from pathlib import Path

HERE = Path(__file__).resolve().parent
FILES = ["review_before.sql", "review_after.sql"]

SPEC = re.compile(r"^\s*CREATE\s+(OR\s+REPLACE\s+)?PACKAGE\s+(?!BODY\b)(\w+)", re.I)
BODY = re.compile(r"^\s*CREATE\s+(OR\s+REPLACE\s+)?PACKAGE\s+BODY\s+(\w+)", re.I)
SUBPROGRAM = re.compile(r"^\s*(PROCEDURE|FUNCTION)\s+\w+", re.I)
DECLARATION = re.compile(r"^\s*([a-z]\w*)\s+[\w.%()]+[^;]*;", re.I)
NOT_VARIABLES = {"cursor", "type", "subtype", "pragma", "procedure", "function", "end"}
FOR_LOOP = re.compile(r"^\s*FOR\s+\w+\s+IN\b.*\bLOOP\b", re.I)
END_LOOP = re.compile(r"^\s*END\s+LOOP\b", re.I)
DML = re.compile(r"^\s*(INSERT|UPDATE|DELETE|MERGE)\b", re.I)
BULK_FETCH = re.compile(r"\bFETCH\b.*\bBULK\s+COLLECT\b.*\bLIMIT\b", re.I)
EXIT_NOTFOUND = re.compile(r"^\s*EXIT\s+WHEN\s+\w+%NOTFOUND\b", re.I)
DYNAMIC_START = re.compile(r"\b(OPEN\s+\w+\s+FOR|EXECUTE\s+IMMEDIATE)\b(.*)", re.I)
COLUMN_IN_FUNCTION = re.compile(
    r"\b(WHERE|AND|OR)\b.*?\b(TRUNC|UPPER|LOWER|TO_CHAR|SUBSTR|NVL)\s*\(\s*([a-z][\w.]*)\s*[,)]", re.I
)
PARAMETER_OR_VARIABLE = re.compile(r"^(p|v|l|g)_", re.I)

RULES = [
    ("errors", "a WHEN OTHERS handler that does nothing hides every failure from the caller"),
    ("transactions", "a COMMIT or ROLLBACK inside the package ends the caller's transaction"),
    ("bulk loop", "EXIT WHEN %NOTFOUND right after a LIMIT fetch drops the last, partial batch"),
    ("set-based", "one DML statement per row inside a FOR loop: one call to SQL per row"),
    ("dynamic SQL", "caller text joined into a statement with || instead of a bind or an allowed value"),
    ("rights", "a package specification without AUTHID: definer's rights by default, never decided"),
    ("state", "a package-level variable that a call changes: its value outlives the call"),
    ("performance", "a function around a column in a condition: an index on that column cannot serve it"),
]


def code(line):
    """The line without a trailing -- comment (enough for these two scripts)."""
    return line.split("--", 1)[0].rstrip()


def statement_from(lines, start):
    """The text of the statement that starts on line `start` (0-based), up to its semicolon."""
    parts = []
    for text in lines[start:]:
        parts.append(code(text))
        if ";" in code(text):
            break
    return " ".join(parts)


def findings(lines):
    """(rule number, line number) pairs for one script."""
    found = []
    body_start, body_end, body_name = None, None, None
    for n, text in enumerate(lines):
        m = BODY.match(text)
        if m:
            body_start, body_name = n, m.group(2)
        elif body_start is not None and body_end is None and re.match(rf"^\s*END\s+{body_name}\s*;", text, re.I):
            body_end = n
    in_body = lambda n: body_start is not None and body_start < n < (body_end or len(lines))

    # Rule 6: every package specification states its rights model.
    for n, text in enumerate(lines):
        if SPEC.match(text):
            header = statement_from(lines, n).split(" IS", 1)[0].split(" AS", 1)[0]
            if not re.search(r"\bAUTHID\b", header, re.I):
                found.append((6, n + 1))

    # Rule 7: variables declared in the body before its first subprogram, then assigned by a subprogram.
    if body_start is not None:
        first_sub = next((n for n in range(body_start + 1, len(lines)) if SUBPROGRAM.match(lines[n])), len(lines))
        for n in range(body_start + 1, first_sub):
            m = DECLARATION.match(code(lines[n]))
            if m and m.group(1).lower() not in NOT_VARIABLES:
                name = m.group(1)
                if any(re.search(rf"\b{name}\s*:=", code(t), re.I) for t in lines[first_sub:body_end]):
                    found.append((7, n + 1))

    depth = 0
    for n, text in enumerate(lines):
        line = code(text)
        if re.search(r"\bWHEN\s+OTHERS\s+THEN\s+NULL\s*;", line, re.I):
            found.append((1, n + 1))
        if in_body(n) and re.match(r"^\s*(COMMIT|ROLLBACK)\b", line, re.I):
            found.append((2, n + 1))
        if BULK_FETCH.search(line):
            nxt = next((k for k in range(n + 1, len(lines)) if code(lines[k]).strip()), None)
            if nxt is not None and EXIT_NOTFOUND.match(code(lines[nxt])):
                found.append((3, nxt + 1))
        if FOR_LOOP.match(line):
            depth += 1
        elif END_LOOP.match(line) and depth:
            depth -= 1
        elif depth and DML.match(line):
            found.append((4, n + 1))
        if DYNAMIC_START.search(line):
            rest = DYNAMIC_START.search(statement_from(lines, n)).group(2).strip()
            if not rest.upper().startswith(("SELECT", "WITH")) and "||" in rest:
                found.append((5, n + 1))
        m = COLUMN_IN_FUNCTION.search(line)
        if m and not PARAMETER_OR_VARIABLE.match(m.group(3)):
            found.append((8, n + 1))
    return sorted(found)


for name in FILES:
    lines = (HERE / name).read_text(encoding="utf-8").splitlines()
    result = findings(lines)
    print(f"{name}: {len(result)} finding{'s' if len(result) != 1 else ''}")
    for rule, line_no in result:
        area, why = RULES[rule - 1]
        print(f"  line {line_no:>3}  {area:<13} {why}")
    print()

review_before.sql: 8 findings
  line  61  errors        a WHEN OTHERS handler that does nothing hides every failure from the caller
  line  57  transactions  a COMMIT or ROLLBACK inside the package ends the caller's transaction
  line  47  bulk loop     EXIT WHEN %NOTFOUND right after a LIMIT fetch drops the last, partial batch
  line  52  set-based     one DML statement per row inside a FOR loop: one call to SQL per row
  line  71  dynamic SQL   caller text joined into a statement with || instead of a bind or an allowed value
  line  24  rights        a package specification without AUTHID: definer's rights by default, never decided
  line  33  state         a package-level variable that a call changes: its value outlives the call
  line  82  performance   a function around a column in a condition: an index on that column cannot serve it

review_after.sql: 0 findings