Skip to content

Declare a Feature trait, re-implement FeatureProxy #26

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 11, 2024
Merged

Declare a Feature trait, re-implement FeatureProxy #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
58ca974
Declare a Feature trait, implement FeatureProxy in terms of inner Fea…
jgsogo Dec 11, 2024
0d12c68
Rename feature.rs file to feature_snapshot.rs, to match the struct name
jgsogo Dec 11, 2024
9199999
Merge branch 'main' into api/feature-trait
jgsogo Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions examples/demo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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) => {
Expand Down
13 changes: 5 additions & 8 deletions src/client/app_configuration_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think we should change the function name to get_feature_snapshot too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, but I'm not sure. Maybe we should change FeatureSnapshot instead. Something like FeatureImpl or similar

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I like the type name "FeatureSnapshot" :)

let config_snapshot = self.latest_config_snapshot.lock()?;

// Get the feature from the snapshot
Expand Down Expand Up @@ -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>> {
Expand Down
202 changes: 27 additions & 175 deletions src/client/feature_proxy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Comment on lines +25 to +26
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it evident that the feature proxy can only live as long as the client is around. See the getter:

pub fn get_feature_proxy<'a>(&'a self, feature_id: &str) -> Result<FeatureProxy<'a>>

This implementation will also let us simplify the internals of the AppConfigurationClient... I'm not 100% sure if this will be the final API, but as long as we don't have a user requesting something different I would choose whatever is easier to implement/maintain.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the reference here :-) How would a user use this though? is the proxy still useful?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed: probably like this:

{
let client = AppConfigClient::new(...);
let feature1 = client.get_feature_proxy(...);
let feature2 = client.get_feature_proxy(...);
some_sub_component_entry_point(feature1);
some_other_sub_component(feature2);
}

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)
}
}

Expand Down
Loading
Loading