From 2c3276ee27a2d6791bfb0e409e3f99d857367bab Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Wed, 23 Oct 2024 16:30:51 -0700 Subject: [PATCH] Bug fix for db.Ping(ctx) against a Doltgres server --- postgres/parser/parser/parse.go | 5 ++- postgres/parser/parser/sql/sql_parser.go | 2 +- testing/go/framework.go | 4 ++ testing/go/smoke_test.go | 12 ++++++ testing/go/sql_parser_test.go | 52 ++++++++++++++++++++++++ 5 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 testing/go/sql_parser_test.go diff --git a/postgres/parser/parser/parse.go b/postgres/parser/parser/parse.go index 24944e921e..b6e6b94268 100644 --- a/postgres/parser/parser/parse.go +++ b/postgres/parser/parser/parse.go @@ -38,6 +38,7 @@ import ( "strings" "github.com/cockroachdb/errors" + vitess "github.com/dolthub/vitess/go/vt/sqlparser" "github.com/dolthub/doltgresql/postgres/parser/pgcode" "github.com/dolthub/doltgresql/postgres/parser/pgerror" @@ -120,8 +121,10 @@ func (p *Parser) parseOneWithDepth(depth int, sql string) (Statement, error) { if err != nil { return Statement{}, err } - if len(stmts) != 1 { + if len(stmts) > 1 { return Statement{}, errors.AssertionFailedf("expected 1 statement, but found %d", len(stmts)) + } else if len(stmts) == 0 { + return Statement{}, vitess.ErrEmpty } return stmts[0], nil } diff --git a/postgres/parser/parser/sql/sql_parser.go b/postgres/parser/parser/sql/sql_parser.go index acc9be29c3..9a470efc7f 100644 --- a/postgres/parser/parser/sql/sql_parser.go +++ b/postgres/parser/parser/sql/sql_parser.go @@ -56,7 +56,7 @@ func (p *PostgresParser) ParseWithOptions(ctx context.Context, query string, del return nil, "", "", fmt.Errorf("only a single statement at a time is currently supported") } if len(stmts) == 0 { - return nil, q, "", nil + return nil, q, "", vitess.ErrEmpty } vitessAST, err := ast.Convert(stmts[0]) diff --git a/testing/go/framework.go b/testing/go/framework.go index 60bb2219e7..f3a8aafdfc 100644 --- a/testing/go/framework.go +++ b/testing/go/framework.go @@ -130,6 +130,7 @@ func RunScript(t *testing.T, script ScriptTest, normalizeRows bool) { Default: pgxConn, Current: pgxConn, } + require.NoError(t, pgxConn.Ping(ctx)) defer func() { conn.Close(ctx) }() @@ -309,6 +310,9 @@ func CreateServer(t *testing.T, database string) (context.Context, *Connection, conn, err := pgx.Connect(ctx, fmt.Sprintf("postgres://postgres:password@127.0.0.1:%d/%s", port, database)) require.NoError(t, err) + + // Ping the connection to test that it's alive, and also to test that Doltgres handles empty queries correctly + require.NoError(t, conn.Ping(ctx)) return ctx, &Connection{ Default: conn, Current: conn, diff --git a/testing/go/smoke_test.go b/testing/go/smoke_test.go index bad59ad287..9a89d7f11c 100644 --- a/testing/go/smoke_test.go +++ b/testing/go/smoke_test.go @@ -71,6 +71,18 @@ func TestSmokeTests(t *testing.T) { Query: "SELECT NULL = NULL", Expected: []sql.Row{{nil}}, }, + { + Query: ";", + Expected: []sql.Row{}, + }, + { + Query: " ; ", + Expected: []sql.Row{}, + }, + { + Query: "-- this is only a comment", + Expected: []sql.Row{}, + }, }, }, { diff --git a/testing/go/sql_parser_test.go b/testing/go/sql_parser_test.go new file mode 100644 index 0000000000..a8eb749217 --- /dev/null +++ b/testing/go/sql_parser_test.go @@ -0,0 +1,52 @@ +// Copyright 2024 Dolthub, Inc. +// +// 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 _go + +import ( + "context" + "testing" + + vitess "github.com/dolthub/vitess/go/vt/sqlparser" + "github.com/stretchr/testify/require" + + "github.com/dolthub/doltgresql/postgres/parser/parser/sql" +) + +// TestParserBehaviors tests behaviors, such as empty statement handling, for the Doltgres parser. +func TestParserBehaviors(t *testing.T) { + parser := sql.NewPostgresParser() + + // Parser implementations should return Vitess' ErrEmpty error for empty statements. + t.Run("empty statement parsing", func(t *testing.T) { + emptyStatements := []string{"", " ", "\t", ";", "-- comment", "/* comment */"} + for _, statement := range emptyStatements { + parsed, err := parser.ParseSimple(statement) + require.Nil(t, parsed) + require.ErrorIs(t, err, vitess.ErrEmpty) + + parsed, _, _, err = parser.Parse(nil, statement, true) + require.Nil(t, parsed) + require.ErrorIs(t, err, vitess.ErrEmpty) + + parsed, _, err = parser.ParseOneWithOptions(context.Background(), statement, vitess.ParserOptions{}) + require.Nil(t, parsed) + require.ErrorIs(t, err, vitess.ErrEmpty) + + parsed, _, _, err = parser.ParseWithOptions(context.Background(), statement, ';', false, vitess.ParserOptions{}) + require.Nil(t, parsed) + require.ErrorIs(t, err, vitess.ErrEmpty) + } + }) +}