Err for grant/revoke when no row affected

[MarcusGoldschmidt]

Description

• Bug fix
• New feature

Useful links:

• Baton SDK coding guidelines
• New contributor guide

[github-actions]

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

---

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

Review: Err for grant/revoke when no row affected

This PR introduces sentinel errors for zero/multi-row affected cases in provisioning queries and handles the zero-rows case in Revoke by returning a GrantAlreadyRevoked annotation. The approach is sound, but there's one notable gap.

---

Issue: Grant doesn't handle ErrQueryAffectedZeroRows

Revoke correctly maps zero affected rows → GrantAlreadyRevoked annotation (lines 122-126 of provisioning.go), but Grant at line 76 still propagates the error as-is:

// provisioning.go:75-78
err = s.RunProvisioningQueries(ctx, provisioningConfig.Grant.Queries, provisioningVars, useTx)
if err != nil {
    return nil, err  // ← ErrQueryAffectedZeroRows not handled
}

Per the Grant Idempotency pattern in CLAUDE.md, "already exists" should be treated as success:

if errors.Is(err, ErrQueryAffectedZeroRows) {
    anno := annotations.Annotations{}
    anno.Update(&v2.GrantAlreadyExists{})
    return anno, nil
}

Fix this →

---

Minor observation: zero rows may be legitimate in multi-query provisioning

RunProvisioningQueries iterates over multiple queries. If any query in the batch affects 0 rows, the whole operation fails/rolls back. This is a reasonable safety check, but worth noting that some provisioning setups might have auxiliary queries (e.g., logging or audit inserts) where 0 rows could be unexpected for a different reason. The error message "ending and rolling back" accurately describes the behavior, so this is fine for now.

---

Summary: The sentinel errors and Revoke handling look good. The main gap is the missing Grant handler for ErrQueryAffectedZeroRows — without it, Grant will return an error instead of the expected GrantAlreadyExists annotation.

[github-actions]

Claude finished @MarcusGoldschmidt's task in 52s —— View job

---

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

Review: Err for grant/revoke when no row affected

The sentinel errors and Revoke handling look good overall. A few items to address:

---

1. Grant doesn't handle ErrQueryAffectedZeroRows (same issue flagged in prior review)

Revoke correctly maps zero affected rows to a GrantAlreadyRevoked annotation (lines 122-126 of provisioning.go), but Grant at line 76 still returns the raw error:

// provisioning.go:74-78
useTx := !provisioningConfig.Grant.NoTransaction
err = s.RunProvisioningQueries(ctx, provisioningConfig.Grant.Queries, provisioningVars, useTx)
if err != nil {
    return nil, err  // ← ErrQueryAffectedZeroRows not handled
}

Per the Grant Idempotency pattern in CLAUDE.md, "already exists" should return success with an annotation:

if errors.Is(err, ErrQueryAffectedZeroRows) {
    anno := annotations.Annotations{}
    anno.Update(&v2.GrantAlreadyExists{})
    return anno, nil
}

Fix this →

---

2. ErrQueryAffectedMoreThanOneRow is unused outside RunProvisioningQueries

The new ErrQueryAffectedMoreThanOneRow sentinel is exported but never checked with errors.Is anywhere — neither Grant nor Revoke handle it specially. This is fine if you plan to handle it later or want callers to be able to check it, but currently it's functionally equivalent to the old inline errors.New. Just noting this for awareness — no change needed.

---

3. Minor: gosec nolint removal

The removal of //nolint:gosec comments from config.go and database.go is a nice cleanup. These were false positives on password field names, so removing them is correct.

---

Summary: The core change (sentinel errors + Revoke annotation) is solid. The main gap remains: Grant should handle ErrQueryAffectedZeroRows → GrantAlreadyExists for idempotency parity with Revoke.

[github-actions]

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

---

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

Review: Err for grant/revoke when no row affected (v3)

Good progress — the previous review feedback about Grant handling ErrQueryAffectedZeroRows has been addressed. The new validation queries feature is a nice addition. A few items to flag:

---

1. Validation query *sql.Rows not closed on error path (query.go:349-354)

If executor.QueryContext succeeds but result.Next() returns false (no rows), you correctly close and return. But if result.Err() returns an error on line 356, you return without closing result. The result.Close() call on line 360 is skipped:

result, err := executor.QueryContext(ctx, q, qArgs...)
if err != nil {
    return fmt.Errorf("failed to execute validation query: %w", err)
}

valid := result.Next()

if err := result.Err(); err != nil {
    return fmt.Errorf("failed to read validation query result: %w", err)  // ← result not closed
}

err = result.Close()

Use defer result.Close() immediately after the QueryContext error check to guarantee cleanup on all paths:

result, err := executor.QueryContext(ctx, q, qArgs...)
if err != nil {
    return fmt.Errorf("failed to execute validation query: %w", err)
}
defer result.Close()

valid := result.Next()
if err := result.Err(); err != nil {
    return fmt.Errorf("failed to read validation query result: %w", err)
}

This also lets you remove the explicit result.Close() and its error handling block.

Fix this →

---

2. Validation query failure returns a generic error — not mapped to a sentinel

When a validation query returns no rows (line 366):

if !valid {
    return fmt.Errorf("validation query returned no rows")
}

This is a plain fmt.Errorf, which means callers in Grant/Revoke won't match it with errors.Is(err, ErrQueryAffectedZeroRows). So if validation fails, the Grant method returns the raw error instead of the GrantAlreadyExists annotation. Similarly for Revoke.

If the intent is that failed validation should be treated the same as zero affected rows (i.e., idempotent "already exists"/"already revoked"), wrap the sentinel:

if !valid {
    return fmt.Errorf("validation query returned no rows: %w", ErrQueryAffectedZeroRows)
}

If validation failure should be a hard error (different from zero rows affected), the current behavior is correct — just be aware that it bypasses the idempotency handling.

---

3. Minor: ErrQueryAffectedMoreThanOneRow is still unused by callers

As noted in the previous review, neither Grant nor Revoke handle ErrQueryAffectedMoreThanOneRow with errors.Is. It's a nice sentinel to have for future use, but currently provides no behavioral difference from the old inline errors.New. No action needed — just noting for awareness.

---

Summary: The core changes look good. Both Grant and Revoke now properly handle zero-rows as idempotent success with annotations. The main issue is the *sql.Rows leak on the error path in the validation query loop — use defer result.Close() to fix it. The validation query error mapping (item 2) is a design decision worth clarifying.