chore: Improve naming

- rename cache.rs to configuration.rs
- rename ConfigurationSnapshot to Configuration. Snapshot did not add
  meaningful information.
- Rename models::Configuration to ConfigurationJson to make clear
  this is the format used for data transport
- Add some comments to make ambiguities more clear
- Add some comments to start discussion with AppConfig Server team
  about some naming issues.

Signed-off-by: Rainer Schoenberger <[email protected]>

Author: rainerschoe

src/client/app_configuration_http.rs

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use crate::client::cache::ConfigurationSnapshot;
+use crate::client::configuration::Configuration;
 pub use crate::client::feature_proxy::FeatureProxy;
 use crate::client::feature_snapshot::FeatureSnapshot;
 pub use crate::client::property_proxy::PropertyProxy;
@@ -33,7 +33,7 @@ use super::{AppConfigurationClient, ConfigurationId};
 /// AppConfiguration client implementation that connects to a server
 #[derive(Debug)]
 pub struct AppConfigurationClientHttp {
-    latest_config_snapshot: Arc<Mutex<ConfigurationSnapshot>>,
+    latest_config_snapshot: Arc<Mutex<Configuration>>,
     _thread_terminator: std::sync::mpsc::Sender<()>,
 }
 
@@ -56,7 +56,7 @@ impl AppConfigurationClientHttp {
         let server_client = ServerClientImpl::new(service_address, token_provider)?;
 
         // Populate initial configuration
-        let latest_config_snapshot: Arc<Mutex<ConfigurationSnapshot>> = Arc::new(Mutex::new(
+        let latest_config_snapshot: Arc<Mutex<Configuration>> = Arc::new(Mutex::new(
             Self::get_configuration_snapshot(&server_client, &configuration_id)?,
         ));
 
@@ -78,16 +78,16 @@ impl AppConfigurationClientHttp {
     fn get_configuration_snapshot(
         server_client: &ServerClientImpl,
         configuration_id: &ConfigurationId,
-    ) -> Result<ConfigurationSnapshot> {
+    ) -> Result<Configuration> {
         let configuration = server_client.get_configuration(configuration_id)?;
-        ConfigurationSnapshot::new(&configuration_id.environment_id, configuration)
+        Configuration::new(&configuration_id.environment_id, configuration)
     }
 
     fn wait_for_configuration_update(
         socket: &mut WebSocket<MaybeTlsStream<TcpStream>>,
         server_client_impl: &ServerClientImpl,
         configuration_id: &ConfigurationId,
-    ) -> Result<ConfigurationSnapshot> {
+    ) -> Result<Configuration> {
         loop {
             // read() blocks until something happens.
             match socket.read()? {
@@ -110,7 +110,7 @@ impl AppConfigurationClientHttp {
 
     fn update_configuration_on_change(
         mut socket: WebSocket<MaybeTlsStream<TcpStream>>,
-        latest_config_snapshot: Arc<Mutex<ConfigurationSnapshot>>,
+        latest_config_snapshot: Arc<Mutex<Configuration>>,
         server_client_impl: ServerClientImpl,
         configuration_id: ConfigurationId,
     ) -> std::sync::mpsc::Sender<()> {
@@ -146,7 +146,7 @@ impl AppConfigurationClientHttp {
     }
 
     fn update_cache_in_background(
-        latest_config_snapshot: Arc<Mutex<ConfigurationSnapshot>>,
+        latest_config_snapshot: Arc<Mutex<Configuration>>,
         server_client_impl: ServerClientImpl,
         configuration_id: ConfigurationId,
     ) -> Result<std::sync::mpsc::Sender<()>> {
@@ -245,17 +245,17 @@ mod tests {
         configuration_feature1_enabled, configuration_property1_enabled,
         example_configuration_enterprise,
     };
-    use crate::{models::Configuration, Feature, Property};
+    use crate::{models::ConfigurationJson, Feature, Property};
     use rstest::rstest;
 
     #[rstest]
     fn test_get_feature_persistence(
-        example_configuration_enterprise: Configuration,
-        configuration_feature1_enabled: Configuration,
+        example_configuration_enterprise: ConfigurationJson,
+        configuration_feature1_enabled: ConfigurationJson,
     ) {
         let client = {
             let configuration_snapshot =
-                ConfigurationSnapshot::new("dev", example_configuration_enterprise).unwrap();
+                Configuration::new("dev", example_configuration_enterprise).unwrap();
 
             let (sender, _) = std::sync::mpsc::channel();
 
@@ -272,7 +272,7 @@ mod tests {
 
         // We simulate an update of the configuration:
         let configuration_snapshot =
-            ConfigurationSnapshot::new("environment_id", configuration_feature1_enabled).unwrap();
+            Configuration::new("environment_id", configuration_feature1_enabled).unwrap();
         *client.latest_config_snapshot.lock().unwrap() = configuration_snapshot;
         // The feature value should not have changed (as we did not retrieve it again)
         let feature_value2 = feature.get_value(&entity).unwrap();
@@ -287,12 +287,12 @@ mod tests {
 
     #[rstest]
     fn test_get_property_persistence(
-        example_configuration_enterprise: Configuration,
-        configuration_property1_enabled: Configuration,
+        example_configuration_enterprise: ConfigurationJson,
+        configuration_property1_enabled: ConfigurationJson,
     ) {
         let client = {
             let configuration_snapshot =
-                ConfigurationSnapshot::new("dev", example_configuration_enterprise).unwrap();
+                Configuration::new("dev", example_configuration_enterprise).unwrap();
 
             let (sender, _) = std::sync::mpsc::channel();
 
@@ -309,7 +309,7 @@ mod tests {
 
         // We simulate an update of the configuration:
         let configuration_snapshot =
-            ConfigurationSnapshot::new("environment_id", configuration_property1_enabled).unwrap();
+            Configuration::new("environment_id", configuration_property1_enabled).unwrap();
         *client.latest_config_snapshot.lock().unwrap() = configuration_snapshot;
         // The property value should not have changed (as we did not retrieve it again)
         let property_value2 = property.get_value(&entity).unwrap();

src/client/app_configuration_offline.rs

@@ -12,20 +12,20 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use crate::client::cache::ConfigurationSnapshot;
+use crate::client::configuration::Configuration;
 pub use crate::client::feature_proxy::FeatureProxy;
 use crate::client::feature_snapshot::FeatureSnapshot;
 pub use crate::client::property_proxy::PropertyProxy;
 use crate::client::property_snapshot::PropertySnapshot;
 use crate::errors::{DeserializationError, Error, Result};
-use crate::models::Configuration;
+use crate::models::ConfigurationJson;
 
 use super::AppConfigurationClient;
 
 /// AppConfiguration client using a local file with a configuration snapshot
 #[derive(Debug)]
 pub struct AppConfigurationOffline {
-    pub(crate) config_snapshot: ConfigurationSnapshot,
+    pub(crate) config_snapshot: Configuration,
 }
 
 impl AppConfigurationOffline {
@@ -44,7 +44,7 @@ impl AppConfigurationOffline {
         })?;
         let reader = std::io::BufReader::new(file);
 
-        let configuration: Configuration = serde_json::from_reader(reader).map_err(|e| {
+        let configuration: ConfigurationJson = serde_json::from_reader(reader).map_err(|e| {
             Error::DeserializationError(DeserializationError {
                 string: format!(
                     "Error deserializing Configuration from file '{}'",
@@ -53,7 +53,7 @@ impl AppConfigurationOffline {
                 source: e.into(),
             })
         })?;
-        let config_snapshot = ConfigurationSnapshot::new(environment_id, configuration)?;
+        let config_snapshot = Configuration::new(environment_id, configuration)?;
 
         Ok(Self { config_snapshot })
     }

src/client/cache.rs renamed to src/client/configuration.rs

@@ -15,16 +15,19 @@
 use std::collections::{HashMap, HashSet};
 
 use crate::errors::{ConfigurationAccessError, Result};
-use crate::models::{Configuration, Feature, Property, Segment, TargetingRule};
+use crate::models::{ConfigurationJson, Feature, Property, Segment, TargetingRule};
 
+/// Represents all the configuration data needed for the client to perform
+/// feature/propery evaluation.
+/// It contains a subset of models::ConfigurationJson, adding indexing.
 #[derive(Debug, Default)]
-pub(crate) struct ConfigurationSnapshot {
+pub(crate) struct Configuration {
     pub(crate) features: HashMap<String, Feature>,
     pub(crate) properties: HashMap<String, Property>,
     pub(crate) segments: HashMap<String, Segment>,
 }
 
-impl ConfigurationSnapshot {
+impl Configuration {
     pub fn get_feature(&self, feature_id: &str) -> Result<&Feature> {
         self.features.get(feature_id).ok_or_else(|| {
             ConfigurationAccessError::FeatureNotFound {
@@ -43,7 +46,8 @@ impl ConfigurationSnapshot {
         })
     }
 
-    pub fn new(environment_id: &str, configuration: Configuration) -> Result<Self> {
+    /// Constructs the Configuration, by consuming and filtering data in exchange format
+    pub fn new(environment_id: &str, configuration: ConfigurationJson) -> Result<Self> {
         let environment = configuration
             .environments
             .into_iter()
@@ -69,7 +73,7 @@ impl ConfigurationSnapshot {
         for segment in configuration.segments {
             segments.insert(segment.segment_id.clone(), segment.clone());
         }
-        Ok(ConfigurationSnapshot {
+        Ok(Configuration {
             features,
             properties,
             segments,
@@ -106,14 +110,14 @@ mod tests {
     use super::*;
     use crate::errors::Error;
     use crate::models::tests::example_configuration_enterprise;
-    use crate::models::Configuration;
+    use crate::models::ConfigurationJson;
 
     use rstest::*;
 
     #[rstest]
-    fn test_filter_configurations(example_configuration_enterprise: Configuration) {
+    fn test_filter_configurations(example_configuration_enterprise: ConfigurationJson) {
         let result =
-            ConfigurationSnapshot::new("does_for_sure_not_exist", example_configuration_enterprise);
+            Configuration::new("does_for_sure_not_exist", example_configuration_enterprise);
         assert!(result.is_err());
 
         assert!(matches!(

src/client/mod.rs

@@ -17,7 +17,7 @@ mod app_configuration_http;
 mod app_configuration_ibm_cloud;
 mod app_configuration_offline;
 
-pub(crate) mod cache;
+pub(crate) mod configuration;
 pub(crate) mod feature_proxy;
 pub(crate) mod feature_snapshot;
 pub(crate) mod property_proxy;

src/models.rs

@@ -18,8 +18,13 @@ use serde::Deserialize;
 
 use crate::Value;
 
+/// Represents AppConfig data in a structure intended for data exchange
+/// (typically JSON encoded) used by
+/// - AppConfig Server REST API (/config endpoint)
+/// - AppConfig database dumps (via Web GUI)
+/// - Offline configuration files used in offline-mode
 #[derive(Debug, Deserialize)]
-pub(crate) struct Configuration {
+pub(crate) struct ConfigurationJson {
     pub environments: Vec<Environment>,
     pub segments: Vec<Segment>,
 }
@@ -55,6 +60,8 @@ pub(crate) struct Feature {
     pub _format: Option<String>,
     pub enabled_value: ConfigValue,
     pub disabled_value: ConfigValue,
+    // NOTE: why is this field called `segment_rules` and not `targeting_rules`?
+    // This causes quite som ambiguity with SegmentRule vs TargetingRule.
     pub segment_rules: Vec<TargetingRule>,
     pub enabled: bool,
     pub rollout_percentage: u32,
@@ -71,6 +78,8 @@ pub(crate) struct Property {
     #[serde(rename = "format")]
     pub _format: Option<String>,
     pub value: ConfigValue,
+    // NOTE: why is this field called `segment_rules` and not `targeting_rules`?
+    // This causes quite som ambiguity with SegmentRule vs TargetingRule.
     pub segment_rules: Vec<TargetingRule>,
 }
 
@@ -165,15 +174,26 @@ impl TryFrom<(ValueKind, ConfigValue)> for Value {
     }
 }
 
+/// Represents a Rule of a Segment.
+/// Those are the rules to check if an entity belongs to a segment.
+/// NOTE: This is easily confused with `TargetingRule`, which is
+/// sometimes also called "SegmentRule".
 #[derive(Clone, Debug, Deserialize)]
 pub(crate) struct SegmentRule {
     pub attribute_name: String,
     pub operator: String,
     pub values: Vec<String>,
 }
 
+/// Associates a Feature/Property to one or more Segments
+/// NOTE: This is easily confused with `SegmentRule`, as the field name in
+/// Features containing TargetingRules is called `segment_rules`
 #[derive(Debug, Deserialize, Clone)]
 pub(crate) struct TargetingRule {
+    /// The list of targeted segments
+    /// NOTE: no rules by itself, but the rules are found in the segments
+    /// NOTE: why list of lists?
+    /// NOTE: why is this field called "rules"?
     pub rules: Vec<Segments>,
     pub value: ConfigValue,
     pub order: u32,
@@ -205,17 +225,17 @@ pub(crate) mod tests {
     // Create a [`Configuration`] object from the data files
     pub(crate) fn example_configuration_enterprise(
         example_configuration_enterprise_path: PathBuf,
-    ) -> Configuration {
+    ) -> ConfigurationJson {
         let content = fs::File::open(example_configuration_enterprise_path)
             .expect("file should open read only");
-        let configuration: Configuration =
+        let configuration: ConfigurationJson =
             serde_json::from_reader(content).expect("Error parsing JSON into Configuration");
         configuration
     }
 
     #[fixture]
-    pub(crate) fn configuration_feature1_enabled() -> Configuration {
-        Configuration {
+    pub(crate) fn configuration_feature1_enabled() -> ConfigurationJson {
+        ConfigurationJson {
             environments: vec![Environment {
                 _name: "name".to_string(),
                 environment_id: "environment_id".to_string(),
@@ -237,8 +257,8 @@ pub(crate) mod tests {
     }
 
     #[fixture]
-    pub(crate) fn configuration_property1_enabled() -> Configuration {
-        Configuration {
+    pub(crate) fn configuration_property1_enabled() -> ConfigurationJson {
+        ConfigurationJson {
             environments: vec![Environment {
                 _name: "name".to_string(),
                 environment_id: "environment_id".to_string(),
@@ -258,7 +278,7 @@ pub(crate) mod tests {
     }
 
     #[fixture]
-    pub(crate) fn configuration_unordered_segment_rules() -> Configuration {
+    pub(crate) fn configuration_unordered_segment_rules() -> ConfigurationJson {
         let segment_rules = vec![
             TargetingRule {
                 rules: vec![Segments {
@@ -279,7 +299,7 @@ pub(crate) mod tests {
         ];
         assert!(segment_rules[0].order > segment_rules[1].order);
 
-        Configuration {
+        ConfigurationJson {
             environments: vec![Environment {
                 _name: "name".to_string(),
                 environment_id: "environment_id".to_string(),

src/network/http_client.rs

@@ -13,7 +13,7 @@
 // limitations under the License.
 
 use super::TokenProvider;
-use crate::models::Configuration;
+use crate::models::ConfigurationJson;
 use crate::{ConfigurationId, Error, Result};
 use reqwest::blocking::Client;
 use std::cell::RefCell;
@@ -99,7 +99,7 @@ impl ServerClientImpl {
         })
     }
 
-    pub fn get_configuration(&self, collection: &ConfigurationId) -> Result<Configuration> {
+    pub fn get_configuration(&self, collection: &ConfigurationId) -> Result<ConfigurationJson> {
         let url = format!(
             "{}/feature/v1/instances/{}/config",
             self.service_address.base_url(ServiceAddressProtocol::Https),

src/tests/test_get_feature.rs

@@ -14,9 +14,9 @@
 
 use std::collections::HashMap;
 
-use crate::models::Configuration;
+use crate::models::ConfigurationJson;
 
-use crate::client::cache::ConfigurationSnapshot;
+use crate::client::configuration::Configuration;
 use crate::client::AppConfigurationClient;
 use crate::{AppConfigurationOffline, Value};
 use rstest::*;
@@ -36,10 +36,9 @@ fn test_get_feature_doesnt_exist(client_enterprise: Box<dyn AppConfigurationClie
 }
 
 #[rstest]
-fn test_get_feature_ordered(configuration_unordered_segment_rules: Configuration) {
+fn test_get_feature_ordered(configuration_unordered_segment_rules: ConfigurationJson) {
     let config_snapshot =
-        ConfigurationSnapshot::new("environment_id", configuration_unordered_segment_rules)
-            .unwrap();
+        Configuration::new("environment_id", configuration_unordered_segment_rules).unwrap();
 
     let client = AppConfigurationOffline { config_snapshot };