Sfoglia il codice sorgente

fix(executor): reject non-constant DEFAULT expressions at DDL

The schema catalog stores the evaluated default rather than the expression,
so DEFAULT CURRENT_TIMESTAMP was frozen at CREATE TABLE time and every later
INSERT silently received the same value. Reject any default that is not a
literal (optionally signed, parenthesized, or cast) with SQLSTATE 0A000 and
tell the client to supply the value on INSERT.

Constant defaults keep their existing evaluation and storage behavior. The
GoatCounter rebuild-shape test now uses a constant default; a separate test
records that its real DEFAULT current_timestamp DDL is rejected.

Task: #57
PizzaSQL Bot 1 giorno fa
parent
commit
df8443a031

+ 53 - 0
pkg/executor/ddl_defaults.go

@@ -0,0 +1,53 @@
+package executor
+
+import (
+	"fmt"
+
+	"github.com/danfragoso/pizzasql-next/pkg/lexer"
+	"github.com/danfragoso/pizzasql-next/pkg/parser"
+	"github.com/danfragoso/pizzasql-next/pkg/pgerror"
+)
+
+// constantDefaultValue evaluates a DEFAULT expression that cannot depend on the
+// row, session clock, or other mutable state.
+//
+// The schema catalog stores the evaluated value, not the expression SQL, so a
+// non-constant default such as CURRENT_TIMESTAMP would be frozen at DDL time and
+// every later INSERT would silently receive the same value. Rejecting the
+// declaration is safer than accepting it with different semantics. Per-INSERT
+// evaluation is not implemented.
+func constantDefaultValue(e *Executor, column string, expr parser.Expr) (interface{}, error) {
+	if !isConstantExpr(expr) {
+		return nil, pgerror.Unsupported(
+			"DEFAULT for column %q is not a constant expression: %s; PizzaSQL stores the evaluated default in the schema catalog, so a non-constant default would be frozen at CREATE/ALTER time. Omit the default and supply the value on INSERT",
+			column, parser.FormatExpr(expr),
+		)
+	}
+	value, err := e.evalExpr(expr, nil)
+	if err != nil {
+		return nil, fmt.Errorf("evaluate DEFAULT for column %q: %w", column, err)
+	}
+	return value, nil
+}
+
+// isConstantExpr reports whether expr is a literal, a unary sign applied to a
+// literal, a parenthesized constant, or a cast of a constant. Everything else
+// (column references including CURRENT_TIMESTAMP, function calls, subqueries,
+// arithmetic) is rejected because its value can change between statements.
+func isConstantExpr(expr parser.Expr) bool {
+	switch ex := expr.(type) {
+	case *parser.LiteralExpr:
+		return true
+	case *parser.UnaryExpr:
+		if ex.Op != lexer.TokenMinus && ex.Op != lexer.TokenPlus {
+			return false
+		}
+		return isConstantExpr(ex.Operand)
+	case *parser.ParenExpr:
+		return isConstantExpr(ex.Expr)
+	case *parser.CastExpr:
+		return isConstantExpr(ex.Expr)
+	default:
+		return false
+	}
+}

+ 83 - 0
pkg/executor/ddl_defaults_test.go

@@ -0,0 +1,83 @@
+package executor
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/danfragoso/pizzasql-next/pkg/pgerror"
+)
+
+// Non-constant defaults cannot be stored in the schema catalog without freezing
+// their value at DDL time, so they must be rejected instead of silently
+// producing a constant.
+func TestCreateTableRejectsNonConstantDefaults(t *testing.T) {
+	_, schema, table := newTestDB(t)
+	e := newExec(schema, table)
+
+	for _, sql := range []string{
+		"CREATE TABLE t1 (id INTEGER PRIMARY KEY, created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP)",
+		"CREATE TABLE t2 (id INTEGER PRIMARY KEY, created_at TIMESTAMP DEFAULT datetime('now'))",
+		"CREATE TABLE t3 (id INTEGER PRIMARY KEY, token TEXT DEFAULT lower('X'))",
+	} {
+		if _, err := execSQL(e, sql); !errors.Is(err, pgerror.ErrFeatureNotSupported) {
+			t.Fatalf("%s: error = %v, want ErrFeatureNotSupported", sql, err)
+		}
+	}
+}
+
+func TestAlterTableRejectsNonConstantDefault(t *testing.T) {
+	_, schema, table := newTestDB(t)
+	e := newExec(schema, table)
+	execMust(t, e, "CREATE TABLE t (id INTEGER PRIMARY KEY)")
+
+	if _, err := execSQL(e, "ALTER TABLE t ADD COLUMN created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP"); !errors.Is(err, pgerror.ErrFeatureNotSupported) {
+		t.Fatalf("ALTER error = %v, want ErrFeatureNotSupported", err)
+	}
+}
+
+func TestConstantDefaultsStillApplyOnInsert(t *testing.T) {
+	_, schema, table := newTestDB(t)
+	e := newExec(schema, table)
+	execMust(t, e, "CREATE TABLE t (id INTEGER PRIMARY KEY, label TEXT DEFAULT 'new', score INTEGER DEFAULT -1, flag BOOLEAN DEFAULT false, note TEXT DEFAULT NULL)")
+
+	execMust(t, e, "INSERT INTO t (id) VALUES (1)")
+	res := execMust(t, e, "SELECT label, score, flag, note FROM t WHERE id = 1")
+	if res.RowCount != 1 {
+		t.Fatalf("row count = %d, want 1", res.RowCount)
+	}
+	if got := res.Rows[0][0]; got != "new" {
+		t.Fatalf("label = %v, want new", got)
+	}
+	if got := res.Rows[0][1]; got != int64(-1) {
+		t.Fatalf("score = %v (%T), want int64(-1)", got, got)
+	}
+	if got := res.Rows[0][2]; got != false {
+		t.Fatalf("flag = %v, want false", got)
+	}
+	if got := res.Rows[0][3]; got != nil {
+		t.Fatalf("note = %v, want NULL", got)
+	}
+}
+
+func TestAlterTableAddColumnDoesNotBackfill(t *testing.T) {
+	_, schema, table := newTestDB(t)
+	e := newExec(schema, table)
+	execMust(t, e, "CREATE TABLE t (id INTEGER PRIMARY KEY)")
+	execMust(t, e, "INSERT INTO t (id) VALUES (1)")
+
+	execMust(t, e, "ALTER TABLE t ADD COLUMN status TEXT DEFAULT 'pending'")
+
+	res := execMust(t, e, "SELECT id, status FROM t WHERE id = 1")
+	if res.RowCount != 1 {
+		t.Fatalf("row count = %d, want 1", res.RowCount)
+	}
+	if got := res.Rows[0][1]; got != nil {
+		t.Fatalf("existing row status = %v, want NULL (ADD COLUMN does not backfill)", got)
+	}
+
+	execMust(t, e, "INSERT INTO t (id) VALUES (2)")
+	res = execMust(t, e, "SELECT status FROM t WHERE id = 2")
+	if got := res.Rows[0][0]; got != "pending" {
+		t.Fatalf("new row status = %v, want pending", got)
+	}
+}

+ 8 - 2
pkg/executor/executor.go

@@ -3551,7 +3551,10 @@ func (e *Executor) executeCreateTable(stmt *parser.CreateTableStmt) (*Result, er
 				col.Nullable = false
 			case parser.ConstraintDefault:
 				if constraint.Default != nil {
-					val, _ := e.evalExpr(constraint.Default, nil)
+					val, err := constantDefaultValue(e, col.Name, constraint.Default)
+					if err != nil {
+						return nil, err
+					}
 					col.Default = val
 				}
 			case parser.ConstraintAutoIncrement:
@@ -3960,7 +3963,10 @@ func (e *Executor) executeAlterTableAddColumn(table string, action *parser.AddCo
 			col.Nullable = false
 		case parser.ConstraintDefault:
 			if constraint.Default != nil {
-				val, _ := e.evalExpr(constraint.Default, nil)
+				val, err := constantDefaultValue(e, col.Name, constraint.Default)
+				if err != nil {
+					return nil, err
+				}
 				col.Default = val
 			}
 		}

+ 27 - 1
pkg/executor/goatcounter_migration_test.go

@@ -1,7 +1,10 @@
 package executor
 
 import (
+	"errors"
 	"testing"
+
+	"github.com/danfragoso/pizzasql-next/pkg/pgerror"
 )
 
 // TestGoatCounterSQLiteMigrations runs the shapes of GoatCounter's SQLite
@@ -82,8 +85,12 @@ func TestGoatCounterMigrationRebuildShape(t *testing.T) {
 		user_id INTEGER PRIMARY KEY AUTOINCREMENT,
 		site_id INTEGER NOT NULL,
 		email VARCHAR NOT NULL,
-		last_report_at TIMESTAMP NOT NULL DEFAULT current_timestamp
+		last_report_at TIMESTAMP NOT NULL DEFAULT ''
 	)`)
+	// GoatCounter's real migration declares DEFAULT current_timestamp. That is
+	// rejected (see TestGoatCounterMigrationRejectsFrozenTimestampDefault), so
+	// this rebuild-shape test uses a constant default to keep exercising the
+	// copy/drop/rename/index steps.
 	execMust(t, e, `INSERT INTO users2 (user_id, site_id, email)
 		SELECT user_id, site_id, email FROM users`)
 	execMust(t, e, "DROP TABLE users")
@@ -101,6 +108,25 @@ func TestGoatCounterMigrationRebuildShape(t *testing.T) {
 	}
 }
 
+// TestGoatCounterMigrationRejectsFrozenTimestampDefault records the deliberate
+// break: the GoatCounter schema uses a non-constant timestamp default, which the
+// engine now rejects instead of freezing at DDL time. A fresh GoatCounter tenant
+// cannot be bootstrapped until that schema is changed or a legacy import mode is
+// designed.
+func TestGoatCounterMigrationRejectsFrozenTimestampDefault(t *testing.T) {
+	_, schema, table := newTestDB(t)
+	e := newExec(schema, table)
+	_, err := execSQL(e, `CREATE TABLE users2 (
+		user_id INTEGER PRIMARY KEY AUTOINCREMENT,
+		site_id INTEGER NOT NULL,
+		email VARCHAR NOT NULL,
+		last_report_at TIMESTAMP NOT NULL DEFAULT current_timestamp
+	)`)
+	if !errors.Is(err, pgerror.ErrFeatureNotSupported) {
+		t.Fatalf("error = %v, want ErrFeatureNotSupported", err)
+	}
+}
+
 func TestGoatCounterInsertWithSelectMigrationShape(t *testing.T) {
 	_, schema, table := newTestDB(t)
 	e := newExec(schema, table)

+ 28 - 0
pkg/pgerror/pgerror.go

@@ -0,0 +1,28 @@
+// Package pgerror holds SQLSTATE metadata and sentinel errors shared by the
+// parser, executor, and protocol servers.
+package pgerror
+
+import (
+	"errors"
+	"fmt"
+)
+
+// SQLSTATE codes used by PizzaSQL's protocol and HTTP surfaces.
+const (
+	CodeFeatureNotSupported  = "0A000"
+	CodeUniqueViolation      = "23505"
+	CodeSerializationFailure = "40001"
+	CodeSyntaxError          = "42601"
+	CodeInternalError        = "XX000"
+)
+
+// ErrFeatureNotSupported marks SQL that PizzaSQL intentionally rejects instead
+// of accepting with different semantics. Protocol layers map it to SQLSTATE
+// 0A000 (feature_not_supported) rather than a generic syntax or internal error.
+var ErrFeatureNotSupported = errors.New("feature not supported")
+
+// Unsupported returns an error that wraps ErrFeatureNotSupported with a
+// client-facing explanation and migration hint.
+func Unsupported(format string, args ...interface{}) error {
+	return fmt.Errorf("%w: %s", ErrFeatureNotSupported, fmt.Sprintf(format, args...))
+}

+ 27 - 2
pkg/pgserver/connection.go

@@ -18,6 +18,7 @@ import (
 	"github.com/danfragoso/pizzasql-next/pkg/executor"
 	"github.com/danfragoso/pizzasql-next/pkg/lexer"
 	"github.com/danfragoso/pizzasql-next/pkg/parser"
+	"github.com/danfragoso/pizzasql-next/pkg/pgerror"
 	"github.com/danfragoso/pizzasql-next/pkg/storage"
 )
 
@@ -306,7 +307,14 @@ func (c *Connection) handleQuery(msg *Message) error {
 	p := parser.New(l)
 	stmts, err := p.ParseMultiple()
 	if err != nil {
-		c.sendError("ERROR", ErrCodeSyntaxError, fmt.Sprintf("Syntax error: %v", err))
+		code := ErrCodeSyntaxError
+		if errors.Is(err, pgerror.ErrFeatureNotSupported) {
+			code = ErrCodeFeatureNotSupported
+			if c.txStatus == TxStatusInBlock {
+				c.txStatus = TxStatusFailed
+			}
+		}
+		c.sendError("ERROR", code, fmt.Sprintf("Syntax error: %v", err))
 		return c.sendReadyForQuery()
 	}
 
@@ -323,6 +331,11 @@ func (c *Connection) handleQuery(msg *Message) error {
 			if errors.Is(err, storage.ErrSerialization) {
 				code = ErrCodeSerializationFailure
 				c.txStatus = TxStatusIdle
+			} else if errors.Is(err, pgerror.ErrFeatureNotSupported) {
+				code = ErrCodeFeatureNotSupported
+				if c.txStatus == TxStatusInBlock {
+					c.txStatus = TxStatusFailed
+				}
 			} else if c.txStatus == TxStatusInBlock {
 				c.txStatus = TxStatusFailed
 			}
@@ -484,7 +497,14 @@ func (c *Connection) handleExecute(msg *Message) error {
 	l := lexer.New(portal.query)
 	stmt, err := parser.New(l).Parse()
 	if err != nil {
-		return c.failExtended(ErrCodeSyntaxError, fmt.Errorf("syntax error: %w", err))
+		code := ErrCodeSyntaxError
+		if errors.Is(err, pgerror.ErrFeatureNotSupported) {
+			code = ErrCodeFeatureNotSupported
+			if c.txStatus == TxStatusInBlock {
+				c.txStatus = TxStatusFailed
+			}
+		}
+		return c.failExtended(code, fmt.Errorf("syntax error: %w", err))
 	}
 	result, err := c.executor.Execute(stmt)
 	if err != nil {
@@ -492,6 +512,11 @@ func (c *Connection) handleExecute(msg *Message) error {
 		if errors.Is(err, storage.ErrSerialization) {
 			code = ErrCodeSerializationFailure
 			c.txStatus = TxStatusIdle
+		} else if errors.Is(err, pgerror.ErrFeatureNotSupported) {
+			code = ErrCodeFeatureNotSupported
+			if c.txStatus == TxStatusInBlock {
+				c.txStatus = TxStatusFailed
+			}
 		} else if c.txStatus == TxStatusInBlock {
 			c.txStatus = TxStatusFailed
 		}

+ 54 - 0
pkg/pgserver/unsupported_default_test.go

@@ -0,0 +1,54 @@
+package pgserver
+
+import (
+	"bufio"
+	"bytes"
+	"net"
+	"testing"
+)
+
+func runExtendedMessage(t *testing.T, c *Connection, client net.Conn, msg *Message) *Message {
+	t.Helper()
+	done := make(chan error, 1)
+	go func() {
+		done <- c.handleExecute(msg)
+	}()
+	m, err := ReadMessage(bufio.NewReader(client))
+	if err != nil {
+		t.Fatalf("read response: %v", err)
+	}
+	if err := <-done; err != nil {
+		t.Fatalf("handle message: %v", err)
+	}
+	return m
+}
+
+func TestNonConstantDefaultRejectedOverSimpleQuery(t *testing.T) {
+	c, client := newDataConnection(t)
+	msgs := runQuery(t, c, client, &Message{Type: MsgQuery, Data: append([]byte("CREATE TABLE t (id INTEGER PRIMARY KEY, created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP)"), 0)})
+	if len(msgs) != 2 {
+		t.Fatalf("got %d messages, want ErrorResponse+ReadyForQuery", len(msgs))
+	}
+	if code := errorCode(msgs[0]); code != ErrCodeFeatureNotSupported {
+		t.Fatalf("error code = %q, want %q", code, ErrCodeFeatureNotSupported)
+	}
+	if message := errorField(t, msgs[0], ErrorFieldMessage); !bytes.Contains([]byte(message), []byte("constant")) {
+		t.Fatalf("error message = %q, want an explanation about constant defaults", message)
+	}
+}
+
+func TestNonConstantDefaultRejectedOverExtendedQuery(t *testing.T) {
+	c, client := newDataConnection(t)
+
+	// Parse does not parse the SQL (it is parsed at Execute), so install the
+	// portal directly; this exercises the extended-protocol execution error path.
+	c.portals["p1"] = &portal{query: "CREATE TABLE t (id INTEGER PRIMARY KEY, created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP)"}
+
+	execute := NewMessageBuilder()
+	execute.WriteString("p1")
+	execute.WriteInt32(0)
+	errMsg := runExtendedMessage(t, c, client, &Message{Type: MsgExecute, Data: execute.Bytes()})
+	if code := errorCode(errMsg); code != ErrCodeFeatureNotSupported {
+		t.Fatalf("error code = %q, want %q", code, ErrCodeFeatureNotSupported)
+	}
+}