Skip to content

Minimize exposed API #29

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 1 commit into from
Dec 13, 2024
Merged

Minimize exposed API #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 20 additions & 18 deletions examples/demo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@

use std::{collections::HashMap, env, thread, time::Duration};

use appconfiguration_rust_sdk::{client::AppConfigurationClient, AttrValue, Entity, Feature, Property};
use appconfiguration_rust_sdk::{
AppConfigurationClient, AttrValue, Entity, Feature, Property, Value,
};
use dotenvy::dotenv;
use std::error::Error;

type Result<T> = std::result::Result<T, Box<dyn Error + Send + Sync>>;

#[derive(Debug)]
struct CustomerEntity {
id: String,
Expand All @@ -33,16 +33,14 @@ impl Entity for CustomerEntity {
}

fn get_attributes(&self) -> HashMap<String, AttrValue> {
use AttrValue;

HashMap::from_iter(vec![
("city".to_string(), AttrValue::String(self.city.clone())),
("radius".to_string(), AttrValue::Numeric(self.radius as f64)),
])
}
}

fn main() -> Result<()> {
fn main() -> std::result::Result<(), Box<dyn Error>> {
dotenv().ok();
let region = env::var("REGION").expect("REGION should be set.");
let guid = env::var("GUID").expect("GUID should be set.");
Expand Down Expand Up @@ -70,13 +68,15 @@ fn main() -> Result<()> {
match client.get_feature_proxy(&feature_id) {
Ok(feature) => {
println!("Feature name: {}", feature.get_name()?);
println!("Feature id: {}", feature.get_id());
println!("Feature data type: {}", feature.get_data_type()?);
let value = feature.get_value(&entity)?;
let data_type = match &value {
Value::Numeric(_) => "Numeric",
Value::String(_) => "String",
Value::Boolean(_) => "Boolean",
};
println!("Feature data type: {}", data_type);
println!("Is feature enabled: {}", feature.is_enabled()?);
println!(
"Feature evaluated value is: {:?}",
feature.get_value(&entity)?
);
println!("Feature evaluated value is: {value:?}");
}
Err(error) => {
println!("There was an error getting the Feature Flag. Error {error}",);
Expand All @@ -87,12 +87,14 @@ fn main() -> Result<()> {
match client.get_property_proxy(&property_id) {
Ok(property) => {
println!("Property name: {}", property.get_name()?);
println!("Property id: {}", property.get_id());
println!("Property data type: {}", property.get_data_type()?);
println!(
"Property evaluated value is: {:?}",
property.get_value(&entity)?
);
let value = property.get_value(&entity)?;
let data_type = match &value {
Value::Numeric(_) => "Numeric",
Value::String(_) => "String",
Value::Boolean(_) => "Boolean",
};
println!("Property data type: {data_type}");
println!("Property evaluated value is: {value:?}");
}
Err(error) => {
println!("There was an error getting the Property. Error {error}",);
Expand Down
18 changes: 2 additions & 16 deletions src/client/feature_proxy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use std::io::Cursor;
use murmur3::murmur3_32;

use crate::entity::Entity;
use crate::Feature;
use crate::{Feature, Value};

use super::feature_snapshot::FeatureSnapshot;
use super::AppConfigurationClient;
Expand All @@ -38,29 +38,15 @@ impl<'a> FeatureProxy<'a> {
}

impl<'a> Feature for FeatureProxy<'a> {
fn get_id(&self) -> &str {
&self.feature_id
}

fn get_name(&self) -> crate::errors::Result<String> {
self.client.get_feature(&self.feature_id)?.get_name()
}

fn get_data_type(&self) -> crate::errors::Result<crate::models::ValueKind> {
self.client.get_feature(&self.feature_id)?.get_data_type()
}

fn is_enabled(&self) -> crate::errors::Result<bool> {
self.client.get_feature(&self.feature_id)?.is_enabled()
}
Comment on lines 45 to 47
Copy link
Member

@rainerschoe rainerschoe Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

for the future:

Not sure if is_enabled this is something we want to expose to the user. The concept of "enabled" is hard to understand and does not consider entity evaluation.
It might be confusing to see the feature as enabled but when getting the value (using entity) a user will see the disabled value, as then rollout percentage and segment evaluation is considered.
It might tempt users to use the is_enabled() function to evaluate the feature (assuming most features are boolean) which would give wrong results.

IMHO, the client should not provide this function, only admin interface server config should give control over this.

maybe @saikumar1607 can help to clarify?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If a feature is disabled, the user can choose not to evaluate it for the entities, but it doesn't mean that the user knows the enabled and disabled values.

It might tempt users to use the is_enabled() function to evaluate the feature (assuming most features are boolean) which would give wrong results.

Only using our get_value, the user can retrieve the values.... and it requires the Entity:

fn get_value(&self, entity: &impl Entity) -> Result<Value>;

And of course, even if the features are boolean and "disabled" means "false", the user won't benefit of the rollout related capabilities provided by IBM AppConfiguration.

In my mind, knowing if a feature is enabled or not, could be needed for applications that might choose whether to load a module or not. For example, given an e-commerce, if the "groceries feature" is not enabled, I won't load that module (maybe at startup time). Or, for example, if the "advanced-pricing" is disabled, I will just use default pricing for all the users instead of running some evaluation for each product for each user.


fn get_enabled_value(&self) -> crate::errors::Result<crate::models::ConfigValue> {
self.client
.get_feature(&self.feature_id)?
.get_enabled_value()
}

fn get_value(&self, entity: &impl Entity) -> crate::errors::Result<super::value::Value> {
fn get_value(&self, entity: &impl Entity) -> crate::errors::Result<Value> {
self.client.get_feature(&self.feature_id)?.get_value(entity)
}
}
Expand Down
21 changes: 3 additions & 18 deletions src/client/feature_snapshot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use crate::client::value::{NumericValue, Value};
use crate::value::{NumericValue, Value};
use crate::entity::Entity;
use crate::Feature;
use std::collections::HashMap;
Expand Down Expand Up @@ -102,26 +102,14 @@ impl FeatureSnapshot {
}

impl Feature for FeatureSnapshot {
fn get_id(&self) -> &str {
&self.feature.feature_id
}

fn get_name(&self) -> Result<String> {
Ok(self.feature.name.clone())
}

fn get_data_type(&self) -> Result<crate::models::ValueKind> {
Ok(self.feature.kind)
}

fn is_enabled(&self) -> Result<bool> {
Ok(self.feature.enabled)
}

fn get_enabled_value(&self) -> Result<crate::models::ConfigValue> {
Ok(self.feature.enabled_value.clone())
}

fn get_value(&self, entity: &impl Entity) -> Result<Value> {
let model_value = self.evaluate_feature_for_entity(entity)?;

Expand Down Expand Up @@ -151,11 +139,8 @@ impl Feature for FeatureSnapshot {
pub mod tests {

use super::*;
use crate::{
entity,
models::{ConfigValue, Segment, SegmentRule, Segments, TargetingRule, ValueKind},
AttrValue,
};
use crate::entity::AttrValue;
use crate::models::{ConfigValue, Segment, SegmentRule, Segments, TargetingRule, ValueKind};
use rstest::rstest;

#[rstest]
Expand Down
6 changes: 3 additions & 3 deletions src/client/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@
mod app_configuration_client;

pub(crate) mod cache;
pub mod feature_snapshot;
pub(crate) mod feature_snapshot;
pub(crate) mod feature_proxy;
pub(crate) mod http;
pub mod property_snapshot;
pub(crate) mod property_snapshot;
pub(crate) mod property_proxy;
pub mod value;


pub use app_configuration_client::AppConfigurationClient;

Expand Down
20 changes: 3 additions & 17 deletions src/client/property_proxy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@

use crate::Property;

use crate::entity::Entity;

use super::property_snapshot::PropertySnapshot;
use super::AppConfigurationClient;
use crate::value::Value;
use crate::Entity;

pub struct PropertyProxy<'a> {
client: &'a AppConfigurationClient,
Expand All @@ -38,25 +38,11 @@ impl<'a> PropertyProxy<'a> {
}

impl<'a> Property for PropertyProxy<'a> {
fn get_id(&self) -> &str {
&self.property_id
}

fn get_name(&self) -> crate::errors::Result<String> {
self.client.get_property(&self.property_id)?.get_name()
}

fn get_data_type(&self) -> crate::errors::Result<crate::models::ValueKind> {
self.client.get_property(&self.property_id)?.get_data_type()
}

fn get_value_default(&self) -> crate::errors::Result<crate::models::ConfigValue> {
self.client
.get_property(&self.property_id)?
.get_value_default()
}

fn get_value(&self, entity: &impl Entity) -> crate::errors::Result<super::value::Value> {
fn get_value(&self, entity: &impl Entity) -> crate::errors::Result<Value> {
self.client
.get_property(&self.property_id)?
.get_value(entity)
Expand Down
21 changes: 3 additions & 18 deletions src/client/property_snapshot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use crate::client::value::{NumericValue, Value};
use crate::value::{NumericValue, Value};
use crate::entity::Entity;
use crate::Property;
use std::collections::HashMap;
Expand Down Expand Up @@ -63,22 +63,10 @@ impl PropertySnapshot {
}

impl Property for PropertySnapshot {
fn get_id(&self) -> &str {
&self.property.property_id
}

fn get_name(&self) -> Result<String> {
Ok(self.property.name.clone())
}

fn get_data_type(&self) -> Result<crate::models::ValueKind> {
Ok(self.property.kind)
}

fn get_value_default(&self) -> Result<crate::models::ConfigValue> {
Ok(self.property.value.clone())
}

fn get_value(&self, entity: &impl Entity) -> Result<Value> {
let model_value = self.evaluate_feature_for_entity(entity)?;

Expand All @@ -104,14 +92,11 @@ impl Property for PropertySnapshot {
}
}


#[cfg(test)]
pub mod tests {
use super::*;
use crate::{
models::{ConfigValue, Segment, SegmentRule, Segments, TargetingRule, ValueKind},
AttrValue,
};
use crate::entity::AttrValue;
use crate::models::{ConfigValue, Segment, SegmentRule, Segments, TargetingRule, ValueKind};

#[test]
fn test_get_value_segment_with_default_value() {
Expand Down
2 changes: 1 addition & 1 deletion src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::sync::PoisonError;

use thiserror::Error;

use crate::segment_evaluation::SegmentEvaluationError;
use crate::segment_evaluation::errors::SegmentEvaluationError;

pub type Result<T> = std::result::Result<T, Error>;

Expand Down
9 changes: 1 addition & 8 deletions src/feature.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,13 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use crate::client::value::Value;
use crate::errors::Result;
use crate::Entity;
use crate::{Entity, Value};

pub trait Feature {
fn get_id(&self) -> &str;

fn get_name(&self) -> Result<String>;

fn get_data_type(&self) -> Result<crate::models::ValueKind>;

fn is_enabled(&self) -> Result<bool>;

fn get_enabled_value(&self) -> Result<crate::models::ConfigValue>;

fn get_value(&self, entity: &impl Entity) -> Result<Value>;
}
16 changes: 10 additions & 6 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,21 @@
// See the License for the specific language governing permissions and
// limitations under the License.

pub mod client;
pub mod entity;
pub mod errors;
pub mod models;
mod segment_evaluation;
mod client;
mod entity;
mod errors;
mod feature;
mod models;
mod property;
mod segment_evaluation;
mod value;

pub use client::AppConfigurationClient;
pub use entity::{Entity, AttrValue};
pub use feature::Feature;
pub use property::Property;
pub use entity::{AttrValue, Entity};
pub use value::Value;
pub use errors::{Result, Error};

#[cfg(test)]
mod tests;
16 changes: 8 additions & 8 deletions src/models.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ pub(crate) struct Property {
}

#[derive(Copy, Clone, Debug, Deserialize, PartialEq)]
pub enum ValueKind {
pub(crate) enum ValueKind {
#[serde(rename(deserialize = "NUMERIC"))]
Numeric,
#[serde(rename(deserialize = "BOOLEAN"))]
Expand All @@ -87,7 +87,7 @@ impl Display for ValueKind {
}

#[derive(Debug, Clone, Deserialize)]
pub struct ConfigValue(pub(crate) serde_json::Value);
pub(crate) struct ConfigValue(pub(crate) serde_json::Value);

impl ConfigValue {
pub fn as_i64(&self) -> Option<i64> {
Expand Down Expand Up @@ -133,27 +133,27 @@ pub(crate) struct SegmentRule {
}

#[derive(Debug, Deserialize, Clone)]
pub struct TargetingRule {
pub(crate) struct TargetingRule {
pub rules: Vec<Segments>,
pub value: ConfigValue,
pub order: u32,
pub rollout_percentage: Option<ConfigValue>,
}

#[derive(Debug, Deserialize, Clone)]
pub struct Segments {
pub(crate) struct Segments {
pub segments: Vec<String>,
}

#[cfg(test)]
pub mod tests {
pub(crate) mod tests {

use super::*;
use rstest::*;
use std::{fs, path::PathBuf};

#[fixture]
pub fn example_configuration_enterprise() -> Configuration {
pub(crate) fn example_configuration_enterprise() -> Configuration {
Comment on lines +144 to +156
Copy link
Member

Choose a reason for hiding this comment

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

Just for my understanding:
why sometimes

pub(crate)...{
   pub ...
}

and sometimes

pub(crate)...{
   pub(crate) ...
}

?

// Create a configuration object from the data files

let mut mocked_data = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
Expand All @@ -166,7 +166,7 @@ pub mod tests {
}

#[fixture]
pub fn configuration_feature1_enabled() -> Configuration {
pub(crate) fn configuration_feature1_enabled() -> Configuration {
Configuration {
environments: vec![Environment {
name: "name".to_string(),
Expand All @@ -189,7 +189,7 @@ pub mod tests {
}

#[fixture]
pub fn configuration_property1_enabled() -> Configuration {
pub(crate) fn configuration_property1_enabled() -> Configuration {
Configuration {
environments: vec![Environment {
name: "name".to_string(),
Expand Down
Loading
Loading