Skip to content

Commit 51c0803

Browse files
authored
refactor: extract CQL into feature/cql (part of #778)
Extract the CQL (Cassandra interface) statement family out of core internal/mycli into internal/mycli/feature/cql through the Feature seam: the statement, the #757 fail-closed first-keyword classifier (byte-identical), the Cassandra type-name formatter, and the lazy adapter session (build-once, now mutex-guarded, closed via the feature store) all move; core no longer imports gocql or go-spanner-cassandra (verified via go list -deps; the root main still does via feature/all). CQL stays NOT detached-compatible. feature/all registers (cql, bigquery), keeping the merged statement order LLM, CQL, BIGQUERY - README and docs/system_variables.md are byte-identical. READONLY coverage preserved or improved: mutating CQL is blocked through the real dispatch path with the READONLY sentinel; the allowed SELECT and the mutating DELETE are classification-asserted on DISPATCH-BUILT statements (review nit), so a HandleGroups regression returning a zero-value statement fails tests instead of silently blocking CQL SELECT under READONLY. The allowed path deliberately stops before Execute: spancql.NewCluster panics when the adapter cannot be built (pre-existing upstream behavior, recorded as a follow-up candidate). Reviewed: maintainer APPROVE-WITH-NIT (nit addressed in-review); independent adversarial review SAFE TO MERGE (classifier/Execute equivalence verified, one unobservable failure-retry nuance recorded); Gemini INFO. CI green.
1 parent 09e4d81 commit 51c0803

13 files changed

Lines changed: 551 additions & 260 deletions

internal/mycli/client_side_statement_def.go

Lines changed: 5 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1191,10 +1191,11 @@ var clientSideStatementDefs = []*clientSideStatementDef{
11911191
return &ExitStatement{}, nil
11921192
},
11931193
},
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).
1194+
// The optional statement family GEMINI/LLM sits at the end of the table.
1195+
// BIGQUERY (#778) and CQL (#778) were extracted into internal/mycli/feature/
1196+
// {bigquery,cql}; GEMINI/LLM follows in PR3. Feature-contributed defs are
1197+
// appended after this core table in feature/all.All() order (llm, cql,
1198+
// bigquery).
11981199
// LLM
11991200
{
12001201
Descriptions: []clientSideStatementDescription{
@@ -1209,20 +1210,6 @@ var clientSideStatementDefs = []*clientSideStatementDef{
12091210
return &GeminiStatement{Text: unquoteString(groups["text"])}, nil
12101211
},
12111212
},
1212-
// Cassandra interface
1213-
{
1214-
Descriptions: []clientSideStatementDescription{
1215-
{
1216-
Usage: `Execute CQL`,
1217-
Syntax: `CQL ...`,
1218-
Note: "EARLY EXPERIMENTAL",
1219-
},
1220-
},
1221-
Pattern: regexp.MustCompile(`(?is)^CQL\s+(?P<cql>.+)$`),
1222-
HandleGroups: func(groups map[string]string) (Statement, error) {
1223-
return &CQLStatement{CQL: groups["cql"]}, nil
1224-
},
1225-
},
12261213
}
12271214

12281215
// Helper functions for HandleGroups implementations
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
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 CQL family (#757,
18+
// re-homed by #778). These live in the external mycli_test package so they can
19+
// import feature/cql (which imports mycli) without a cycle, and prove the guard
20+
// fires through the real dispatch path: build the statement from the merged def
21+
// table exactly as production does, then execute it against a READONLY session.
22+
23+
import (
24+
"testing"
25+
26+
"github.com/apstndb/spanner-mycli/internal/mycli"
27+
"github.com/apstndb/spanner-mycli/internal/mycli/feature/cql"
28+
)
29+
30+
func buildCQL(t *testing.T, cqlText string) mycli.Statement {
31+
t.Helper()
32+
defs := mycli.MergedStatementDefs(cql.Feature())
33+
stmt, err := mycli.BuildStatementWithDefs(defs, "CQL "+cqlText)
34+
if err != nil {
35+
t.Fatalf("BuildStatementWithDefs(CQL %s) error = %v", cqlText, err)
36+
}
37+
return stmt
38+
}
39+
40+
// TestReadOnlyGuardBlocksMutatingCQL verifies that mutating CQL is rejected by
41+
// Session.ExecuteStatement in READONLY mode before the statement can reach the
42+
// Cassandra adapter. These statements short-circuit at the guard before
43+
// CQLStatement.Execute, so no adapter/proxy is built.
44+
func TestReadOnlyGuardBlocksMutatingCQL(t *testing.T) {
45+
t.Parallel()
46+
47+
for _, tt := range []struct {
48+
desc string
49+
cql string
50+
}{
51+
{desc: "DELETE blocked", cql: "DELETE FROM t WHERE id = 1"},
52+
{desc: "INSERT blocked", cql: "INSERT INTO t (id) VALUES (1)"},
53+
{desc: "unrecognized keyword blocked", cql: "FROBNICATE t"},
54+
} {
55+
t.Run(tt.desc, func(t *testing.T) {
56+
t.Parallel()
57+
session := mycli.NewReadOnlySessionForTest(t)
58+
_, err := session.ExecuteStatement(t.Context(), buildCQL(t, tt.cql))
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+
// TestReadOnlyGuardAllowsReadOnlyCQL verifies, at the dispatch level, that a
67+
// read-only CQL SELECT is not classified as a static MutationStatement and that
68+
// the DISPATCH-BUILT statement's conditional classifier reports non-mutating,
69+
// so the READONLY guard lets it through. Asserting the classification on the
70+
// statement produced by the real def table (not one built via the constructor)
71+
// catches a HandleGroups regression that returns a zero-value CQLStatement:
72+
// with the fail-closed nil-Classify default, that regression would silently
73+
// block CQL SELECT in READONLY mode (#782 review nit).
74+
//
75+
// Unlike the BigQuery counterpart, this does NOT run the SELECT through
76+
// Session.ExecuteStatement: a passing SELECT proceeds to CQLStatement.Execute,
77+
// which builds the Cassandra adapter via spancql.NewCluster. That call panics
78+
// when the adapter client cannot be constructed (no live Spanner backend in
79+
// tests), so it cannot "fail later for a non-READONLY reason" the way BigQuery's
80+
// project-not-configured guard does.
81+
func TestReadOnlyGuardAllowsReadOnlyCQL(t *testing.T) {
82+
t.Parallel()
83+
84+
stmt := buildCQL(t, "SELECT * FROM t")
85+
if _, isMutation := stmt.(mycli.MutationStatement); isMutation {
86+
t.Fatal("CQLStatement must not be a static MutationStatement")
87+
}
88+
conditional, mutating := mycli.ClassifyForTest(stmt)
89+
if !conditional {
90+
t.Fatal("dispatch-built CQLStatement must be a ConditionallyMutatingStatement")
91+
}
92+
if mutating {
93+
t.Fatal("dispatch-built CQL SELECT classifies as mutating; READONLY mode would wrongly block it (HandleGroups regression?)")
94+
}
95+
96+
// The mutating counterpart, classified on the dispatch-built statement.
97+
del := buildCQL(t, "DELETE FROM t WHERE id = 1")
98+
conditional, mutating = mycli.ClassifyForTest(del)
99+
if !conditional || !mutating {
100+
t.Fatalf("dispatch-built CQL DELETE: conditional=%v mutating=%v, want true/true", conditional, mutating)
101+
}
102+
}

internal/mycli/export_test.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,3 +58,16 @@ func NewSessionWithFeaturesForTest(t *testing.T, features ...Feature) *Session {
5858
func ListVariablesForTest(s *Session) map[string]string {
5959
return s.systemVariables.ListVariables()
6060
}
61+
62+
// ClassifyForTest reports how the READONLY guard would classify stmt:
63+
// conditional is true when stmt is a ConditionallyMutatingStatement, and
64+
// mutating is its runtime classification (false when not conditional). The
65+
// marker method is unexported, so external dispatch-level tests use this bridge
66+
// to assert the classification of statements built through the real def table.
67+
func ClassifyForTest(stmt Statement) (conditional, mutating bool) {
68+
cm, ok := stmt.(ConditionallyMutatingStatement)
69+
if !ok {
70+
return false, false
71+
}
72+
return true, cm.isConditionallyMutating()
73+
}

internal/mycli/feature/all/all.go

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ package all
2020
import (
2121
"github.com/apstndb/spanner-mycli/internal/mycli"
2222
"github.com/apstndb/spanner-mycli/internal/mycli/feature/bigquery"
23+
"github.com/apstndb/spanner-mycli/internal/mycli/feature/cql"
2324
)
2425

2526
// All returns every optional feature in a fixed, documented order: GEMINI/LLM,
@@ -30,16 +31,17 @@ import (
3031
// Order-contract note (#778 §7): features append AFTER the core table, so
3132
// extracting a family moves its statement-help row to the end of the merged
3233
// 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.
34+
// than the pre-extraction core order (llm, bigquery, cql). The single
35+
// BIGQUERY/CQL row swap in the generated statement help landed in PR1 (BIGQUERY
36+
// extraction) alone; CQL now re-appends at the same relative position (before
37+
// BIGQUERY) and stays byte-identical.
3738
//
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}.
39+
// BIGQUERY and CQL are extracted; LLM still lives in core and will be added here
40+
// (before CQL, preserving the llm, cql, bigquery order) when it is extracted
41+
// under internal/mycli/feature/llm.
4142
func All() []mycli.Feature {
4243
return []mycli.Feature{
44+
cql.Feature(),
4345
bigquery.Feature(),
4446
}
4547
}
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
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 cql
16+
17+
import "strings"
18+
19+
// firstCQLKeyword returns the uppercased first whitespace-delimited token of a
20+
// CQL statement, or "" if there is none.
21+
func firstCQLKeyword(cql string) string {
22+
fields := strings.Fields(cql)
23+
if len(fields) == 0 {
24+
return ""
25+
}
26+
return strings.ToUpper(fields[0])
27+
}
28+
29+
// cqlStatementMutates reports whether a CQL statement should be treated as
30+
// mutating for the purpose of the READONLY guard.
31+
//
32+
// It classifies by the statement's first keyword and is deliberately
33+
// fail-closed: only statements whose first keyword is a known read-only verb
34+
// are allowed under READONLY. Everything else - data mutations
35+
// (INSERT/UPDATE/DELETE), batches (BATCH), DDL (CREATE/DROP/ALTER/TRUNCATE),
36+
// permission changes (GRANT/REVOKE), and any unrecognized or empty keyword - is
37+
// treated as mutating so the guard blocks it. This avoids silently allowing an
38+
// unknown CQL verb to bypass READONLY.
39+
//
40+
// SELECT is the only clearly read-only data verb in the Spanner Cassandra
41+
// adapter's supported surface. Other Cassandra read verbs (e.g. LIST, DESCRIBE)
42+
// are not part of that surface, so they are intentionally not allow-listed.
43+
func cqlStatementMutates(cql string) bool {
44+
switch firstCQLKeyword(cql) {
45+
case "SELECT":
46+
return false
47+
default:
48+
return true
49+
}
50+
}
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
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 cql
16+
17+
import "testing"
18+
19+
// TestCQLStatementMutates exhaustively checks the fail-closed classification of
20+
// CQL statements: only SELECT is treated as read-only; every other verb
21+
// (mutations, DDL, permissions) and any unrecognized or empty keyword is
22+
// treated as mutating. Preserves the #757 coverage moved from the core
23+
// readonly_guard_test.go (#778).
24+
func TestCQLStatementMutates(t *testing.T) {
25+
t.Parallel()
26+
27+
for _, tt := range []struct {
28+
cql string
29+
want bool
30+
}{
31+
{cql: "SELECT * FROM t", want: false},
32+
{cql: "select * from t", want: false},
33+
{cql: " SELECT * FROM t", want: false},
34+
{cql: "INSERT INTO t (id) VALUES (1)", want: true},
35+
{cql: "UPDATE t SET c = 1 WHERE id = 1", want: true},
36+
{cql: "DELETE FROM t WHERE id = 1", want: true},
37+
{cql: "BEGIN BATCH INSERT INTO t (id) VALUES (1) APPLY BATCH", want: true},
38+
{cql: "TRUNCATE t", want: true},
39+
{cql: "CREATE TABLE t (id int PRIMARY KEY)", want: true},
40+
{cql: "DROP TABLE t", want: true},
41+
{cql: "ALTER TABLE t ADD c int", want: true},
42+
{cql: "GRANT SELECT ON t TO role", want: true},
43+
{cql: "REVOKE SELECT ON t FROM role", want: true},
44+
{cql: "BOGUS not a real verb", want: true}, // fail-closed on unknown keyword
45+
{cql: "", want: true}, // fail-closed on empty
46+
{cql: " ", want: true}, // fail-closed on whitespace-only
47+
} {
48+
t.Run(tt.cql, func(t *testing.T) {
49+
t.Parallel()
50+
if got := cqlStatementMutates(tt.cql); got != tt.want {
51+
t.Errorf("cqlStatementMutates(%q) = %v, want %v", tt.cql, got, tt.want)
52+
}
53+
})
54+
}
55+
}

0 commit comments

Comments
 (0)