-
Notifications
You must be signed in to change notification settings - Fork 11
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.
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
chore: redesign source and sinks to store features #57
Conversation
… filter the responses by project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
.environment | ||
.as_ref() | ||
.map(|environment| format!("{FEATURE_PREFIX}{environment}")) | ||
.expect("Tying to resolve features for a token that hasn't been validated") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.expect("Tying to resolve features for a token that hasn't been validated") | |
.expect("Resolving features for a validated token") |
Just words, but when I read the expect
(might be because I'm unfamiliar with Rust yet), it seems that you're expecting a non validated token...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's fair. Expect should loosely read as "I expected this value and if I didn't get it, crash with the enclosed error message"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also agree that expect
is a bit confusing readability wise. It's almost like it should be a except
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine, just some comments
let features_to_store = | ||
if let Some(stored_features) = con.get::<&str, Option<String>>(key.as_str()).await? { | ||
let stored_features = serde_json::from_str::<ClientFeatures>(&stored_features)?; | ||
stored_features.merge(features.clone()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a problem for later, but this could be an always-growing collection. We won't remove data and the TTL might get updated so stored_features will never expire...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point! Yes, we should fix this but probably in a different PR
self.iter() | ||
.filter(|feature| { | ||
if let Some(feature_project) = &feature.project { | ||
token.projects.contains(&"*".to_string()) | ||
|| token.projects.contains(feature_project) | ||
} else { | ||
false | ||
} | ||
}) | ||
.cloned() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal, but this code is pretty similar to the one in server/src/data_sources/redis_provider.rs, maybe something to extract
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, basically the same code. It'll be the same code for every source. Nice target for a trait impl in future
fn build_features_key(token: &EdgeToken) -> String { | ||
token | ||
.environment | ||
.as_ref() | ||
.map(|environment| format!("{FEATURE_PREFIX}{environment}")) | ||
.expect("Tying to resolve features for a token that hasn't been validated") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use the function from redis_provider here? Maybe it's a thing about function scopes and visibility that makes it difficult to do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally didn't do that because I don't want to expose that function. I figure it's okay because if you'll break these tests hard if the backing implementation changes. Don't mind a bit of duplication in tests to preserve sanity in the public API
This redesigns the way we store data in the memory and Redis providers to store incoming features by environment rather the token string. This means that incoming features for an environment will be merged in the case that the come from the same environment but different tokens. To handle this in the sources, outgoing data is filtered by project