diff --git a/crates/engine/src/update.rs b/crates/engine/src/update.rs index 98094a69947..66b981f6a2b 100644 --- a/crates/engine/src/update.rs +++ b/crates/engine/src/update.rs @@ -199,16 +199,21 @@ fn auto_migrate_database( .clone(); // Convert `SequenceDef` min/max to `AlgebraicValue`s of the correct type. - let min = AlgebraicValue::from_i128(&ty, sequence_def.min_value.unwrap_or(1)).ok_or_else(|| { - anyhow::anyhow!("Precheck failed: added sequence {sequence_name} has invalid min value") - })?; - - let max = - AlgebraicValue::from_i128(&ty, sequence_def.max_value.unwrap_or(i128::MAX)).ok_or_else(|| { - anyhow::anyhow!("Precheck failed: added sequence {sequence_name} has invalid max value") + let min = ty + .saturating_value_from_i128(sequence_def.min_value.unwrap_or(1)) + .ok_or_else(|| { + anyhow::anyhow!("Precheck failed: added sequence {sequence_name} has invalid min value") })?; - let range = min..max; + let max = match sequence_def.max_value { + Some(max) => ty.saturating_value_from_i128(max), + None => ty.saturating_value_from_i128(i128::MAX), + } + .ok_or_else(|| { + anyhow::anyhow!("Precheck failed: added sequence {sequence_name} has invalid max value") + })?; + + let range = min..=max; if stdb .iter_by_col_range_mut(tx, table_id, sequence_def.column, range)? .next() @@ -988,6 +993,51 @@ mod test { Ok(()) } + #[test] + fn add_sequence_precheck_rejects_existing_column_max_value() -> anyhow::Result<()> { + let auth_ctx = AuthCtx::for_testing(); + let stdb = TestDB::durable()?; + + let module_v1: ModuleDef = { + let mut b = RawModuleDefV9Builder::new(); + b.build_table_with_new_type("seq_t", [("id", AlgebraicType::U8)], true) + .with_access(TableAccess::Public) + .finish(); + b.finish().try_into().expect("valid module v1") + }; + + let module_v2: ModuleDef = { + let mut b = RawModuleDefV9Builder::new(); + b.build_table_with_new_type("seq_t", [("id", AlgebraicType::U8)], true) + .with_column_sequence(0) + .with_access(TableAccess::Public) + .finish(); + b.finish().try_into().expect("valid module v2") + }; + + { + let mut tx = begin_mut_tx(&stdb); + for def in module_v1.tables() { + create_table_from_def(&stdb, &mut tx, &module_v1, def)?; + } + let table_id = stdb.table_id_from_name_mut(&tx, "seq_t")?.expect("seq_t should exist"); + insert(&stdb, &mut tx, table_id, &product![u8::MAX])?; + stdb.commit_tx(tx)?; + } + + let mut tx = begin_mut_tx(&stdb); + let plan = ponder_migrate(&module_v1, &module_v2)?; + let err = update_database(&stdb, &mut tx, auth_ctx, plan, &TestLogger) + .err() + .expect("adding a sequence over an existing max value should fail"); + assert!( + err.to_string().contains("already has values in range"), + "error should mention existing values in range, got: {err}" + ); + + Ok(()) + } + /// Verifies that `autoinc` sequence survives a schema migration that adds a column, /// and is also correctly persisted across database replay. /// diff --git a/crates/sats/src/algebraic_type.rs b/crates/sats/src/algebraic_type.rs index 6c2472f0bb1..cf9b76e50da 100644 --- a/crates/sats/src/algebraic_type.rs +++ b/crates/sats/src/algebraic_type.rs @@ -371,6 +371,35 @@ impl AlgebraicType { Self::deserialize(ValueDeserializer::from_ref(value)) } + /// Constructs an `AlgebraicValue` of this type from an `i128` + /// using saturating conversion. + /// + /// Returns `None` if this is not an integer type. + /// If `value` does not fit in this type, + /// it is saturated to the type's range. + /// Negative values convert to `0` for unsigned types. + pub fn saturating_value_from_i128(&self, value: i128) -> Option { + let signed = |min, max| value.clamp(min, max); + let unsigned = |max| value.clamp(0, max); + + match self { + Self::I8 => Some((signed(i8::MIN as i128, i8::MAX as i128) as i8).into()), + Self::I16 => Some((signed(i16::MIN as i128, i16::MAX as i128) as i16).into()), + Self::I32 => Some((signed(i32::MIN as i128, i32::MAX as i128) as i32).into()), + Self::I64 => Some((signed(i64::MIN as i128, i64::MAX as i128) as i64).into()), + Self::I128 => Some(value.into()), + Self::I256 => Some(i256::from(value).into()), + + Self::U8 => Some((unsigned(u8::MAX as i128) as u8).into()), + Self::U16 => Some((unsigned(u16::MAX as i128) as u16).into()), + Self::U32 => Some((unsigned(u32::MAX as i128) as u32).into()), + Self::U64 => Some((unsigned(u64::MAX as i128) as u64).into()), + Self::U128 => Some((value.max(0) as u128).into()), + Self::U256 => Some(u256::from(value.max(0) as u128).into()), + _ => None, + } + } + #[inline] /// Given an AlgebraicType, returns the min value for that type. pub fn min_value(&self) -> Option { diff --git a/crates/sats/src/algebraic_value.rs b/crates/sats/src/algebraic_value.rs index 3648ac99db0..1b0077278cc 100644 --- a/crates/sats/src/algebraic_value.rs +++ b/crates/sats/src/algebraic_value.rs @@ -306,30 +306,6 @@ impl AlgebraicValue { _ => false, } } - - /// Constructs an `AlgebraicValue` from an `i128` according to the given `AlgebraicType`. - /// - /// Returns `None` if the type is not a supported integer type. - pub fn from_i128(ty: &AlgebraicType, value: i128) -> Option { - let val = match ty { - AlgebraicType::I8 => (value as i8).into(), - AlgebraicType::I16 => (value as i16).into(), - AlgebraicType::I32 => (value as i32).into(), - AlgebraicType::I64 => (value as i64).into(), - AlgebraicType::I128 => value.into(), - AlgebraicType::I256 => i256::from(value).into(), - - AlgebraicType::U8 => (value as u8).into(), - AlgebraicType::U16 => (value as u16).into(), - AlgebraicType::U32 => (value as u32).into(), - AlgebraicType::U64 => (value as u64).into(), - AlgebraicType::U128 => (value as u128).into(), - AlgebraicType::U256 => (u256::from(value as u128)).into(), - - _ => return None, - }; - Some(val) - } } impl> From> for AlgebraicValue {