CE-651 Add check for all queires to ErrQueryAffectedZeroRows

[MarcusGoldschmidt]

Description

• Bug fix
• New feature

Useful links:

• Baton SDK coding guidelines
• New contributor guide

[github-actions]

Claude finished @MarcusGoldschmidt's task in 1m 12s —— View job

---

PR Review: Add check for all queries to ErrQueryAffectedZeroRows

• Gather context and read changed files
• Review code changes
• Post review feedback

---

Summary

This PR changes the behavior of RunProvisioningQueries so that ErrQueryAffectedZeroRows is only returned when all queries in a provisioning set affect 0 rows, instead of failing immediately when any single query affects 0 rows.

This is a sensible change. For multi-query provisioning configs (e.g., an INSERT + an auxiliary UPDATE), the old behavior would incorrectly bail on the first 0-row query even if subsequent queries would succeed. With this change, the operation is only considered a no-op (triggering GrantAlreadyExists / GrantAlreadyRevoked annotations) when the entire set of queries collectively did nothing.

Issues

1. Edge case: empty queries slice (low severity)

At pkg/bsql/query.go:405:

if zeroRowCount == len(queries) {
    return ErrQueryAffectedZeroRows
}

If queries is empty, zeroRowCount == 0 and len(queries) == 0, so 0 == 0 evaluates to true and the function returns ErrQueryAffectedZeroRows — even though no queries were executed. The old code didn't have this issue because the loop simply wouldn't execute and the function would proceed to commit.

In practice, callers (Grant and Revoke in pkg/bsql/provisioning.go) check len(queries) == 0 before calling RunProvisioningQueries, so this is unlikely to trigger. However, RunProvisioningQueries is an exported function, so it's worth guarding:

if len(queries) > 0 && zeroRowCount == len(queries) {
    return ErrQueryAffectedZeroRows
}

Fix this →

2. Behavioral side effect worth noting (informational)

Previously, if query 1 affected 0 rows, query 2 never executed (early return). Now all queries execute regardless, and the zero-row check happens after. This is a meaningful change beyond just the error condition — subsequent queries may have side effects (inserts, updates) even when earlier queries were no-ops. This seems intentional and correct for the use case, but worth calling out for reviewers.

Looks good otherwise

The change aligns well with the idempotency patterns described in CLAUDE.md ("already exists" and "already revoked" are not errors). The logic is clean and easy to follow.

[github-actions]

Claude finished @MarcusGoldschmidt's task in 1m 5s —— View job

---

PR Review: Add check for all queries to ErrQueryAffectedZeroRows

• Gather context and read changed files
• Review code changes
• Post review feedback

---

Summary

This PR changes RunProvisioningQueries so that ErrQueryAffectedZeroRows is returned only when all queries in a provisioning set affect 0 rows, rather than failing immediately when any single query affects 0 rows.

This is a good change. For multi-query provisioning configs (e.g., an INSERT + an auxiliary UPDATE), the old behavior would incorrectly bail on the first 0-row query even when subsequent queries would succeed. Now, the operation is only treated as a no-op (triggering GrantAlreadyExists / GrantAlreadyRevoked annotations) when the entire set of queries collectively did nothing.

I can see the second commit (44a5763) already addressed the empty-slice edge case from the prior review by adding the len(queries) > 0 guard — nice.

Review

Looks good. The logic is clean, the edge case is handled, and the change aligns well with the idempotency patterns in this codebase (GrantAlreadyExists / GrantAlreadyRevoked annotations for no-op provisioning).

A few observations:

1. Behavioral change worth documenting (informational)

Previously, if query 1 affected 0 rows, query 2 was never executed (early return with rollback). Now all queries execute regardless, and the zero-row check happens after the loop. This means subsequent queries may have side effects (inserts, updates) even when earlier queries were no-ops. When using transactions (useTx = true), this is safe — if all queries are zero-row, the function returns the error before the commit at line 414, so the deferred rollback at line 342 kicks in. This seems intentional and correct.

2. Transaction safety is preserved ✓

When useTx = true, the rollback path is correctly triggered: the function returns ErrQueryAffectedZeroRows at line 406 before reaching the tx.Commit() at line 414, so committed remains false and the deferred rollback executes. No issue here.

3. No test coverage (minor)

There are no unit tests for RunProvisioningQueries. While the existing callers (Grant, Revoke, action.go, user_syncer.go) all guard against empty query slices, adding a test for the multi-query zero-row behavior would help document and protect this new semantic. Not blocking, but worth considering.

4. Consider: should a mix of zero-row and non-zero-row queries in a non-transactional context be a concern?

When useTx = false and some queries affect rows while others don't, those side effects are already committed. The function returns nil (success), which seems correct — partial progress in a non-transactional context is expected. Just flagging for awareness.

Verdict

The change is correct and well-motivated. LGTM.

[linear]

CE-651 baton-sql v0.5.2: multi-query revoke transactions silently rolled back by zero-rows check