Skip to content

Commit 09e4d81

Browse files
authored
refactor: extract BIGQUERY into feature/bigquery (part of #778)
Extract the BIGQUERY statement family out of core internal/mycli into internal/mycli/feature/bigquery through the Feature seam (#779): the statement, the #775 fail-closed READONLY classifier, the value formatter, the three CLI_BIGQUERY_* system variables, and the lazy client cache (rebuild on project/location change, now mutex-guarded and closed via the feature store) all move; core no longer imports cloud.google.com/go/bigquery (verified via go list -deps; the root main still does via feature/all). Credential handling is re-homed: the raw --credential bytes live on the durable startup config (Session.CredentialBytes), which makes them survive USE/DETACH by construction and deletes #775's SessionHandler carry-over machinery. Feature() allocates fresh config/Variable instances per call. Review fixes: ListVariables/ListMultiValues/ListVariableInfo now iterate a single core+feature def helper so SHOW VARIABLES and variable-name completion keep listing feature vars (regression found in review, covered by new tests); MutationClassifier fails closed on nil Classify; stale comment removed. The registration order contract becomes (llm, cql, bigquery); this PR carries the single disclosed README statement-help change (BIGQUERY/CQL rows swap) so PR2/PR3 extractions stay byte-identical. Reviewed: maintainer REQUEST-CHANGES addressed and APPROVEd; independent adversarial review SAFE TO MERGE; Gemini INFO. All CI checks green.
1 parent 3c53157 commit 09e4d81

29 files changed

Lines changed: 1174 additions & 644 deletions

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -633,8 +633,8 @@ and `{A|B|...}` for a mutually exclusive keyword.
633633
| Show help for variables | `HELP VARIABLES;` | |
634634
| Exit CLI | `EXIT;` | |
635635
| Compose query using LLM | `GEMINI "<prompt>";` | |
636-
| Execute BigQuery SQL | `BIGQUERY <sql>;` | |
637636
| Execute CQL | `CQL ...;` | EARLY EXPERIMENTAL |
637+
| Execute BigQuery SQL | `BIGQUERY <sql>;` | |
638638
<!-- statement-help end -->
639639

640640
## Meta Commands

internal/mycli/app.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ func run(ctx context.Context, opts *spannerOptions, features ...Feature) error {
164164
}
165165

166166
if opts.SysVarsHelp {
167-
fmt.Print(renderSystemVariablesHelp())
167+
fmt.Print(renderSystemVariablesHelp(features...))
168168
return nil
169169
}
170170

@@ -433,9 +433,12 @@ func renderClientStatementHelp(stmts []*clientSideStatementDef) string {
433433
// renderSystemVariablesHelp generates a markdown table of all system variables
434434
// from the variable registry. It backs the hidden --sysvars-help flag, which
435435
// `make docs-update` uses to regenerate the reference table in
436-
// docs/system_variables.md.
437-
func renderSystemVariablesHelp() string {
436+
// docs/system_variables.md. Feature-contributed variables (issue #778) are
437+
// registered too, so the generated reference documents the full binary's
438+
// surface (the full variant is what docs-update runs).
439+
func renderSystemVariablesHelp(features ...Feature) string {
438440
sysVars := newSystemVariablesWithDefaults()
441+
sysVars.featureVarDefs = featureVarDefs(features)
439442
sysVars.ensureRegistry()
440443

441444
var sb strings.Builder
Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
// Copyright 2026 apstndb
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package mycli_test
16+
17+
// Dispatch-level READONLY guard regression for the extracted BIGQUERY family
18+
// (#775, re-homed by #778). These live in the external mycli_test package so
19+
// they can import feature/bigquery (which imports mycli) without a cycle, and
20+
// prove the guard fires through the real dispatch path: build the statement from
21+
// the merged def table exactly as production does, then execute it against a
22+
// READONLY session.
23+
24+
import (
25+
"testing"
26+
27+
"github.com/apstndb/spanner-mycli/internal/mycli"
28+
"github.com/apstndb/spanner-mycli/internal/mycli/feature/bigquery"
29+
)
30+
31+
func buildBigQuery(t *testing.T, sql string) mycli.Statement {
32+
t.Helper()
33+
defs := mycli.MergedStatementDefs(bigquery.Feature())
34+
stmt, err := mycli.BuildStatementWithDefs(defs, "BIGQUERY "+sql)
35+
if err != nil {
36+
t.Fatalf("BuildStatementWithDefs(BIGQUERY %s) error = %v", sql, err)
37+
}
38+
return stmt
39+
}
40+
41+
// TestReadOnlyGuardBlocksMutatingBigQuery verifies that mutating BIGQUERY SQL is
42+
// rejected by Session.ExecuteStatement in READONLY mode before the statement can
43+
// reach BigQuery (no client is built).
44+
func TestReadOnlyGuardBlocksMutatingBigQuery(t *testing.T) {
45+
t.Parallel()
46+
47+
for _, tt := range []struct {
48+
desc string
49+
sql string
50+
}{
51+
{desc: "DELETE blocked", sql: "DELETE FROM dataset.table WHERE TRUE"},
52+
{desc: "CREATE blocked", sql: "CREATE TABLE dataset.table AS SELECT 1"},
53+
{desc: "unrecognized keyword blocked", sql: "FROBNICATE dataset.table"},
54+
} {
55+
t.Run(tt.desc, func(t *testing.T) {
56+
t.Parallel()
57+
session := mycli.NewReadOnlySessionForTest(t)
58+
_, err := session.ExecuteStatement(t.Context(), buildBigQuery(t, tt.sql))
59+
if !mycli.IsReadOnlyError(err) {
60+
t.Errorf("%s in READONLY mode: got error %v, want READONLY error", tt.desc, err)
61+
}
62+
})
63+
}
64+
}
65+
66+
// TestReadOnlyGuardAllowsReadOnlyBigQuery verifies that read-only BIGQUERY SQL
67+
// passes the READONLY guard: dispatch reaches Execute, which then fails for an
68+
// unrelated reason (no BigQuery project configured), NOT with the READONLY
69+
// error. This proves, through the real guard, that a SELECT/WITH is not wrongly
70+
// blocked. A statically-mutating classification is also ruled out.
71+
func TestReadOnlyGuardAllowsReadOnlyBigQuery(t *testing.T) {
72+
t.Parallel()
73+
74+
for _, tt := range []struct {
75+
desc string
76+
sql string
77+
}{
78+
{desc: "SELECT", sql: "SELECT 1"},
79+
{desc: "WITH", sql: "WITH cte AS (SELECT 1) SELECT * FROM cte"},
80+
} {
81+
t.Run(tt.desc, func(t *testing.T) {
82+
t.Parallel()
83+
stmt := buildBigQuery(t, tt.sql)
84+
if _, isMutation := stmt.(mycli.MutationStatement); isMutation {
85+
t.Fatal("BigQueryStatement must not be a static MutationStatement")
86+
}
87+
88+
session := mycli.NewReadOnlySessionForTest(t)
89+
_, err := session.ExecuteStatement(t.Context(), stmt)
90+
// The guard let it through; Execute then fails on missing project
91+
// config, which must NOT be the READONLY sentinel.
92+
if err == nil {
93+
t.Fatalf("%s: expected a non-READONLY error from Execute (no project configured), got nil", tt.desc)
94+
}
95+
if mycli.IsReadOnlyError(err) {
96+
t.Errorf("%s BIGQUERY wrongly blocked by READONLY guard: %v", tt.desc, err)
97+
}
98+
})
99+
}
100+
}
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
// Copyright 2026 apstndb
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package mycli_test
16+
17+
// Regression for the #781 review finding: after the BIGQUERY extraction, the
18+
// feature's variables must still appear in the variable enumeration that backs
19+
// SHOW VARIABLES and fuzzy variable-name completion (ListVariables), not only
20+
// in HELP VARIABLES / generated docs (ListVariableInfo). Exercised with the
21+
// real bigquery feature registered.
22+
23+
import (
24+
"testing"
25+
26+
"github.com/apstndb/spanner-mycli/internal/mycli"
27+
"github.com/apstndb/spanner-mycli/internal/mycli/feature/bigquery"
28+
)
29+
30+
func TestVariableListingIncludesBigQueryVars(t *testing.T) {
31+
t.Parallel()
32+
33+
session := mycli.NewSessionWithFeaturesForTest(t, bigquery.Feature())
34+
35+
listed := mycli.ListVariablesForTest(session)
36+
for _, name := range []string{
37+
"CLI_BIGQUERY_PROJECT",
38+
"CLI_BIGQUERY_LOCATION",
39+
"CLI_BIGQUERY_MAX_BYTES_BILLED",
40+
} {
41+
if _, ok := listed[name]; !ok {
42+
t.Errorf("variable listing (SHOW VARIABLES / completion source) does not include %s", name)
43+
}
44+
}
45+
}

internal/mycli/cli.go

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,12 +55,20 @@ type Cli struct {
5555
}
5656

5757
func NewCli(ctx context.Context, credential []byte, sysVars *systemVariables) (*Cli, error) {
58+
// Store the raw credential once on the durable startup config so features
59+
// (BigQuery) can build their own clients via Session.CredentialBytes()
60+
// regardless of USE/DETACH session replacement (#778 §4.6). The defensive
61+
// copy avoids retaining the caller-owned slice.
62+
if len(credential) > 0 {
63+
sysVars.Config.Credential = append([]byte(nil), credential...)
64+
}
65+
5866
session, err := createSession(ctx, credential, sysVars)
5967
if err != nil {
6068
return nil, err
6169
}
6270

63-
sessionHandler := newSessionHandlerWithCredential(session, credential)
71+
sessionHandler := NewSessionHandler(session)
6472

6573
// StreamManager now manages the TTY stream internally
6674

internal/mycli/client_side_statement_def.go

Lines changed: 4 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1191,10 +1191,10 @@ var clientSideStatementDefs = []*clientSideStatementDef{
11911191
return &ExitStatement{}, nil
11921192
},
11931193
},
1194-
// The optional statement families (GEMINI/BIGQUERY/CQL) sit at the end of
1195-
// the table so their extraction into feature packages (issue #778) appends
1196-
// them at the same position in the merged table, keeping dispatch order and
1197-
// the generated statement help byte-identical across the move.
1194+
// The optional statement families (GEMINI/CQL) sit at the end of the table.
1195+
// BIGQUERY was extracted into internal/mycli/feature/bigquery (#778); the
1196+
// remaining families follow in PR2/PR3. Feature-contributed defs are appended
1197+
// after this core table in feature/all.All() order (llm, cql, bigquery).
11981198
// LLM
11991199
{
12001200
Descriptions: []clientSideStatementDescription{
@@ -1209,19 +1209,6 @@ var clientSideStatementDefs = []*clientSideStatementDef{
12091209
return &GeminiStatement{Text: unquoteString(groups["text"])}, nil
12101210
},
12111211
},
1212-
// BigQuery
1213-
{
1214-
Descriptions: []clientSideStatementDescription{
1215-
{
1216-
Usage: `Execute BigQuery SQL`,
1217-
Syntax: `BIGQUERY <sql>`,
1218-
},
1219-
},
1220-
Pattern: regexp.MustCompile(`(?is)^BIGQUERY\s+(?P<sql>\S.*)$`),
1221-
HandleGroups: func(groups map[string]string) (Statement, error) {
1222-
return &BigQueryStatement{SQL: groups["sql"]}, nil
1223-
},
1224-
},
12251212
// Cassandra interface
12261213
{
12271214
Descriptions: []clientSideStatementDescription{

internal/mycli/client_side_statement_def_test.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,11 +41,14 @@ import (
4141
"testing"
4242

4343
"github.com/apstndb/spanner-mycli/internal/mycli"
44+
"github.com/apstndb/spanner-mycli/internal/mycli/feature/all"
4445
)
4546

4647
// mergedDefs is the merged (core + feature) client-side statement table the
47-
// invariants run over. With no features it is a copy of the core table.
48-
var mergedDefs = mycli.MergedStatementDefs()
48+
// invariants run over — the same table every consumer dispatches against in the
49+
// full binary. It includes the extracted feature families (e.g. BIGQUERY) via
50+
// all.All(), so the #728 invariants keep covering them after extraction (#778).
51+
var mergedDefs = mycli.MergedStatementDefs(all.All()...)
4952

5053
// syntaxPlaceholderValues maps <placeholder> tokens appearing in
5154
// Description.Syntax strings to plausible example values. The expander fails

internal/mycli/export_test.go

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
// Copyright 2026 apstndb
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package mycli
16+
17+
// Test-only bridge exposing a few unexported internals to the external
18+
// mycli_test package (issue #778). This keeps the production API surface small
19+
// while letting external tests — e.g. the feature/bigquery dispatch-level
20+
// READONLY guard test, which must live outside package mycli to avoid an import
21+
// cycle with feature packages — drive a real Session through the guard.
22+
23+
import (
24+
"errors"
25+
"testing"
26+
)
27+
28+
// NewReadOnlySessionForTest builds a READONLY, database-connected Session wired
29+
// for the pending-transaction lifecycle (no RPCs), for external-package dispatch
30+
// tests.
31+
func NewReadOnlySessionForTest(t *testing.T) *Session {
32+
t.Helper()
33+
s := newSessionForLocalVarTest(t)
34+
s.systemVariables.Transaction.ReadOnly = true
35+
return s
36+
}
37+
38+
// IsReadOnlyError reports whether err is the READONLY guard sentinel. The
39+
// sentinel stays unexported in production; this bridge lets external tests
40+
// assert on it.
41+
func IsReadOnlyError(err error) bool { return errors.Is(err, errReadOnly) }
42+
43+
// NewSessionWithFeaturesForTest builds a database-connected Session whose
44+
// variable registry includes the given features' variables, so external tests
45+
// can drive enumeration surfaces (SHOW VARIABLES, completion) with real
46+
// feature packages registered.
47+
func NewSessionWithFeaturesForTest(t *testing.T, features ...Feature) *Session {
48+
t.Helper()
49+
s := newSessionForLocalVarTest(t)
50+
s.systemVariables.featureVarDefs = featureVarDefs(features)
51+
s.systemVariables.Registry = NewVarRegistry(s.systemVariables)
52+
return s
53+
}
54+
55+
// ListVariablesForTest returns the single-value variable listing backing
56+
// SHOW VARIABLES and fuzzy variable-name completion, for external
57+
// enumeration-surface regression tests.
58+
func ListVariablesForTest(s *Session) map[string]string {
59+
return s.systemVariables.ListVariables()
60+
}

internal/mycli/feature.go

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,11 +185,15 @@ func (MarksDetachedCompatible) isDetachedCompatible() {}
185185
// MutationClassifier marks an embedding statement as a
186186
// ConditionallyMutatingStatement whose mutation-ness is decided at runtime by
187187
// Classify (e.g. CQL/BIGQUERY inspecting the statement's leading keyword).
188+
// A nil Classify classifies as mutating: the zero value of an embedding
189+
// statement fails closed under the READONLY guard instead of panicking.
188190
type MutationClassifier struct {
189191
Classify func() bool
190192
}
191193

192-
func (m MutationClassifier) isConditionallyMutating() bool { return m.Classify() }
194+
func (m MutationClassifier) isConditionallyMutating() bool {
195+
return m.Classify == nil || m.Classify()
196+
}
193197

194198
// NewRow builds a result Row from string cells. Exported wrapper over toRow for
195199
// feature packages.
@@ -206,6 +210,20 @@ func UnquoteString(s string) string { return unquoteString(s) }
206210
// ProjectID returns the configured Spanner project for the session.
207211
func (s *Session) ProjectID() string { return s.systemVariables.Connection.Project }
208212

213+
// CredentialBytes returns a defensive copy of the raw --credential bytes stored
214+
// on the durable startup config. Feature packages that build their own
215+
// (non-Spanner) clients use it as the credential source; because the bytes live
216+
// in the process-wide startup config rather than on the Session, they survive
217+
// USE/DETACH session replacement without any carry-over machinery (#778 §4.6).
218+
// Returns nil when no credential file was supplied.
219+
func (s *Session) CredentialBytes() []byte {
220+
cred := s.systemVariables.Config.Credential
221+
if len(cred) == 0 {
222+
return nil
223+
}
224+
return append([]byte(nil), cred...)
225+
}
226+
209227
// AuthOptions builds client auth options for the given credential using the
210228
// session's system variables. Exported wrapper over createAuthClientOptions for
211229
// feature packages that build their own (non-Spanner) clients.

internal/mycli/feature/all/all.go

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,29 @@
1717
// registers no features.
1818
package all
1919

20-
import "github.com/apstndb/spanner-mycli/internal/mycli"
20+
import (
21+
"github.com/apstndb/spanner-mycli/internal/mycli"
22+
"github.com/apstndb/spanner-mycli/internal/mycli/feature/bigquery"
23+
)
2124

22-
// All returns every optional feature in a fixed, documented order that mirrors
23-
// today's client-side statement table order: GEMINI/LLM, then BIGQUERY, then
24-
// CQL. The order is load-bearing: features are appended to the core statement
25-
// table in this order, and the #728 non-shadowing invariant is proven over the
26-
// resulting merged table.
25+
// All returns every optional feature in a fixed, documented order: GEMINI/LLM,
26+
// then CQL, then BIGQUERY. The order is load-bearing: features are appended to
27+
// the core statement table in this order, and the #728 non-shadowing invariant
28+
// is proven over the resulting merged table.
2729
//
28-
// It is empty in PR0 (the seam is introduced with zero behavior change; the
29-
// three families still live in core). Feature packages will be added under
30-
// internal/mycli/feature/{llm,bigquery,cql} as they are extracted.
31-
func All() []mycli.Feature { return nil }
30+
// Order-contract note (#778 §7): features append AFTER the core table, so
31+
// extracting a family moves its statement-help row to the end of the merged
32+
// table in All() order. The final order is fixed as (llm, cql, bigquery) rather
33+
// than the pre-extraction core order (llm, bigquery, cql). This makes the single
34+
// BIGQUERY/CQL row swap in the generated statement help land in PR1 (BIGQUERY
35+
// extraction) alone; PR2 (CQL) and PR3 (LLM) then re-append at the same relative
36+
// positions and stay byte-identical.
37+
//
38+
// Only BIGQUERY is extracted so far; LLM and CQL still live in core and will be
39+
// added here (before BIGQUERY, preserving the llm, cql, bigquery order) as they
40+
// are extracted under internal/mycli/feature/{llm,cql}.
41+
func All() []mycli.Feature {
42+
return []mycli.Feature{
43+
bigquery.Feature(),
44+
}
45+
}

0 commit comments

Comments
 (0)