fix(cli): render gosqlx format through pkg/formatter and refuse non-round-tripping output (#599)

cmd/gosqlx/cmd/doc.go

@@ -285,11 +285,18 @@
 //
 // ## format Command
 //
-// Formats SQL with intelligent indentation.
+// Formats SQL with intelligent indentation. Rendering delegates to
+// pkg/formatter, and the output is checked by re-parsing it: it must re-parse
+// to the same number of statements with the same AST.SQL() and per-statement
+// rendering, and keep every string literal and the meaning of every quoted
+// identifier; otherwise the file is reported as an error and left unchanged.
+// The check compares the input's parse with the output's parse, so anything
+// the parser itself does not keep (comments, clauses it skips) is not
+// detected and is dropped from the output.
 //
 // Implementation: formatter.go, sql_formatter.go
 // Key types: Formatter, CLIFormatterOptions, SQLFormatter
-// Key functions: Format(), formatFile(), formatSQL()
+// Key functions: Format(), formatFile(), formatSQL(), verifyFormattedSQL()
 //
 // ## parse Command
 //

cmd/gosqlx/cmd/format_check_summary_test.go

@@ -0,0 +1,62 @@
+// Copyright 2026 GoSQLX Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package cmd
+
+import (
+	"bytes"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// TestFormat_CheckSummaryOmitsFailedFiles checks that --check does not claim
+// every file is properly formatted when some files failed the check.
+func TestFormat_CheckSummaryOmitsFailedFiles(t *testing.T) {
+	dir := t.TempDir()
+	good := filepath.Join(dir, "good.sql")
+	bad := filepath.Join(dir, "bad.sql")
+	// EXPLAIN is refused by the formatter, so bad.sql always fails.
+	if err := os.WriteFile(good, []byte("SELECT a FROM t"), 0600); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(bad, []byte("EXPLAIN VERBOSE DELETE FROM users"), 0600); err != nil {
+		t.Fatal(err)
+	}
+
+	var out, errBuf bytes.Buffer
+	f := NewFormatter(&out, &errBuf, CLIFormatterOptions{Check: true, Compact: true, Uppercase: true})
+	res, err := f.Format([]string{good, bad})
+	if err != nil {
+		t.Fatalf("Format: %v", err)
+	}
+	if res.FailedFiles != 1 {
+		t.Fatalf("FailedFiles = %d, want 1 (stderr: %s)", res.FailedFiles, errBuf.String())
+	}
+	if strings.Contains(out.String(), "properly formatted") {
+		t.Fatalf("check summary claims files are properly formatted although one failed:\n%s", out.String())
+	}
+
+	// With no failures the summary is still printed.
+	out.Reset()
+	errBuf.Reset()
+	f = NewFormatter(&out, &errBuf, CLIFormatterOptions{Check: true, Compact: true, Uppercase: true})
+	if _, err := f.Format([]string{good}); err != nil {
+		t.Fatalf("Format: %v (stderr: %s)", err, errBuf.String())
+	}
+	if !strings.Contains(out.String(), "All 1 files are properly formatted") {
+		t.Fatalf("missing check summary, got stdout %q", out.String())
+	}
+}

cmd/gosqlx/cmd/format_corpus_test.go

@@ -0,0 +1,180 @@
+// Copyright 2026 GoSQLX Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package cmd
+
+import (
+	"io/fs"
+	"os"
+	"path/filepath"
+	"sort"
+	"strings"
+	"testing"
+
+	"github.com/ajitpratap0/GoSQLX/pkg/sql/ast"
+)
+
+// corpusFormatKnownGaps lists corpus files (relative to testdata) whose
+// formatted output does not yet round-trip because of a pkg/formatter or SQL()
+// rendering gap. For these the CLI must refuse to format (return an error)
+// rather than emit changed SQL; once the gap is fixed they simply pass.
+var corpusFormatKnownGaps = map[string]string{
+	"corpus/orm/gorm_joins.sql":      "follow-up: a name quoted only for case (\"Company\") renders bare; the AST does not record quoting, so the CLI refuses",
+	"mssql/13_cross_apply.sql":       "follow-up: CROSS APPLY rendered as CROSS APPLY JOIN, TOP lost in derived table",
+	"mssql/14_outer_apply.sql":       "follow-up: OUTER APPLY rendered as OUTER APPLY JOIN, TOP lost in derived table",
+	"mysql/29_full_text_search.sql":  "follow-up: MATCH ... AGAINST rendered as AGAINST AGAINST(...)",
+	"oracle/08_connect_by_path.sql":  "follow-up: START WITH / CONNECT BY not rendered",
+	"oracle/19_regexp_functions.sql": "follow-up: string literal backslashes doubled for a dialect that keeps them literal",
+}
+
+// corpusDialect maps a testdata path to the dialect its SQL is written in.
+func corpusDialect(rel string) string {
+	p := "/" + rel
+	switch {
+	case strings.Contains(p, "/mssql/"):
+		return "sqlserver"
+	case strings.Contains(p, "/mysql/"):
+		return "mysql"
+	case strings.Contains(p, "/oracle/"):
+		return "oracle"
+	case strings.Contains(p, "/postgresql/"):
+		return "postgresql"
+	default:
+		return ""
+	}
+}
+
+// TestFormatSQL_CorpusRoundTrip formats every parseable testdata/**/*.sql file
+// with the CLI formatter in several modes and requires that the output
+// re-parses to an equivalent tree and formats idempotently (#599).
+func TestFormatSQL_CorpusRoundTrip(t *testing.T) {
+	root := filepath.Join("..", "..", "..", "testdata")
+	var files []string
+	err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error {
+		if err != nil {
+			return err
+		}
+		if !d.IsDir() && strings.HasSuffix(path, ".sql") {
+			files = append(files, path)
+		}
+		return nil
+	})
+	if err != nil {
+		t.Fatalf("walking %s: %v", root, err)
+	}
+	sort.Strings(files)
+
+	modes := []struct {
+		name string
+		opts CLIFormatterOptions
+	}{
+		{"readable-upper", CLIFormatterOptions{IndentSize: 2, Uppercase: true}},
+		{"compact-upper", CLIFormatterOptions{Compact: true, Uppercase: true}},
+		{"readable-lower", CLIFormatterOptions{IndentSize: 4, Uppercase: false}},
+	}
+
+	parsed := 0
+	for _, path := range files {
+		relPath, err := filepath.Rel(root, path)
+		if err != nil {
+			t.Fatalf("relative path for %s: %v", path, err)
+		}
+		rel := filepath.ToSlash(relPath)
+		data, err := os.ReadFile(path)
+		if err != nil {
+			t.Fatalf("reading %s: %v", rel, err)
+		}
+		input := strings.ReplaceAll(string(data), "\r\n", "\n")
+		dialect := corpusDialect(rel)
+
+		orig, err := parseWithDialect(t, input, dialect)
+		if err != nil || len(orig.Statements) == 0 {
+			if orig != nil {
+				ast.ReleaseAST(orig)
+			}
+			continue // the corpus also holds inputs the parser does not support yet
+		}
+		parsed++
+		origSQL := orig.SQL()
+		origCount := len(orig.Statements)
+		ast.ReleaseAST(orig)
+
+		for _, m := range modes {
+			opts := m.opts
+			opts.Dialect = dialect
+			out, err := formatWith(input, opts)
+			if err != nil {
+				if reason, ok := corpusFormatKnownGaps[rel]; ok {
+					t.Logf("%s [%s]: known gap (%s): %v", rel, m.name, reason, err)
+					continue
+				}
+				t.Errorf("%s [%s]: format failed: %v", rel, m.name, err)
+				continue
+			}
+			if reason, ok := corpusFormatKnownGaps[rel]; ok {
+				t.Errorf("%s [%s]: listed as a known gap (%s) but now formats; remove it from corpusFormatKnownGaps", rel, m.name, reason)
+			}
+			// The parser does not yet accept every lowercase keyword sequence
+			// (e.g. "returning", "cross join lateral"), so lowercase output is
+			// checked through its uppercase-keyword twin, mirroring the CLI's
+			// own verification: the two must differ only in case.
+			checkText, checkOpts := out, opts
+			if !opts.Uppercase {
+				checkOpts.Uppercase = true
+				twin, err := formatWith(input, checkOpts)
+				if err != nil {
+					t.Errorf("%s [%s]: uppercase twin failed: %v", rel, m.name, err)
+					continue
+				}
+				if !strings.EqualFold(out, twin) {
+					t.Errorf("%s [%s]: output differs from its uppercase twin beyond keyword case\nlower:\n%s\nupper:\n%s", rel, m.name, out, twin)
+					continue
+				}
+				checkText = twin
+			}
+			checkCorpusOutput(t, rel, m.name, checkText, dialect, origSQL, origCount)
+			again, err := formatWith(checkText, checkOpts)
+			if err != nil {
+				t.Errorf("%s [%s]: re-formatting output failed: %v", rel, m.name, err)
+				continue
+			}
+			if again != checkText {
+				t.Errorf("%s [%s]: formatting is not idempotent\nfirst:\n%s\nsecond:\n%s", rel, m.name, checkText, again)
+			}
+		}
+	}
+	if parsed < 100 {
+		t.Fatalf("only %d corpus files parsed; expected at least 100 (is testdata missing?)", parsed)
+	}
+	t.Logf("formatted %d parseable corpus files in %d modes", parsed, len(modes))
+}
+
+// checkCorpusOutput independently re-parses out and compares it with the
+// original input's statement count and SQL().
+func checkCorpusOutput(t *testing.T, rel, mode, out, dialect, origSQL string, origCount int) {
+	t.Helper()
+	reparsed, err := parseWithDialect(t, out, dialect)
+	if err != nil {
+		t.Errorf("%s [%s]: output does not re-parse: %v\noutput:\n%s", rel, mode, err, out)
+		return
+	}
+	defer ast.ReleaseAST(reparsed)
+	if len(reparsed.Statements) != origCount {
+		t.Errorf("%s [%s]: statement count changed: %d -> %d", rel, mode, origCount, len(reparsed.Statements))
+		return
+	}
+	if got := reparsed.SQL(); !strings.EqualFold(got, origSQL) {
+		t.Errorf("%s [%s]: output changes the query\n original SQL(): %s\nreparsed SQL(): %s", rel, mode, origSQL, got)
+	}
+}

cmd/gosqlx/cmd/format_explain_test.go

@@ -0,0 +1,131 @@
+// Copyright 2026 GoSQLX Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package cmd
+
+import (
+	"bytes"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// TestFormatSQL_RefusesExplainAndDescribe guards against gosqlx format turning
+// a non-executing EXPLAIN into an executable statement. The parser splits
+// "EXPLAIN VERBOSE|ANALYZE <stmt>" into a DescribeStatement plus <stmt>, and
+// DescribeStatement drops EXPLAIN options and object kinds, so the CLI must
+// refuse it instead of writing "DESCRIBE VERBOSE; DELETE FROM users".
+func TestFormatSQL_RefusesExplainAndDescribe(t *testing.T) {
+	tests := []struct {
+		name    string
+		dialect string
+		in      string
+	}{
+		{name: "explain verbose delete", in: "EXPLAIN VERBOSE DELETE FROM users"},
+		{name: "explain verbose delete postgresql", dialect: "postgresql", in: "EXPLAIN VERBOSE DELETE FROM users;\n"},
+		{name: "explain verbose update", in: "EXPLAIN VERBOSE UPDATE accounts SET balance = 0"},
+		{name: "explain verbose drop", in: "EXPLAIN VERBOSE DROP TABLE users"},
+		{name: "explain verbose truncate", in: "EXPLAIN VERBOSE TRUNCATE TABLE users"},
+		{name: "explain verbose insert", in: "EXPLAIN VERBOSE INSERT INTO t (a) VALUES (1)"},
+		{name: "explain analyze delete", in: "EXPLAIN ANALYZE DELETE FROM users"},
+		{name: "mysql explain analyze select", dialect: "mysql", in: "EXPLAIN ANALYZE SELECT * FROM users"},
+		{name: "explain select", in: "EXPLAIN SELECT 1"},
+		{name: "describe table", in: "DESCRIBE users"},
+		{name: "snowflake describe table kind", dialect: "snowflake", in: "DESCRIBE TABLE users"},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			for _, opts := range []CLIFormatterOptions{
+				{Compact: true, Uppercase: true, Dialect: tc.dialect},
+				{IndentSize: 2, Uppercase: true, Dialect: tc.dialect},
+				{IndentSize: 2, Dialect: tc.dialect},
+			} {
+				out, err := formatWith(tc.in, opts)
+				if err == nil {
+					t.Fatalf("formatSQL(%q, %+v) = %q, want an error", tc.in, opts, out)
+				}
+				if out != "" {
+					t.Fatalf("formatSQL(%q) returned output %q alongside error %v", tc.in, out, err)
+				}
+			}
+		})
+	}
+}
+
+// TestFormat_InPlaceLeavesExplainUnchanged checks the end-to-end format -i
+// path: the file must be left untouched and counted as failed.
+func TestFormat_InPlaceLeavesExplainUnchanged(t *testing.T) {
+	const original = "EXPLAIN VERBOSE DELETE FROM users;\n"
+	path := filepath.Join(t.TempDir(), "r.sql")
+	if err := os.WriteFile(path, []byte(original), 0600); err != nil {
+		t.Fatal(err)
+	}
+	var out, errBuf bytes.Buffer
+	f := NewFormatter(&out, &errBuf, CLIFormatterOptions{InPlace: true, IndentSize: 2, Uppercase: true, Dialect: "postgresql"})
+	res, err := f.Format([]string{path})
+	if err != nil {
+		t.Fatalf("Format: %v", err)
+	}
+	if res.FailedFiles != 1 || res.FormattedFiles != 0 {
+		t.Fatalf("FailedFiles=%d FormattedFiles=%d, want 1 and 0 (stderr: %s)", res.FailedFiles, res.FormattedFiles, errBuf.String())
+	}
+	got, err := os.ReadFile(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if string(got) != original {
+		t.Fatalf("format -i rewrote the file:\n%s", got)
+	}
+}
+
+// TestStatementSegments covers the separator guard that refuses parses with
+// more statements than semicolon-separated segments in the input.
+func TestStatementSegments(t *testing.T) {
+	tests := []struct {
+		in   string
+		want int
+	}{
+		{"SELECT 1", 1},
+		{"SELECT 1;", 1},
+		{";;SELECT 1;;", 1},
+		{"SELECT 1; SELECT 2", 2},
+		{"SELECT 1;\n\nSELECT 2;\n", 2},
+		{"EXPLAIN VERBOSE DELETE FROM users;", 1},
+	}
+	for _, tc := range tests {
+		tkz, release, err := tokenizerForDialect("")
+		if err != nil {
+			t.Fatal(err)
+		}
+		tokens, err := tkz.TokenizeString(tc.in)
+		if err != nil {
+			release()
+			t.Fatalf("tokenize %q: %v", tc.in, err)
+		}
+		got := statementSegments(tokens)
+		release()
+		if got != tc.want {
+			t.Errorf("statementSegments(%q) = %d, want %d", tc.in, got, tc.want)
+		}
+	}
+}
+
+// TestFormatSQL_UnsupportedStatementMessage checks the refusal names the type.
+func TestFormatSQL_UnsupportedStatementMessage(t *testing.T) {
+	_, err := formatWith("DESCRIBE users", CLIFormatterOptions{Compact: true})
+	if err == nil || !strings.Contains(err.Error(), "DescribeStatement") {
+		t.Fatalf("want an error naming DescribeStatement, got %v", err)
+	}
+}