From e1983ef274a729a5f54b7dd1ed12f19430623bc8 Mon Sep 17 00:00:00 2001 From: h3n4l Date: Wed, 29 Jul 2026 18:23:05 +0800 Subject: [PATCH 1/3] feat: enforce single-statement contract in translator.Parse With omni's mongo.Parse now strict (any statement parse error fails the whole input), translator.Parse no longer silently ignores content after the first statement: extra statements are rejected with a ParseError instead of being dropped. Comment-only input remains a no-op. Also pins the BYT-9950 statement: createIndex with a constant arithmetic expireAfterSeconds (90 * 24 * 60 * 60) folds to an int32 TTL and creates the index end to end. Requires bumping github.com/bytebase/omni to a version with strict mongo.Parse (folding landed in omni#393); the bump lands in this PR once the omni change merges. Co-Authored-By: Claude Fable 5 --- admin_test.go | 45 ++++++++++++++ internal/translator/translator.go | 27 ++++++--- internal/translator/translator_test.go | 84 ++++++++++++++++++++++++++ 3 files changed, 149 insertions(+), 7 deletions(-) create mode 100644 internal/translator/translator_test.go diff --git a/admin_test.go b/admin_test.go index 7c76376..816963b 100644 --- a/admin_test.go +++ b/admin_test.go @@ -92,6 +92,51 @@ func TestCreateIndexWithOptions(t *testing.T) { }) } +// TestCreateIndexArithmeticTTL pins the BYT-9950 statement: a TTL index whose +// expireAfterSeconds is a constant arithmetic expression must be created with +// the folded value. +func TestCreateIndexArithmeticTTL(t *testing.T) { + testutil.RunOnAllDBs(t, func(t *testing.T, db testutil.TestDB) { + dbName := fmt.Sprintf("testdb_create_idx_ttl_%s", db.Name) + defer testutil.CleanupDatabase(t, db.Client, dbName) + + ctx := context.Background() + + collection := db.Client.Database(dbName).Collection("cs_customer_frequency") + _, err := collection.InsertOne(ctx, bson.M{"trans_date": 1}) + require.NoError(t, err) + + gc := gomongo.NewClient(db.Client) + + result, err := gc.Execute(ctx, dbName, `db.cs_customer_frequency.createIndex( + { trans_date: 1 }, + { expireAfterSeconds: 90 * 24 * 60 * 60, name: "cs_customer_frequency_idx2" } +);`) + require.NoError(t, err) + require.NotNil(t, result) + require.Equal(t, 1, len(result.Value)) + indexName, ok := result.Value[0].(string) + require.True(t, ok) + require.Equal(t, "cs_customer_frequency_idx2", indexName) + + // Verify the index exists with the folded TTL value 7776000. + cursor, err := collection.Indexes().List(ctx) + require.NoError(t, err) + var indexes []bson.M + require.NoError(t, cursor.All(ctx, &indexes)) + found := false + for _, idx := range indexes { + if idx["name"] == "cs_customer_frequency_idx2" { + found = true + ttl, ok := idx["expireAfterSeconds"] + require.True(t, ok, "expected expireAfterSeconds on index, got %v", idx) + require.EqualValues(t, 7776000, ttl) + } + } + require.True(t, found, "index cs_customer_frequency_idx2 not found in %v", indexes) + }) +} + func TestDropIndex(t *testing.T) { testutil.RunOnAllDBs(t, func(t *testing.T, db testutil.TestDB) { dbName := fmt.Sprintf("testdb_drop_idx_%s", db.Name) diff --git a/internal/translator/translator.go b/internal/translator/translator.go index e15a0b5..c66e296 100644 --- a/internal/translator/translator.go +++ b/internal/translator/translator.go @@ -5,10 +5,13 @@ import ( "github.com/bytebase/gomongo/types" "github.com/bytebase/omni/mongo" + "github.com/bytebase/omni/mongo/ast" "github.com/bytebase/omni/mongo/parser" ) -// Parse parses a MongoDB shell statement and returns the operation. +// Parse parses a single MongoDB shell statement and returns the operation. +// The input must contain exactly one executable statement: parse errors and +// extra statements are rejected rather than silently dropped. // // Input that contains no executable statements (e.g., only comments or // whitespace) is treated as a no-op: Parse returns an Operation with @@ -28,13 +31,23 @@ func Parse(statement string) (*Operation, error) { return nil, err } - // Find the first non-empty statement. + var node ast.Node for _, s := range stmts { - if !s.Empty() { - return translateNode(s.AST) + if s.Empty() { + continue } + if node != nil { + return nil, &ParseError{ + Line: s.Start.Line, + Column: s.Start.Column, + Message: "expected a single statement, got multiple", + } + } + node = s.AST } - - // Comment-only / whitespace-only input: no-op, no error. - return &Operation{OpType: types.OpNoOp}, nil + if node == nil { + // Comment-only / whitespace-only input: no-op, no error. + return &Operation{OpType: types.OpNoOp}, nil + } + return translateNode(node) } diff --git a/internal/translator/translator_test.go b/internal/translator/translator_test.go new file mode 100644 index 0000000..bed97c6 --- /dev/null +++ b/internal/translator/translator_test.go @@ -0,0 +1,84 @@ +package translator + +import ( + "errors" + "testing" + + "github.com/bytebase/gomongo/types" +) + +// TestParseSingleStatementContract pins the strict single-statement contract: +// parse errors and extra statements are rejected rather than silently dropped +// (BYT-9950: dropped statements made migrations appear to skip commands). +func TestParseSingleStatementContract(t *testing.T) { + t.Run("valid single statement", func(t *testing.T) { + op, err := Parse(`db.users.find({ name: "alice" })`) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op.OpType != types.OpFind { + t.Errorf("expected OpFind, got %v", op.OpType) + } + }) + + t.Run("comment-only input is a no-op", func(t *testing.T) { + op, err := Parse("// just a comment\n") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op.OpType != types.OpNoOp { + t.Errorf("expected OpNoOp, got %v", op.OpType) + } + }) + + t.Run("invalid statement is rejected", func(t *testing.T) { + _, err := Parse("db.users.find({ name: })") + var pe *ParseError + if !errors.As(err, &pe) { + t.Fatalf("expected *ParseError, got %T: %v", err, err) + } + }) + + t.Run("valid statement followed by invalid is rejected", func(t *testing.T) { + _, err := Parse("db.users.find();\nthis is not mongosh") + var pe *ParseError + if !errors.As(err, &pe) { + t.Fatalf("expected *ParseError, got %T: %v", err, err) + } + if pe.Line != 2 { + t.Errorf("expected error on line 2, got %d", pe.Line) + } + }) + + t.Run("multiple valid statements are rejected", func(t *testing.T) { + _, err := Parse("db.users.find();\ndb.orders.find();") + var pe *ParseError + if !errors.As(err, &pe) { + t.Fatalf("expected *ParseError, got %T: %v", err, err) + } + if pe.Line != 2 { + t.Errorf("expected error on line 2, got %d", pe.Line) + } + }) +} + +// TestParseCreateIndexArithmeticTTL pins the BYT-9950 statement: createIndex +// with a constant arithmetic expireAfterSeconds folds to an int32 TTL. +func TestParseCreateIndexArithmeticTTL(t *testing.T) { + op, err := Parse(`db.cs_customer_frequency.createIndex( + { trans_date: 1 }, + { expireAfterSeconds: 90 * 24 * 60 * 60, name: "cs_customer_frequency_idx2" } +);`) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if op.OpType != types.OpCreateIndex { + t.Fatalf("expected OpCreateIndex, got %v", op.OpType) + } + if op.IndexTTL == nil || *op.IndexTTL != 7776000 { + t.Errorf("expected IndexTTL 7776000, got %v", op.IndexTTL) + } + if op.IndexName != "cs_customer_frequency_idx2" { + t.Errorf("expected index name cs_customer_frequency_idx2, got %q", op.IndexName) + } +} From e877bf0b790351684bd164a65971b080ffd64dce Mon Sep 17 00:00:00 2001 From: h3n4l Date: Thu, 30 Jul 2026 10:21:02 +0800 Subject: [PATCH 2/3] chore: bump omni for strict mongo.Parse and arithmetic folding Picks up bytebase/omni#395 (Parse fails on any statement parse error) and bytebase/omni#393 (constant arithmetic folding), which the new single-statement contract and TTL-index tests rely on. Co-Authored-By: Claude Fable 5 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 693d6bf..b0d6c51 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/bytebase/gomongo go 1.25.7 require ( - github.com/bytebase/omni v0.0.0-20260509021101-01140a7b9722 + github.com/bytebase/omni v0.0.0-20260730021650-cd9ba7a3a4e5 github.com/google/uuid v1.6.0 github.com/stretchr/testify v1.11.1 github.com/testcontainers/testcontainers-go v0.41.0 diff --git a/go.sum b/go.sum index 2188140..7164484 100644 --- a/go.sum +++ b/go.sum @@ -6,8 +6,8 @@ github.com/Azure/go-ansiterm v0.0.0-20250102033503-faa5f7b0171c h1:udKWzYgxTojEK github.com/Azure/go-ansiterm v0.0.0-20250102033503-faa5f7b0171c/go.mod h1:xomTg63KZ2rFqZQzSB4Vz2SUXa1BpHTVz9L5PTmPC4E= github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY= github.com/Microsoft/go-winio v0.6.2/go.mod h1:yd8OoFMLzJbo9gZq8j5qaps8bJ9aShtEA8Ipt1oGCvU= -github.com/bytebase/omni v0.0.0-20260509021101-01140a7b9722 h1:zeMIExVxJVNoU3DZfrKrMMrdZTmUJ264bihZ3fDNcqo= -github.com/bytebase/omni v0.0.0-20260509021101-01140a7b9722/go.mod h1:EVG8nQbNPUnUBGKn6J0niSGOs3jgfKo3TciY22R4bfo= +github.com/bytebase/omni v0.0.0-20260730021650-cd9ba7a3a4e5 h1:1vn2vPAkDMjSVsXuqeVcNEwxpDlAVcA2Iw5Sra7egRY= +github.com/bytebase/omni v0.0.0-20260730021650-cd9ba7a3a4e5/go.mod h1:Gp5RN3FM07f9/FFOTZCeu8duTAIMeuinkjjBCrs2Dw4= github.com/cenkalti/backoff/v4 v4.3.0 h1:MyRJ/UdXutAwSAT+s3wNd7MfTIcy71VQueUuFK343L8= github.com/cenkalti/backoff/v4 v4.3.0/go.mod h1:Y3VNntkOUPxTVeUxJ/G5vcM//AlwfmyYozVcomhLiZE= github.com/cenkalti/backoff/v5 v5.0.3 h1:ZN+IMa753KfX5hd8vVaMixjnqRZ3y8CuJKRKj1xcsSM= From ae13cdf5d44c0fea749b4b83829ad4b1e0397dc6 Mon Sep 17 00:00:00 2001 From: h3n4l Date: Thu, 30 Jul 2026 10:53:02 +0800 Subject: [PATCH 3/3] fix: pin arithmetic TTL as a parse error after folding revert omni#393 (constant arithmetic folding) was reverted in omni#396: arithmetic expressions in mongosh statements are intentionally unsupported. The BYT-9950 tests now assert that translator.Parse and Execute return a ParseError instead of silently skipping the statement. Bumps omni past the revert. Co-Authored-By: Claude Fable 5 --- admin_test.go | 36 +++++--------------------- go.mod | 2 +- go.sum | 4 +-- internal/translator/translator_test.go | 21 +++++---------- 4 files changed, 16 insertions(+), 47 deletions(-) diff --git a/admin_test.go b/admin_test.go index 816963b..3856873 100644 --- a/admin_test.go +++ b/admin_test.go @@ -92,9 +92,9 @@ func TestCreateIndexWithOptions(t *testing.T) { }) } -// TestCreateIndexArithmeticTTL pins the BYT-9950 statement: a TTL index whose -// expireAfterSeconds is a constant arithmetic expression must be created with -// the folded value. +// TestCreateIndexArithmeticTTL pins the BYT-9950 statement: arithmetic +// expressions are not supported, and Execute must return a ParseError instead +// of silently skipping the statement. func TestCreateIndexArithmeticTTL(t *testing.T) { testutil.RunOnAllDBs(t, func(t *testing.T, db testutil.TestDB) { dbName := fmt.Sprintf("testdb_create_idx_ttl_%s", db.Name) @@ -102,38 +102,14 @@ func TestCreateIndexArithmeticTTL(t *testing.T) { ctx := context.Background() - collection := db.Client.Database(dbName).Collection("cs_customer_frequency") - _, err := collection.InsertOne(ctx, bson.M{"trans_date": 1}) - require.NoError(t, err) - gc := gomongo.NewClient(db.Client) - result, err := gc.Execute(ctx, dbName, `db.cs_customer_frequency.createIndex( + _, err := gc.Execute(ctx, dbName, `db.cs_customer_frequency.createIndex( { trans_date: 1 }, { expireAfterSeconds: 90 * 24 * 60 * 60, name: "cs_customer_frequency_idx2" } );`) - require.NoError(t, err) - require.NotNil(t, result) - require.Equal(t, 1, len(result.Value)) - indexName, ok := result.Value[0].(string) - require.True(t, ok) - require.Equal(t, "cs_customer_frequency_idx2", indexName) - - // Verify the index exists with the folded TTL value 7776000. - cursor, err := collection.Indexes().List(ctx) - require.NoError(t, err) - var indexes []bson.M - require.NoError(t, cursor.All(ctx, &indexes)) - found := false - for _, idx := range indexes { - if idx["name"] == "cs_customer_frequency_idx2" { - found = true - ttl, ok := idx["expireAfterSeconds"] - require.True(t, ok, "expected expireAfterSeconds on index, got %v", idx) - require.EqualValues(t, 7776000, ttl) - } - } - require.True(t, found, "index cs_customer_frequency_idx2 not found in %v", indexes) + var pe *gomongo.ParseError + require.ErrorAs(t, err, &pe) }) } diff --git a/go.mod b/go.mod index b0d6c51..c4935de 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/bytebase/gomongo go 1.25.7 require ( - github.com/bytebase/omni v0.0.0-20260730021650-cd9ba7a3a4e5 + github.com/bytebase/omni v0.0.0-20260730024920-8e82223f7635 github.com/google/uuid v1.6.0 github.com/stretchr/testify v1.11.1 github.com/testcontainers/testcontainers-go v0.41.0 diff --git a/go.sum b/go.sum index 7164484..d1509d9 100644 --- a/go.sum +++ b/go.sum @@ -6,8 +6,8 @@ github.com/Azure/go-ansiterm v0.0.0-20250102033503-faa5f7b0171c h1:udKWzYgxTojEK github.com/Azure/go-ansiterm v0.0.0-20250102033503-faa5f7b0171c/go.mod h1:xomTg63KZ2rFqZQzSB4Vz2SUXa1BpHTVz9L5PTmPC4E= github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY= github.com/Microsoft/go-winio v0.6.2/go.mod h1:yd8OoFMLzJbo9gZq8j5qaps8bJ9aShtEA8Ipt1oGCvU= -github.com/bytebase/omni v0.0.0-20260730021650-cd9ba7a3a4e5 h1:1vn2vPAkDMjSVsXuqeVcNEwxpDlAVcA2Iw5Sra7egRY= -github.com/bytebase/omni v0.0.0-20260730021650-cd9ba7a3a4e5/go.mod h1:Gp5RN3FM07f9/FFOTZCeu8duTAIMeuinkjjBCrs2Dw4= +github.com/bytebase/omni v0.0.0-20260730024920-8e82223f7635 h1:JBDU5vAImt6qBPy4bUDgWhuuc2NQzR6WTMtKRL51YjE= +github.com/bytebase/omni v0.0.0-20260730024920-8e82223f7635/go.mod h1:Gp5RN3FM07f9/FFOTZCeu8duTAIMeuinkjjBCrs2Dw4= github.com/cenkalti/backoff/v4 v4.3.0 h1:MyRJ/UdXutAwSAT+s3wNd7MfTIcy71VQueUuFK343L8= github.com/cenkalti/backoff/v4 v4.3.0/go.mod h1:Y3VNntkOUPxTVeUxJ/G5vcM//AlwfmyYozVcomhLiZE= github.com/cenkalti/backoff/v5 v5.0.3 h1:ZN+IMa753KfX5hd8vVaMixjnqRZ3y8CuJKRKj1xcsSM= diff --git a/internal/translator/translator_test.go b/internal/translator/translator_test.go index bed97c6..2192494 100644 --- a/internal/translator/translator_test.go +++ b/internal/translator/translator_test.go @@ -62,23 +62,16 @@ func TestParseSingleStatementContract(t *testing.T) { }) } -// TestParseCreateIndexArithmeticTTL pins the BYT-9950 statement: createIndex -// with a constant arithmetic expireAfterSeconds folds to an int32 TTL. +// TestParseCreateIndexArithmeticTTL pins the BYT-9950 statement: arithmetic +// expressions are not supported, and Parse must surface the error instead of +// silently dropping the statement. func TestParseCreateIndexArithmeticTTL(t *testing.T) { - op, err := Parse(`db.cs_customer_frequency.createIndex( + _, err := Parse(`db.cs_customer_frequency.createIndex( { trans_date: 1 }, { expireAfterSeconds: 90 * 24 * 60 * 60, name: "cs_customer_frequency_idx2" } );`) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if op.OpType != types.OpCreateIndex { - t.Fatalf("expected OpCreateIndex, got %v", op.OpType) - } - if op.IndexTTL == nil || *op.IndexTTL != 7776000 { - t.Errorf("expected IndexTTL 7776000, got %v", op.IndexTTL) - } - if op.IndexName != "cs_customer_frequency_idx2" { - t.Errorf("expected index name cs_customer_frequency_idx2, got %q", op.IndexName) + var pe *ParseError + if !errors.As(err, &pe) { + t.Fatalf("expected *ParseError, got %T: %v", err, err) } }