fix(sql): reject procedural division/modulo by zero (#227)

The procedural executor's constant-expression evaluator — a separate,
sqlparser-based tree-walk evaluator used by trigger bodies and
EXCEPTION blocks, with no dependency on the DataFusion-backed
row-expression evaluator — still folded `/` and `%` by a zero divisor
to NULL instead of raising an error. A trigger computing
`NEW.ratio := NEW.numerator / NEW.denominator` would silently write
NULL when the denominator was zero instead of failing the insert.

try_eval_constant, eval_expr, and eval_binary_op now return
Result<Option<Value>, ConstEvalError> instead of Option<Value>, and
raise a division-by-zero signal only when a `/` or `%` reaches a zero
divisor; every other historically-NULL-folding case (unknown
identifiers, unsupported operators, overflow, non-finite float
results) keeps folding to None as before. The signal converts to a
new crate::Error::DivisionByZero variant, which pgwire renders as
SQLSTATE 22012 (division_by_zero), so trigger bodies can also match
it with EXCEPTION WHEN DIVISION_BY_ZERO.

Add the supporting DivisionByZero plumbing at the nodedb-types layer
(ErrorCode, ErrorDetails, msgpack wire tag, SQLSTATE constant,
NodeDbError constructor) and e2e coverage: a BEFORE INSERT trigger
dividing by zero rejects the insert without persisting the row, a
non-zero divisor still computes and persists normally, and an
EXCEPTION WHEN DIVISION_BY_ZERO handler catches the signal and lets
the insert proceed with its fallback assignment.

Author: emanzx

nodedb/src/control/planner/procedural/executor/eval.rs

@@ -12,6 +12,31 @@
 use crate::control::state::SharedState;
 use crate::types::TenantId;
 
+/// Signal from constant-expression evaluation that a `/` or `%` reached a
+/// zero divisor (nodedb issue #227).
+///
+/// Kept local to this module — the procedural executor's constant folder is
+/// a separate, sqlparser-based tree-walk evaluator with no dependency on
+/// `nodedb_query` (whose own `EvalError::DivisionByZero`, from the
+/// DataFusion-backed row-expression evaluator fixed under issue #216,
+/// covers a different code path entirely). Every other historically
+/// `None`-folding case (unknown identifiers, unsupported operators,
+/// overflow, non-finite float results) is unaffected and keeps folding to
+/// `Ok(None)` — see `try_eval_constant`, `evaluate_to_value`.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)]
+enum ConstEvalError {
+    #[error("division by zero")]
+    DivisionByZero,
+}
+
+impl From<ConstEvalError> for crate::Error {
+    fn from(e: ConstEvalError) -> Self {
+        match e {
+            ConstEvalError::DivisionByZero => Self::DivisionByZero,
+        }
+    }
+}
+
 /// Evaluate a SQL boolean condition.
 ///
 /// Fast path for constant TRUE/FALSE/1/0/NULL. Falls back to constant
@@ -25,7 +50,7 @@ pub async fn evaluate_condition(
         return Ok(result);
     }
 
-    match try_eval_constant(condition_sql) {
+    match try_eval_constant(condition_sql)? {
         Some(nodedb_types::Value::Bool(b)) => Ok(b),
         Some(nodedb_types::Value::Integer(i)) => Ok(i != 0),
         Some(nodedb_types::Value::Float(f)) => Ok(f != 0.0),
@@ -48,7 +73,7 @@ pub async fn evaluate_int(
         return Ok(v);
     }
 
-    match try_eval_constant(sql) {
+    match try_eval_constant(sql)? {
         Some(nodedb_types::Value::Integer(i)) => Ok(i),
         Some(nodedb_types::Value::Float(f)) => Ok(f as i64),
         _ => Err(crate::Error::PlanError {
@@ -87,7 +112,7 @@ pub async fn evaluate_to_value(
         return Ok(nodedb_types::Value::String(inner.replace("''", "'")));
     }
 
-    match try_eval_constant(trimmed) {
+    match try_eval_constant(trimmed)? {
         Some(val) => Ok(val),
         None => Ok(nodedb_types::Value::Null),
     }
@@ -97,7 +122,13 @@ pub async fn evaluate_to_value(
 ///
 /// Handles basic arithmetic (+, -, *, /), comparisons, boolean logic,
 /// and string concatenation on literal values.
-fn try_eval_constant(sql: &str) -> Option<nodedb_types::Value> {
+///
+/// Returns `Ok(None)` for anything this mini-evaluator doesn't handle
+/// (unparseable input, unknown identifiers, unsupported operators,
+/// overflow) — callers fold that to `Value::Null`, matching historical
+/// behavior. Returns `Err(ConstEvalError::DivisionByZero)` only when
+/// evaluation reaches a `/` or `%` with a zero divisor (nodedb issue #227).
+fn try_eval_constant(sql: &str) -> Result<Option<nodedb_types::Value>, ConstEvalError> {
     let trimmed = sql.trim();
     let expr_str = if trimmed.to_uppercase().starts_with("SELECT ") {
         trimmed[7..].trim()
@@ -108,24 +139,28 @@ fn try_eval_constant(sql: &str) -> Option<nodedb_types::Value> {
     let dialect = sqlparser::dialect::PostgreSqlDialect {};
     let select_sql = format!("SELECT {expr_str}");
     // reconstructed-sql: parser-only evaluates one constant-expression AST without dispatch
-    let stmts = sqlparser::parser::Parser::parse_sql(&dialect, &select_sql).ok()?;
-    let stmt = stmts.first()?;
+    let Ok(stmts) = sqlparser::parser::Parser::parse_sql(&dialect, &select_sql) else {
+        return Ok(None);
+    };
+    let Some(stmt) = stmts.first() else {
+        return Ok(None);
+    };
 
     if let sqlparser::ast::Statement::Query(query) = stmt
         && let sqlparser::ast::SetExpr::Select(select) = &*query.body
         && let Some(sqlparser::ast::SelectItem::UnnamedExpr(expr)) = select.projection.first()
     {
         return eval_expr(expr);
     }
-    None
+    Ok(None)
 }
 
 /// Recursively evaluate a sqlparser expression tree.
-fn eval_expr(expr: &sqlparser::ast::Expr) -> Option<nodedb_types::Value> {
+fn eval_expr(expr: &sqlparser::ast::Expr) -> Result<Option<nodedb_types::Value>, ConstEvalError> {
     use sqlparser::ast::{Expr, UnaryOperator, Value};
 
     match expr {
-        Expr::Value(v) => match &v.value {
+        Expr::Value(v) => Ok(match &v.value {
             Value::Number(n, _) => {
                 if let Ok(i) = n.parse::<i64>() {
                     Some(nodedb_types::Value::Integer(i))
@@ -139,94 +174,146 @@ fn eval_expr(expr: &sqlparser::ast::Expr) -> Option<nodedb_types::Value> {
             Value::Boolean(b) => Some(nodedb_types::Value::Bool(*b)),
             Value::Null => Some(nodedb_types::Value::Null),
             _ => None,
-        },
+        }),
         Expr::UnaryOp {
             op: UnaryOperator::Minus,
             expr: inner,
-        } => match eval_expr(inner)? {
-            nodedb_types::Value::Integer(i) => Some(nodedb_types::Value::Integer(-i)),
-            nodedb_types::Value::Float(f) => Some(nodedb_types::Value::Float(-f)),
+        } => Ok(match eval_expr(inner)? {
+            Some(nodedb_types::Value::Integer(i)) => Some(nodedb_types::Value::Integer(-i)),
+            Some(nodedb_types::Value::Float(f)) => Some(nodedb_types::Value::Float(-f)),
             _ => None,
-        },
+        }),
         Expr::UnaryOp {
             op: UnaryOperator::Not,
             expr: inner,
-        } => match eval_expr(inner)? {
-            nodedb_types::Value::Bool(b) => Some(nodedb_types::Value::Bool(!b)),
+        } => Ok(match eval_expr(inner)? {
+            Some(nodedb_types::Value::Bool(b)) => Some(nodedb_types::Value::Bool(!b)),
             _ => None,
+        }),
+        Expr::BinaryOp { left, op, right } => match (eval_expr(left)?, eval_expr(right)?) {
+            (Some(l), Some(r)) => eval_binary_op(&l, op, &r),
+            _ => Ok(None),
         },
-        Expr::BinaryOp { left, op, right } => {
-            let l = eval_expr(left)?;
-            let r = eval_expr(right)?;
-            eval_binary_op(&l, op, &r)
-        }
         Expr::Nested(inner) => eval_expr(inner),
-        _ => None,
+        _ => Ok(None),
     }
 }
 
 fn eval_binary_op(
     l: &nodedb_types::Value,
     op: &sqlparser::ast::BinaryOperator,
     r: &nodedb_types::Value,
-) -> Option<nodedb_types::Value> {
+) -> Result<Option<nodedb_types::Value>, ConstEvalError> {
     use nodedb_types::Value;
     use sqlparser::ast::BinaryOperator::*;
 
     match (l, r) {
         (Value::Integer(a), Value::Integer(b)) => match op {
-            Plus => Some(Value::Integer(a.checked_add(*b)?)),
-            Minus => Some(Value::Integer(a.checked_sub(*b)?)),
-            Multiply => Some(Value::Integer(a.checked_mul(*b)?)),
-            Divide if *b != 0 => Some(Value::Integer(a.checked_div(*b)?)),
-            Modulo if *b != 0 => Some(Value::Integer(a.checked_rem(*b)?)),
-            Gt => Some(Value::Bool(a > b)),
-            GtEq => Some(Value::Bool(a >= b)),
-            Lt => Some(Value::Bool(a < b)),
-            LtEq => Some(Value::Bool(a <= b)),
-            Eq => Some(Value::Bool(a == b)),
-            NotEq => Some(Value::Bool(a != b)),
-            _ => None,
+            Plus => Ok(a.checked_add(*b).map(Value::Integer)),
+            Minus => Ok(a.checked_sub(*b).map(Value::Integer)),
+            Multiply => Ok(a.checked_mul(*b).map(Value::Integer)),
+            Divide if *b != 0 => Ok(a.checked_div(*b).map(Value::Integer)),
+            Divide => Err(ConstEvalError::DivisionByZero),
+            Modulo if *b != 0 => Ok(a.checked_rem(*b).map(Value::Integer)),
+            Modulo => Err(ConstEvalError::DivisionByZero),
+            Gt => Ok(Some(Value::Bool(a > b))),
+            GtEq => Ok(Some(Value::Bool(a >= b))),
+            Lt => Ok(Some(Value::Bool(a < b))),
+            LtEq => Ok(Some(Value::Bool(a <= b))),
+            Eq => Ok(Some(Value::Bool(a == b))),
+            NotEq => Ok(Some(Value::Bool(a != b))),
+            _ => Ok(None),
         },
         (Value::Float(a), Value::Float(b)) => match op {
-            Plus => Some(Value::Float(a + b)),
-            Minus => Some(Value::Float(a - b)),
-            Multiply => Some(Value::Float(a * b)),
-            Divide if *b != 0.0 && b.is_finite() && *b != -0.0 => {
+            Plus => Ok(Some(Value::Float(a + b))),
+            Minus => Ok(Some(Value::Float(a - b))),
+            Multiply => Ok(Some(Value::Float(a * b))),
+            // Non-finite divisor (NaN/Infinity) is unsupported input, not a
+            // zero-divisor — keeps folding to `None` as before.
+            Divide if !b.is_finite() => Ok(None),
+            Divide if *b == 0.0 => Err(ConstEvalError::DivisionByZero),
+            Divide => {
                 let result = a / b;
                 if result.is_finite() {
-                    Some(Value::Float(result))
+                    Ok(Some(Value::Float(result)))
                 } else {
-                    None
+                    Ok(None)
                 }
             }
-            Gt => Some(Value::Bool(a > b)),
-            GtEq => Some(Value::Bool(a >= b)),
-            Lt => Some(Value::Bool(a < b)),
-            LtEq => Some(Value::Bool(a <= b)),
-            Eq => Some(Value::Bool(a == b)),
-            NotEq => Some(Value::Bool(a != b)),
-            _ => None,
+            Gt => Ok(Some(Value::Bool(a > b))),
+            GtEq => Ok(Some(Value::Bool(a >= b))),
+            Lt => Ok(Some(Value::Bool(a < b))),
+            LtEq => Ok(Some(Value::Bool(a <= b))),
+            Eq => Ok(Some(Value::Bool(a == b))),
+            NotEq => Ok(Some(Value::Bool(a != b))),
+            _ => Ok(None),
         },
         (Value::Integer(a), Value::Float(b)) => {
             eval_binary_op(&Value::Float(*a as f64), op, &Value::Float(*b))
         }
         (Value::Float(a), Value::Integer(b)) => {
             eval_binary_op(&Value::Float(*a), op, &Value::Float(*b as f64))
         }
-        (Value::Bool(a), Value::Bool(b)) => match op {
+        (Value::Bool(a), Value::Bool(b)) => Ok(match op {
             And => Some(Value::Bool(*a && *b)),
             Or => Some(Value::Bool(*a || *b)),
             Eq => Some(Value::Bool(a == b)),
             NotEq => Some(Value::Bool(a != b)),
             _ => None,
-        },
-        (Value::String(a), Value::String(b)) => match op {
+        }),
+        (Value::String(a), Value::String(b)) => Ok(match op {
             StringConcat => Some(Value::String(format!("{a}{b}"))),
             Eq => Some(Value::Bool(a == b)),
             NotEq => Some(Value::Bool(a != b)),
             _ => None,
-        },
-        _ => None,
+        }),
+        _ => Ok(None),
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    // ── Division/modulo by zero (nodedb issue #227) ─────────────────────────
+
+    #[test]
+    fn integer_division_by_zero_signals_error() {
+        assert_eq!(
+            try_eval_constant("10 / 0"),
+            Err(ConstEvalError::DivisionByZero)
+        );
+    }
+
+    #[test]
+    fn integer_modulo_by_zero_signals_error() {
+        assert_eq!(
+            try_eval_constant("10 % 0"),
+            Err(ConstEvalError::DivisionByZero)
+        );
+    }
+
+    #[test]
+    fn float_division_by_zero_signals_error() {
+        assert_eq!(
+            try_eval_constant("10.0 / 0.0"),
+            Err(ConstEvalError::DivisionByZero)
+        );
+    }
+
+    /// Control: a non-zero divisor still computes normally.
+    #[test]
+    fn integer_division_by_nonzero_still_computes() {
+        assert_eq!(
+            try_eval_constant("10 / 2"),
+            Ok(Some(nodedb_types::Value::Integer(5)))
+        );
+    }
+
+    /// Control: an unrelated `None` case (unknown identifier) is unaffected
+    /// by the division-by-zero signal and still folds to `Ok(None)`.
+    #[test]
+    fn unknown_identifier_still_folds_to_none() {
+        assert_eq!(try_eval_constant("some_unbound_identifier"), Ok(None));
     }
 }