From f63a35ddab8fe336cae3be730c63491ff5702a6a Mon Sep 17 00:00:00 2001 From: Rajiv Ranganath Date: Tue, 19 Mar 2024 18:47:53 +0530 Subject: [PATCH] wip-118 Signed-off-by: Rajiv Ranganath --- fdb-rl/Cargo.toml | 16 ++-- .../protobuf/well_formed_dynamic_message.rs | 75 ++++++++++++++++--- .../well_formed_message_descriptor.rs | 2 +- 3 files changed, 74 insertions(+), 19 deletions(-) diff --git a/fdb-rl/Cargo.toml b/fdb-rl/Cargo.toml index 8c48ba9a5..b8a645628 100644 --- a/fdb-rl/Cargo.toml +++ b/fdb-rl/Cargo.toml @@ -21,14 +21,14 @@ bytes = "1" fdb = { git = "https://github.com/rajivr/fdb-wip", branch = "main" } fdb-rl-proto = { version = "0.1.0", path = "../fdb-rl-proto", default-features = false } num-bigint = "0.4" -partiql-parser = { git = "https://github.com/partiql/partiql-lang-rust.git", rev = "febf454500"} -partiql-catalog = { git = "https://github.com/partiql/partiql-lang-rust.git", rev = "febf454500"} -partiql-source-map = { git = "https://github.com/partiql/partiql-lang-rust.git", rev = "febf454500"} -partiql-ast = { git = "https://github.com/partiql/partiql-lang-rust.git", rev = "febf454500"} -partiql-logical-planner = { git = "https://github.com/partiql/partiql-lang-rust.git", rev = "febf454500"} -partiql-logical = { git = "https://github.com/partiql/partiql-lang-rust.git", rev = "febf454500"} -partiql-value = { git = "https://github.com/partiql/partiql-lang-rust.git", rev = "febf454500"} -partiql-eval = { git = "https://github.com/partiql/partiql-lang-rust.git", rev = "febf454500"} +partiql-parser = "0.7.1" +partiql-catalog = "0.7.1" +partiql-source-map = "0.7.1" +partiql-ast = "0.7.1" +partiql-logical-planner = "0.7.1" +partiql-logical = "0.7.1" +partiql-value = "0.7.1" +partiql-eval = "0.7.1" rust_decimal = { version = "1.25.0", default-features = false, features = ["std"] } # from `partiql-value` prost = "0.12" prost-reflect = "0.12" diff --git a/fdb-rl/src/protobuf/well_formed_dynamic_message.rs b/fdb-rl/src/protobuf/well_formed_dynamic_message.rs index fe608b0f1..56e13a0f6 100644 --- a/fdb-rl/src/protobuf/well_formed_dynamic_message.rs +++ b/fdb-rl/src/protobuf/well_formed_dynamic_message.rs @@ -26,7 +26,7 @@ use super::error::{ use super::{WellFormedMessageDescriptor, FDB_RL_WKT_V1_UUID}; /// Describes a valid `DynamicMessage`. -#[derive(Debug, PartialEq)] +#[derive(Clone, Debug, PartialEq)] pub(crate) struct WellFormedDynamicMessage { inner: DynamicMessage, } @@ -1178,6 +1178,36 @@ where .map(|_| dynamic_message) .map_err(|_| FdbError::new(PROTOBUF_ILL_FORMED_MESSAGE)) }) + .and_then(|dynamic_message| { + // Unlike Java RecordLayer [1], we no not allow the + // dynamic message to have unknown fields. + // + // In our case, when adding a new field, first the + // clients will need to be upgraded with the new + // field. + // + // Even though the field has been added to the client, + // its value *must* be `None`. If the client were to + // set a `Some(...)` value, that would result in + // unknown field in dynamic message, which is not + // allowed. + // + // Then using a transaction, the new field can be + // added to the metadata. At that point, all clients + // would need to refresh their metadata. + // + // We can then roll out the code that adds `Some(...)` + // value to the field, build indexes etc., + // + // We also never remove a field. + // + // [1] https://github.com/FoundationDB/fdb-record-layer/blob/3.4.473.0/docs/SchemaEvolution.md#add-a-field-to-an-existing-record-type + if dynamic_message.unknown_fields().count() == 0 { + Ok(dynamic_message) + } else { + Err(FdbError::new(PROTOBUF_ILL_FORMED_MESSAGE)) + } + }) .and_then(|dynamic_message| { // Check to make sure that any well known types // that might be inside dynamic message is well @@ -1764,12 +1794,6 @@ mod tests { hello_world: Some("hello_world".to_string()), }; - let well_formed_dynamic_message = WellFormedDynamicMessage::try_from(( - well_formed_message_descriptor, - &code_hello_world, - )) - .unwrap(); - // The `DynamicMessageFieldSet` would have unknown // fields [1] [2] for `hello_world`, as it is not // present in `MetadataHelloWorld` message @@ -1781,12 +1805,43 @@ mod tests { // }) // ``` // - // When the dynamic message is "merged" into - // `CodeHelloWorld`, then unknown field is - // properly resolved. + // We do not allow unknown fields in dynamic + // messages created using message descriptors + // stored in metadata. Therefore it results in an + // error. // // [1] https://protobuf.dev/programming-guides/proto3/#unknowns // [2] https://protobuf.dev/programming-guides/proto2/#unknowns + + let well_formed_dynamic_message = WellFormedDynamicMessage::try_from(( + well_formed_message_descriptor.clone(), + &code_hello_world, + )); + + assert_eq!( + Err(FdbError::new(PROTOBUF_ILL_FORMED_MESSAGE)), + well_formed_dynamic_message + ); + + // The correct way to this is to first set + // `hello_world` to `None` and then update the + // metadata. + + let code_hello_world = CodeHelloWorld { + primary_key: Some(FdbRLWktV1UuidProto::from( + Uuid::parse_str("ffffffff-ba5e-ba11-0000-00005ca1ab1e").unwrap(), + )), + hello: Some("hello".to_string()), + world: Some("world".to_string()), + hello_world: None, + }; + + let well_formed_dynamic_message = WellFormedDynamicMessage::try_from(( + well_formed_message_descriptor, + &code_hello_world, + )) + .unwrap(); + let dynamic_message = well_formed_dynamic_message.inner; assert_eq!( diff --git a/fdb-rl/src/protobuf/well_formed_message_descriptor.rs b/fdb-rl/src/protobuf/well_formed_message_descriptor.rs index 852c0b609..afaf0a9f2 100644 --- a/fdb-rl/src/protobuf/well_formed_message_descriptor.rs +++ b/fdb-rl/src/protobuf/well_formed_message_descriptor.rs @@ -63,7 +63,7 @@ struct NewEnumDescriptor(EnumDescriptor); /// If you have a value of type `WellFormedMessageDescriptor`, you can /// be sure that the provided `prost_reflect::MessageDescriptor` is /// well formed. -#[derive(Debug, PartialEq)] +#[derive(Clone, Debug, PartialEq)] pub(crate) struct WellFormedMessageDescriptor { inner: MessageDescriptor, }