Declare a Feature trait, re-implement FeatureProxy (#26)

* A new `Feature` trait that is implemented by our current `Feature` and
`FeatureProxy`.
* Rename our current `Feature` to `FeatureSnapshot`, to make it clear it
will always return the same evaluation
 * Reimplement `FeatureProxy` making it much straightforward

    ```rust
    pub struct FeatureProxy<'a> {
        client: &'a AppConfigurationClient,
        feature_id: String,
    }
    ```

with this approach, every method can be implemented in terms of our
current `Feature`: we just return a regular feature (with the
configuration at that time) and call the method on it.

    ```rust
fn get_value(&self, entity: &impl Entity) ->
crate::errors::Result<super::value::Value> {
            self.client.get_feature(&self.feature_id)?.get_value(entity)
        }
    ```

Author: rainerschoe

examples/demo.rs

@@ -14,7 +14,7 @@
 
 use std::{collections::HashMap, env, thread, time::Duration};
 
-use appconfiguration_rust_sdk::{client::AppConfigurationClient, AttrValue, Entity};
+use appconfiguration_rust_sdk::{client::AppConfigurationClient, AttrValue, Entity, Feature};
 use dotenvy::dotenv;
 use std::error::Error;
 
@@ -69,13 +69,13 @@ fn main() -> Result<()> {
 
         match client.get_feature_proxy(&feature_id) {
             Ok(feature) => {
-                println!("Feature name: {}", feature.get_name());
+                println!("Feature name: {}", feature.get_name()?);
                 println!("Feature id: {}", feature.get_id());
-                println!("Feature data type: {}", feature.get_data_type());
-                println!("Is feature enabled: {}", feature.is_enabled());
+                println!("Feature data type: {}", feature.get_data_type()?);
+                println!("Is feature enabled: {}", feature.is_enabled()?);
                 println!(
-                    "Feature evaluated value is: {}",
-                    feature.get_current_value(&entity)
+                    "Feature evaluated value is: {:?}",
+                    feature.get_value(&entity)?
                 );
             }
             Err(error) => {

src/client/app_configuration_client.rs

@@ -13,7 +13,7 @@
 // limitations under the License.
 
 use crate::client::cache::ConfigurationSnapshot;
-use crate::client::feature::Feature;
+use crate::client::feature_snapshot::FeatureSnapshot;
 pub use crate::client::feature_proxy::FeatureProxy;
 use crate::client::http;
 use crate::client::property::Property;
@@ -184,7 +184,7 @@ impl AppConfigurationClient {
             .collect())
     }
 
-    pub fn get_feature(&self, feature_id: &str) -> Result<Feature> {
+    pub fn get_feature(&self, feature_id: &str) -> Result<FeatureSnapshot> {
         let config_snapshot = self.latest_config_snapshot.lock()?;
 
         // Get the feature from the snapshot
@@ -221,21 +221,18 @@ impl AppConfigurationClient {
             segments
         };
 
-        Ok(Feature::new(feature.clone(), segments))
+        Ok(FeatureSnapshot::new(feature.clone(), segments))
     }
 
     /// Searches for the feature `feature_id` inside the current configured
     /// collection, and environment.
     ///
     /// Return `Ok(feature)` if the feature exists or `Err` if it does not.
-    pub fn get_feature_proxy(&self, feature_id: &str) -> Result<FeatureProxy> {
+    pub fn get_feature_proxy<'a>(&'a self, feature_id: &str) -> Result<FeatureProxy<'a>> {
         // FIXME: there is and was no validation happening if the feature exists.
         // Comments and error messages in FeatureProxy suggest that this should happen here.
         // same applies for properties.
-        Ok(FeatureProxy::new(
-            self.latest_config_snapshot.clone(),
-            feature_id.to_string(),
-        ))
+        Ok(FeatureProxy::new(self, feature_id.to_string()))
     }
 
     pub fn get_property_ids(&self) -> Result<Vec<String>> {

src/client/feature_proxy.rs

@@ -12,204 +12,56 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::sync::Arc;
-use std::{io::Cursor, sync::Mutex};
+use std::io::Cursor;
 
 use murmur3::murmur3_32;
 
-use crate::{
-    client::cache::ConfigurationSnapshot, models,
-    segment_evaluation::find_applicable_segment_rule_for_entity,
-};
-
 use crate::entity::Entity;
+use crate::Feature;
 
-use crate::errors::ConfigurationAccessError;
-
-const MISSING_FEATURE_ERROR_MSG: &str = "The feature should exist in the configuration_snapshot. It should have been validated in `AppConfigurationClient::get_feature()`.";
+use super::feature_snapshot::FeatureSnapshot;
+use super::AppConfigurationClient;
 
-/// A feature in a collection and environment. Use the `get_feature()`
-/// method of the `AppConfigurationClient` to create instances of features.
-#[derive(Debug)]
-pub struct FeatureProxy {
-    configuration_snapshot: Arc<Mutex<ConfigurationSnapshot>>,
+pub struct FeatureProxy<'a> {
+    client: &'a AppConfigurationClient,
     feature_id: String,
 }
 
-impl FeatureProxy {
-    pub(crate) fn new(
-        configuration_snapshot: Arc<Mutex<ConfigurationSnapshot>>,
-        feature_id: String,
-    ) -> Self {
-        FeatureProxy {
-            configuration_snapshot,
-            feature_id,
-        }
+impl<'a> FeatureProxy<'a> {
+    pub(crate) fn new(client: &'a AppConfigurationClient, feature_id: String) -> Self {
+        Self { client, feature_id }
     }
 
-    /// Returns the name of the feature.
-    pub fn get_name(&self) -> String {
-        self.configuration_snapshot
-            .lock()
-            .unwrap_or_else(|_| panic!("{}", ConfigurationAccessError::LockAcquisitionError))
-            .get_feature(&self.feature_id)
-            .expect(MISSING_FEATURE_ERROR_MSG)
-            .name
-            .clone()
-    }
-
-    /// Returns the disable value as a `models::ConfigValue`.
-    pub fn get_disabled_value(&self) -> models::ConfigValue {
-        self.configuration_snapshot
-            .lock()
-            .unwrap_or_else(|_| panic!("{}", ConfigurationAccessError::LockAcquisitionError))
-            .get_feature(&self.feature_id)
-            .expect(MISSING_FEATURE_ERROR_MSG)
-            .disabled_value
-            .clone()
-    }
-
-    /// Returns the enabled value as a `models::ConfigValue`.
-    pub fn get_enabled_value(&self) -> models::ConfigValue {
-        self.configuration_snapshot
-            .lock()
-            .unwrap_or_else(|_| panic!("{}", ConfigurationAccessError::LockAcquisitionError))
-            .get_feature(&self.feature_id)
-            .expect(MISSING_FEATURE_ERROR_MSG)
-            .enabled_value
-            .clone()
-    }
-
-    /// Returns the id of the feature.
-    pub fn get_id(&self) -> String {
-        self.configuration_snapshot
-            .lock()
-            .unwrap_or_else(|_| panic!("{}", ConfigurationAccessError::LockAcquisitionError))
-            .get_feature(&self.feature_id)
-            .expect(MISSING_FEATURE_ERROR_MSG)
-            .feature_id
-            .clone()
-    }
-
-    /// Returns the data type as a member of the `models::ValueKind` enumeration.
-    pub fn get_data_type(&self) -> models::ValueKind {
-        self.configuration_snapshot
-            .lock()
-            .unwrap_or_else(|_| panic!("{}", ConfigurationAccessError::LockAcquisitionError))
-            .get_feature(&self.feature_id)
-            .expect(MISSING_FEATURE_ERROR_MSG)
-            .kind
-    }
-
-    /// Gets the `Some(data_format)` if the feature data type is
-    /// `models::ValueKind::STRING`, or `None` otherwise.
-    pub fn get_data_format(&self) -> Option<String> {
-        self.configuration_snapshot
-            .lock()
-            .unwrap_or_else(|_| panic!("{}", ConfigurationAccessError::LockAcquisitionError))
-            .get_feature(&self.feature_id)
-            .expect(MISSING_FEATURE_ERROR_MSG)
-            .format
-            .clone()
-    }
-
-    /// Returns the rollout peArcentage as a positive integer.
-    pub fn get_rollout_percentage(&self) -> u32 {
-        self.configuration_snapshot
-            .lock()
-            .unwrap_or_else(|_| panic!("{}", ConfigurationAccessError::LockAcquisitionError))
-            .get_feature(&self.feature_id)
-            .expect(MISSING_FEATURE_ERROR_MSG)
-            .rollout_percentage
+    pub fn snapshot(&self) -> crate::errors::Result<FeatureSnapshot> {
+        self.client.get_feature(&self.feature_id)
     }
+}
 
-    /// Returns the targeting rules for the feature. I.e.: what value to
-    /// associate with an entity, under what ciArcumnstances, and how frequent
-    /// it applies.
-    pub fn get_targeting_rules(&self) -> Vec<models::TargetingRule> {
-        self.configuration_snapshot
-            .lock()
-            .unwrap_or_else(|_| panic!("{}", ConfigurationAccessError::LockAcquisitionError))
-            .get_feature(&self.feature_id)
-            .expect(MISSING_FEATURE_ERROR_MSG)
-            .segment_rules
-            .clone()
+impl<'a> Feature for FeatureProxy<'a> {
+    fn get_id(&self) -> &str {
+        &self.feature_id
     }
 
-    /// Returns if the feature is enabled or not.
-    pub fn is_enabled(&self) -> bool {
-        self.configuration_snapshot
-            .lock()
-            .unwrap_or_else(|_| panic!("{}", ConfigurationAccessError::LockAcquisitionError))
-            .get_feature(&self.feature_id)
-            .expect(MISSING_FEATURE_ERROR_MSG)
-            .enabled
+    fn get_name(&self) -> crate::errors::Result<String> {
+        self.client.get_feature(&self.feature_id)?.get_name()
     }
 
-    /// Evaluates the feature for `entity` and returns the evaluation as a
-    /// `models::ConfigValue`.
-    pub fn get_current_value(&self, entity: &impl Entity) -> models::ConfigValue {
-        if !self.is_enabled() {
-            self.get_disabled_value()
-        } else {
-            self.evaluate_feature_for_entity(entity)
-        }
+    fn get_data_type(&self) -> crate::errors::Result<crate::models::ValueKind> {
+        self.client.get_feature(&self.feature_id)?.get_data_type()
     }
 
-    fn evaluate_feature_for_entity(&self, entity: &impl Entity) -> models::ConfigValue {
-        let tag = format!("{}:{}", entity.get_id(), self.get_id());
-
-        if self.get_targeting_rules().len() == 0 && entity.get_attributes().len() == 0 {
-            // TODO rollout percentage evaluation
-        }
-
-        let segment_rule = find_applicable_segment_rule_for_entity(
-            &self
-                .configuration_snapshot
-                .lock()
-                .unwrap_or_else(|e| panic!("Failed to acquire configuration snapshot lock: {e}"))
-                .segments,
-            self.get_targeting_rules().into_iter(),
-            entity,
-        )
-        .unwrap_or_else(|e| panic!("Failed to evaluate segment rules: {e}"));
-        if let Some(segment_rule) = segment_rule {
-            let rollout_percentage = self.resolve_rollout_percentage(&segment_rule);
-            if rollout_percentage == 100 || random_value(&tag) < rollout_percentage {
-                self.resolve_enabled_value(&segment_rule)
-            } else {
-                self.get_disabled_value()
-            }
-        } else {
-            let rollout_percentage = self.get_rollout_percentage();
-            if rollout_percentage == 100 || random_value(&tag) < rollout_percentage {
-                self.get_enabled_value()
-            } else {
-                self.get_disabled_value()
-            }
-        }
+    fn is_enabled(&self) -> crate::errors::Result<bool> {
+        self.client.get_feature(&self.feature_id)?.is_enabled()
     }
 
-    fn resolve_rollout_percentage(&self, segment_rule: &models::TargetingRule) -> u32 {
-        let missing_rollout_msg = "Rollout velue is missing.";
-        let rollout_value = segment_rule
-            .rollout_percentage
-            .as_ref()
-            .expect(missing_rollout_msg);
-        if rollout_value.is_default() {
-            self.get_rollout_percentage()
-        } else {
-            u32::try_from(rollout_value.as_u64().expect(missing_rollout_msg))
-                .expect("Invalid rollout value.")
-        }
+    fn get_enabled_value(&self) -> crate::errors::Result<crate::models::ConfigValue> {
+        self.client
+            .get_feature(&self.feature_id)?
+            .get_enabled_value()
     }
 
-    fn resolve_enabled_value(&self, segment_rule: &models::TargetingRule) -> models::ConfigValue {
-        if segment_rule.value.is_default() {
-            self.get_enabled_value()
-        } else {
-            segment_rule.value.clone()
-        }
+    fn get_value(&self, entity: &impl Entity) -> crate::errors::Result<super::value::Value> {
+        self.client.get_feature(&self.feature_id)?.get_value(entity)
     }
 }