-
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
feat: delta filtering and etag handling #749
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
) -> EdgeJsonResult<ClientFeaturesDelta> { | ||
resolve_delta(edge_token, delta_cache, token_cache, filter_query, req).await | ||
) -> impl Responder { | ||
let requested_revision_id = req |
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.
Get etag from sdk
@@ -60,16 +51,6 @@ pub(crate) fn filter_client_features( | |||
} | |||
} | |||
|
|||
pub(crate) fn filter_delta_events( |
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.
This is moved to delta_filters
filters: Vec<DeltaFilter>, | ||
} | ||
|
||
impl DeltaFilterSet { |
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.
This is based on filters.rs, which is for filtering old features.
server/src/client_api.rs
Outdated
let query_filters = filter_query.into_inner(); | ||
|
||
let delta_filter_set = DeltaFilterSet::default().with_filter(combined_filter( | ||
100, |
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.
What's 100 here? Is the max or something? Hard to know what this is without context, can we put it in a well named const?
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.
Thanks, this was oversight actually. fixed.
@@ -164,27 +213,52 @@ async fn resolve_features( | |||
|
|||
async fn resolve_delta( |
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 think the return type of Option<Result> is making your life much harder than it needs to be around error handling here and it makes semantic understanding of the function challenging. Usually this is expressed as Result<Option> for this reason. I also don't think this function has any business converting things to Json, based on its name, that's a higher level responsibility if we can possibly help it. We can also move the check for the feature refresher earlier where it can act as a guard clause before we need to do any work for the building of fiilters. This can all be expressed as
async fn resolve_delta(
edge_token: EdgeToken,
token_cache: Data<DashMap<String, EdgeToken>>,
filter_query: Query<FeatureFilters>,
requested_revision_id: u32,
req: HttpRequest,
) -> EdgeResult<Option<ClientFeaturesDelta>> {
let Some(refresher) = req.app_data::<Data<FeatureRefresher>>() else {
return Err(EdgeError::ClientHydrationFailed(
"FeatureRefresher is missing - cannot resolve delta in offline mode".to_string(),
));
};
let (validated_token, filter_set, ..) =
get_feature_filter(&edge_token, &token_cache, filter_query.clone())?;
let delta_filter_set = get_delta_filter(&edge_token, &token_cache, filter_query.clone())?;
let current_sdk_revision_id = requested_revision_id + 1; // TODO: get from delta manager
if requested_revision_id >= current_sdk_revision_id {
return Ok(None);
}
let delta = refresher
.delta_events_for_filter(
validated_token.clone(),
&filter_set,
&delta_filter_set,
requested_revision_id,
)
.await?;
if !delta.events.is_empty() {
Ok(Some(delta))
} else {
Ok(None)
}
}
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.
Regarding JSON, this is following existing pattern.
unleash-edge/server/src/client_api.rs
Line 163 in 9a26011
Ok(Json(ClientFeatures { |
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.
Also, Reworked the Option as you suggested. Now it is inside instead. I think its better now.
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.
Regarding JSON, this is following existing pattern.
unleash-edge/server/src/client_api.rs
Line 163 in 9a26011
Ok(Json(ClientFeatures {
Yeah but that doesn't work so well with Options, which is why I suggested not using that. I think what you've done is okay though
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.
Sure
This PR handles 2 things
Next PR