chore: Final pass addressing any outstanding comments (#87)

launchdarkly · Jul 11, 2024 · 495d100 · 495d100
1 parent 207e7dd
commit 495d100
Show file tree

Hide file tree

Showing 8 changed files with 191 additions and 224 deletions.
diff --git a/contract-tests/src/client_entity.rs b/contract-tests/src/client_entity.rs
@@ -1,6 +1,6 @@
 use futures::future::FutureExt;
 use launchdarkly_server_sdk::{
-    Context, ContextBuilder, ExecutionOrder, MigratorBuilder, MultiContextBuilder, Reference,
+    Context, ContextBuilder, MigratorBuilder, MultiContextBuilder, Reference,
 };
 use std::sync::Arc;
 use std::time::Duration;
@@ -243,22 +243,13 @@ impl ClientEntity {
 
                 let mut builder = MigratorBuilder::new(self.client.clone());
 
-                let execution_order = match params.read_execution_order.as_str() {
-                    "serial" => ExecutionOrder::Serial,
-                    "random" => ExecutionOrder::Random,
-                    _ => ExecutionOrder::Parallel,
-                };
-                let old_endpoint = params.old_endpoint.clone();
-
-                let new_endpoint = params.new_endpoint.clone();
-
                 builder = builder
-                    .read_execution_order(execution_order)
+                    .read_execution_order(params.read_execution_order)
                     .track_errors(params.track_errors)
                     .track_latency(params.track_latency)
                     .read(
                         |payload: &Option<String>| {
-                            let old_endpoint = old_endpoint.clone();
+                            let old_endpoint = params.old_endpoint.clone();
                             async move {
                                 let result = send_payload(&old_endpoint, payload.clone()).await;
                                 match result {
@@ -269,7 +260,7 @@ impl ClientEntity {
                             .boxed()
                         },
                         |payload| {
-                            let new_endpoint = new_endpoint.clone();
+                            let new_endpoint = params.new_endpoint.clone();
                             async move {
                                 let result = send_payload(&new_endpoint, payload.clone()).await;
                                 match result {
@@ -287,7 +278,7 @@ impl ClientEntity {
                     )
                     .write(
                         |payload| {
-                            let old_endpoint = old_endpoint.clone();
+                            let old_endpoint = params.old_endpoint.clone();
                             async move {
                                 let result = send_payload(&old_endpoint, payload.clone()).await;
                                 match result {
@@ -298,7 +289,7 @@ impl ClientEntity {
                             .boxed()
                         },
                         |payload| {
-                            let new_endpoint = new_endpoint.clone();
+                            let new_endpoint = params.new_endpoint.clone();
                             async move {
                                 let result = send_payload(&new_endpoint, payload.clone()).await;
                                 match result {
@@ -310,13 +301,13 @@ impl ClientEntity {
                         },
                     );
 
-                let migrator = builder.build().expect("builder failed");
+                let mut migrator = builder.build().expect("builder failed");
                 match params.operation {
                     launchdarkly_server_sdk::Operation::Read => {
                         let result = migrator
                             .read(
+                                &params.context,
                                 params.key,
-                                params.context,
                                 params.default_stage,
                                 params.payload,
                             )
@@ -334,8 +325,8 @@ impl ClientEntity {
                     launchdarkly_server_sdk::Operation::Write => {
                         let result = migrator
                             .write(
+                                &params.context,
                                 params.key,
-                                params.context,
                                 params.default_stage,
                                 params.payload,
                             )
@@ -350,7 +341,10 @@ impl ClientEntity {
                             MigrationOperationResponse { result: payload },
                         )))
                     }
-                    _ => todo!(),
+                    _ => Err(format!(
+                        "Invalid operation requested: {:?}",
+                        params.operation
+                    )),
                 }
             }
             command => Err(format!("Invalid command requested: {}", command)),

diff --git a/contract-tests/src/command_params.rs b/contract-tests/src/command_params.rs
@@ -1,5 +1,5 @@
 use launchdarkly_server_sdk::{
-    AttributeValue, Context, FlagDetail, FlagValue, Operation, Reason, Stage,
+    AttributeValue, Context, ExecutionOrder, FlagDetail, FlagValue, Operation, Reason, Stage,
 };
 use serde::{self, Deserialize, Serialize};
 use std::collections::HashMap;
@@ -157,7 +157,7 @@ pub struct MigrationOperationParams {
     pub key: String,
     pub context: Context,
     pub default_stage: Stage,
-    pub read_execution_order: String,
+    pub read_execution_order: ExecutionOrder,
     pub operation: Operation,
     pub old_endpoint: String,
     pub new_endpoint: String,

diff --git a/launchdarkly-server-sdk/src/client.rs b/launchdarkly-server-sdk/src/client.rs
@@ -641,7 +641,7 @@ impl Client {
         context: &Context,
         flag_key: &str,
         default_stage: Stage,
-    ) -> (Stage, MigrationOpTracker) {
+    ) -> (Stage, Arc<Mutex<MigrationOpTracker>>) {
         let (detail, flag) =
             self.variation_internal(context, flag_key, default_stage, &self.events_default);
 
@@ -655,7 +655,10 @@ impl Client {
             default_stage,
         );
 
-        (migration_detail.value.unwrap_or(default_stage), tracker)
+        (
+            migration_detail.value.unwrap_or(default_stage),
+            Arc::new(Mutex::new(tracker)),
+        )
     }
 
     /// Reports that a context has performed an event.
@@ -1741,7 +1744,7 @@ mod tests {
             )
             .expect("patch should apply");
 
-        let migrator = MigratorBuilder::new(client.clone())
+        let mut migrator = MigratorBuilder::new(client.clone())
             .read(
                 |_| async move { Ok(serde_json::Value::Null) }.boxed(),
                 |_| async move { Ok(serde_json::Value::Null) }.boxed(),
@@ -1761,17 +1764,17 @@ mod tests {
         if let Operation::Read = operation {
             migrator
                 .read(
+                    &context,
                     "stage-flag".into(),
-                    context,
                     Stage::Off,
                     serde_json::Value::Null,
                 )
                 .await;
         } else {
             migrator
                 .write(
+                    &context,
                     "stage-flag".into(),
-                    context,
                     Stage::Off,
                     serde_json::Value::Null,
                 )
@@ -1855,7 +1858,7 @@ mod tests {
             )
             .expect("patch should apply");
 
-        let migrator = MigratorBuilder::new(client.clone())
+        let mut migrator = MigratorBuilder::new(client.clone())
             .track_latency(true)
             .read(
                 |_| {
@@ -1900,17 +1903,17 @@ mod tests {
         if let Operation::Read = operation {
             migrator
                 .read(
+                    &context,
                     "stage-flag".into(),
-                    context,
                     Stage::Off,
                     serde_json::Value::Null,
                 )
                 .await;
         } else {
             migrator
                 .write(
+                    &context,
                     "stage-flag".into(),
-                    context,
                     Stage::Off,
                     serde_json::Value::Null,
                 )
@@ -1957,12 +1960,12 @@ mod tests {
             )
             .expect("patch should apply");
 
-        let migrator = MigratorBuilder::new(client.clone())
+        let mut migrator = MigratorBuilder::new(client.clone())
             .track_latency(true)
             .read(
                 |_| async move { Err("fail".into()) }.boxed(),
                 |_| async move { Err("fail".into()) }.boxed(),
-                Some(|_, _| true),
+                Some(|_: &String, _: &String| true),
             )
             .write(
                 |_| async move { Err("fail".into()) }.boxed(),
@@ -1977,8 +1980,8 @@ mod tests {
 
         migrator
             .read(
+                &context,
                 "stage-flag".into(),
-                context,
                 Stage::Off,
                 serde_json::Value::Null,
             )
@@ -2026,7 +2029,7 @@ mod tests {
             )
             .expect("patch should apply");
 
-        let migrator = MigratorBuilder::new(client.clone())
+        let mut migrator = MigratorBuilder::new(client.clone())
             .track_latency(true)
             .read(
                 |_| async move { Ok(serde_json::Value::Null) }.boxed(),
@@ -2046,8 +2049,8 @@ mod tests {
 
         migrator
             .write(
+                &context,
                 "stage-flag".into(),
-                context,
                 Stage::Off,
                 serde_json::Value::Null,
             )
@@ -2117,7 +2120,7 @@ mod tests {
             )
             .expect("patch should apply");
 
-        let migrator = MigratorBuilder::new(client.clone())
+        let mut migrator = MigratorBuilder::new(client.clone())
             .track_latency(true)
             .read(
                 |_| async move { Ok(serde_json::Value::Null) }.boxed(),
@@ -2155,8 +2158,8 @@ mod tests {
 
         migrator
             .write(
+                &context,
                 "stage-flag".into(),
-                context,
                 Stage::Off,
                 serde_json::Value::Null,
             )
@@ -2202,7 +2205,7 @@ mod tests {
             )
             .expect("patch should apply");
 
-        let migrator = MigratorBuilder::new(client.clone())
+        let mut migrator = MigratorBuilder::new(client.clone())
             .track_latency(true)
             .read(
                 |_| {
@@ -2246,8 +2249,8 @@ mod tests {
 
         migrator
             .read(
+                &context,
                 "stage-flag".into(),
-                context,
                 Stage::Off,
                 serde_json::Value::Null,
             )

diff --git a/launchdarkly-server-sdk/src/events/event.rs b/launchdarkly-server-sdk/src/events/event.rs
@@ -95,7 +95,7 @@ impl BaseEvent {
     }
 }
 
-/// MigrationOpEventData is generated through the migration op tracker provided through the SDK.
+/// A MigrationOpEvent is generated through the migration op tracker provided through the SDK.
 #[derive(Clone, Debug)]
 pub struct MigrationOpEvent {
     pub(crate) base: BaseEvent,
@@ -131,21 +131,17 @@ impl Serialize for MigrationOpEvent {
             key: self.key.clone(),
             value: self.evaluation.value,
             default: self.default_stage,
-            // QUESTION: In the ruby implementation, this can be nil. Why not here?
             reason: self.evaluation.reason.clone(),
             variation_index: self.evaluation.variation_index,
             version: self.version,
         };
         state.serialize_field("evaluation", &evaluation)?;
 
-        // TODO: Add sampling here if it is set and not 1
-
         let mut measurements = vec![];
         if !self.invoked.is_empty() {
             measurements.push(MigrationOpMeasurement::Invoked(&self.invoked));
         }
 
-        // TODO: There is something here to do with consistency check ratio
         if let Some(consistency_check) = self.consistency_check {
             measurements.push(MigrationOpMeasurement::ConsistencyCheck(
                 consistency_check,

diff --git a/launchdarkly-server-sdk/src/lib.rs b/launchdarkly-server-sdk/src/lib.rs
@@ -42,7 +42,7 @@ pub use feature_requester_builders::{
 };
 pub use launchdarkly_server_sdk_evaluation::{Flag, Segment, Versioned};
 pub use migrations::{
-    ExecutionOrder, MigrationOpTracker, MigratorBuilder, Operation, Origin, Stage,
+    ExecutionOrder, MigrationOpTracker, Migrator, MigratorBuilder, Operation, Origin, Stage,
 };
 pub use service_endpoints::ServiceEndpointsBuilder;
 pub use stores::persistent_store::{PersistentDataStore, PersistentStoreError};