-
Notifications
You must be signed in to change notification settings - Fork 2
Description
Context
I have a Claim struct that may contain additional claims, defined as follows:
#[derive(Clone, PartialEq, Debug, Eq)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
pub struct Claim {
r#type: Cow<'static, str>,
value: String,
issuer: Option<String>,
label: Option<Cow<'static, str>>,
additional_claims: Option<Vec<Claim>>,
}
Claims can be created like this:
```rust
let claim = Claim::from("email", String::from("auth-rs@example.com"));This works well for me. Now, I have another struct called Claims, which is used to organize all claims, so we can quickly retrieve claims by a specific label. The current definition of Claimsis:
#[derive(Clone, PartialEq, Debug, Eq)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
pub struct Claims {
// So we don't carry an empty map all around when not used
claims: Option<HashMap<String, Vec<Claim>>>,
}The claims can be grouped as follows:
let claims = vec![
Claim::from("email", String::from("auth-rs@example.com")),
Claim::from("name", String::from("auth-rs"))
];
let google_claim = Claims::new().insert("google", claims);We will have different methods to help get different sets of claims, as defined below:
impl Claims {
pub fn new() -> Self {
Self { claims: None }
}
pub fn is_empty(&self) -> bool {
self.claims
.as_ref()
.map_or(true, |claims| claims.is_empty())
}
pub fn insert(&mut self, label: &str, claim: Claim) -> &mut Self {
todo!()
}
pub fn insert_all<T>(&mut self, label: &str, claims: T) -> &mut Self
where
T: IntoIterator<Item = Claim>,
{
todo!()
}
pub fn get_all(&mut self, label: &str) -> Option<Vec<Claim>> {
todo!()
}
pub fn get_all_with<F>(&mut self, label: &str, predicate: F) -> Option<Vec<Claim>>
where
F: Fn(&Claim) -> bool,
{
todo!()
}
pub fn remove_all(&mut self, label: &str) -> Option<Vec<Claim>> {
todo!()
}
pub fn remove_all_with<F>(&mut self, label: &str, predicate: F) -> Option<Vec<Claim>>
where
F: Fn(&Claim) -> bool,
{
todo!()
}
}The Problem
The problem is that I’m concerned about the heap allocations when storing claims by label, which I believe could be avoided by storing all claims together and filtering them by predicate when needed.
Currently, the Claims struct uses a HashMap<String, Vec<Claim>> to partition the claims by label. However, I am considering an alternative approach where all claims are stored together and filtered by label and predicate as needed.
Proposed Solution
I’m considering changing the Claims struct to:
// OLD API
#[derive(Clone, PartialEq, Debug, Eq)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
pub struct Claims {
// So we don't carry an empty map all around when not used
claims: Option<HashMap<String, Vec<Claim>>>,
}
// NEW API
#[derive(Clone, PartialEq, Debug, Eq)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
pub struct Claims {
// So we don't carry an empty map all around when not used
claims: Option<HashMap<String, Claim>>,
}This would avoid the need for a Vec for each label and would store all claims together. When retrieving claims, they would be recursively arranged as needed, based on the label and any predicates.
Considerations
-
Performance Concerns: By storing all claims together, we might reduce heap allocations, but retrieving claims could require more processing (i.e., recursively arranging claims).
-
Complexity: The new structure might add complexity when retrieving or managing claims, especially if they need to be filtered by label and predicate.
-
Over-engineering?: Am I over-engineering the problem by trying to optimize heap allocation at this stage, or is this a valid concern for improving performance and memory usage?