Skip to content

fix: validate action queries instead of failing on empty query - #148

Merged
alan-lee-12 merged 2 commits into
mainfrom
alee/handle-queries-in-validate
Aug 13, 2026
Merged

fix: validate action queries instead of failing on empty query#148
alan-lee-12 merged 2 commits into
mainfrom
alee/handle-queries-in-validate

Conversation

@alan-lee-12

@alan-lee-12 alan-lee-12 commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Description

  • Bug fix
  • New feature

ActionConfig.staticValidate only inspected the singular Query field, so any action defining the plural queries form failed startup validation with query is required and blocked the sync entirely:

validate failed: validation error for resource type "user", field "Action[disable_user]": query is required

Both forms are already accepted by ActionConfig.Validate (which enforces exactly-one-of, pkg/bsql/config.go:658) and by handleQueryAction, which executes Queries when present (pkg/connector/action.go:209). Only the static validator was out of step — a false positive on configs that would otherwise run fine.

Regression window: introduced in v0.5.0 by c3f62e1 (#98). v0.4.4 had no ActionConfig.staticValidate at all, so the plural form worked by never being checked. Affects v0.5.0 through v0.5.13.

Reported by a customer upgrading service-mode baton-sql from v0.4.4 to v0.5.13 with a two-statement account_disable action.

Testing

Three cases added to the table-driven TestValidate in pkg/bsql/validate_test.go:

  • action with queries — the regression pin. Fails against the parent commit with the exact customer-facing error (query is required), passes with the fix.
  • action with singular query — guards the path the new branch could have bypassed.
  • action with queries referencing undefined var — confirms the loop still validates each statement rather than short-circuiting after the first.

go build ./... clean; full ./pkg/... suite passes.

Follow-up (not in this PR)

The error message blames a resource type (resource type "user") for a top-level actions: failure, because the action loop lives inside the per-resource-type SQLSyncer.Validate (pkg/bsql/sql_syncer.go:149). This misattribution actively misleads — it sent the reporter looking for an actions block under resource_types.user, which doesn't exist.

Useful links:

🤖 Generated with Claude Code

`ActionConfig.staticValidate` only ever inspected the singular `Query`
field, so any action defining the plural `queries` form failed startup
validation with "query is required" and blocked the sync entirely.

Both forms are already accepted by `ActionConfig.Validate` (which
enforces exactly-one-of) and by `handleQueryAction`, which executes
`Queries` when present. Only the static validator was out of step, so
this was a false positive on configs that would otherwise run fine.

Regression introduced in v0.5.0 by c3f62e1 (#98); v0.4.4 had no
`ActionConfig.staticValidate` at all, so the plural form worked by never
being checked.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread pkg/bsql/validate.go
@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Connector PR Review: fix: validate action queries instead of failing on empty query

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 93a1257fad3a.
Review mode: incremental since 20700f6d
View review run: https://github.com/ConductorOne/baton-sql/actions/runs/31650277872

Review Summary

The new commit is test-only: it adds three table-driven ActionConfig cases to pkg/bsql/validate_test.go covering the singular query form, the plural queries form, and a queries entry referencing an undefined variable. This addresses the previous finding that the pkg/bsql/validate.go:215-224 regression fix shipped without a test: the plural path is now pinned in both its passing and failing directions, and the assertions are behavior-based with no timing, ordering, or network dependencies. The full PR diff (validate.go plus validate_test.go, no go.mod/go.sum changes) was scanned for security and correctness; the early return nil in the queries branch is safe because ActionConfig.Validate at pkg/bsql/config.go:658 already rejects configs that set both query and queries. No new issues found.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

Pins the regression fixed in the previous commit. `action with queries`
fails against the prior implementation with the exact error customers
hit ("query is required") and passes with the fix.

Also covers the singular `query` path, which the new branch could have
bypassed, and a `queries` entry referencing an undefined var to confirm
the loop still rejects each statement individually.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@alan-lee-12
alan-lee-12 marked this pull request as ready for review August 13, 2026 14:11
@alan-lee-12
alan-lee-12 merged commit f0c6a77 into main Aug 13, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants