Skip to content

go: minimize and harden the public pkg/query API #1124

Description

@derekperkins

Summary

After #1123 makes pkg/query importable, reduce it to a deliberately supported API and make its resource lifecycle safe for use as a library.

The mechanical move must land separately first. This issue may change Go identifiers and signatures, but must preserve query results, cache behavior, CLI behavior, and the current fail-closed security boundary.

Current accidental surface

The package currently exports configuration state (Options), a raw option function type, DuckDB JSON-AST validator internals (Validator, ValidateSQL, and ErrorDetails), extension-reporting types, both buffered and writer query variants, and implementation-oriented errors. Most are not used by production callers outside the package.

Publishing all of these would couple downstream projects to the cache implementation, DuckDB's unstable serialized AST shape, and current schema-header plumbing.

Public API goals

  • Keep only the constructor, database handle, configuration options, query operations, and stable error classifications that an external consumer actually needs.
  • Replace exported mutable option state with private configuration and a deliberately named/sealed Option contract. Validate invalid connection, cache, TTL, and nil-logger inputs centrally.
  • Keep raw AST walking and concrete parser details private. Validator/ValidateSQL must not become the generic authorization plugin API.
  • Replace cross-package dependence on ErrorDetails with stable errors.Is classifications unless structured parser details are intentionally documented as a compatibility contract.
  • Decide explicitly whether both buffered (QueryJSON/QueryArrow) and streaming (WriteJSON/WriteArrow) operations are supported. Unexport unused variants, or document and add black-box tests for retained variants.
  • Decide whether extension inventory belongs in this package or the extension helper work; avoid two competing public representations.
  • Do not cement allowedSchemas []string as a general-purpose query API. Coordinate its removal with the schema-authorizer migration without weakening current validation.

Lifecycle and ownership

External reuse also requires a deterministic lifecycle contract:

  • Define whether query.New transfers ownership of the duckdb.Connector. sql.OpenDB closes its connector, while the current comment and example binary imply separate ownership.
  • Prefer Close() error over logging close failures inside a library. Make close idempotent and define behavior for in-flight work.
  • Track and close Arrow connections deterministically rather than relying only on runtime cleanup of objects held by sync.Pool.
  • Release the Arrow semaphore on every failed acquisition/type path.
  • Do not reuse a constructor context that may be canceled for connections created much later; document or redesign the long-lived context behavior.
  • Preserve a non-nil logger default even when a caller supplies nil.

Compatibility and security constraints

  • Preserve validation before every cache lookup.
  • Preserve function-policy enforcement and fail-closed restricted exec until the authorization replacement proves equivalent coverage.
  • Do not parse SQL strings with regular expressions or expose the raw DuckDB serializer tree as a supported extension boundary.
  • Do not change the HTTP/WebSocket wire protocol or command-line flags here.
  • Keep the duckdb_arrow build requirement explicit unless this issue deliberately removes it.

Acceptance criteria

  • go doc ./pkg/query contains only intentionally supported identifiers, each with useful package documentation.
  • At least one package query_test example compiles using only the public API.
  • A temporary external module covers construction, a JSON or Arrow query, error classification, and shutdown with the duckdb_arrow tag.
  • Tests cover defaults and invalid options, nil logger behavior, connector ownership, repeated close, failed Arrow connection creation, permit release, and close/query races.
  • Existing cache-policy, function-policy, parser, JSON, and Arrow tests continue to pass.
  • From packages/server/duckdb-server-go, these pass:
    • go test -tags=duckdb_arrow ./... -count=1
    • go test -race -tags=duckdb_arrow ./... -count=1
    • golangci-lint run

Sequencing

Depends on #1123. It can proceed in parallel with #1125 and #1126, but those branches must agree on the small query/server boundary before publication. Coordinate schema-specific signature changes with #1127. Do not cut a new module tag until this cleanup is complete.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions