-
-
Notifications
You must be signed in to change notification settings - Fork 119
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
Make background validation performed by Services configurable #2060 #2150
Changes from 1 commit
81e0840
c411a63
f7d6331
f57c77e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
use std::ops::Deref; | ||
use std::ops::{Deref, DerefMut}; | ||
|
||
/// A service that controls access to some state | ||
/// | ||
|
@@ -62,6 +62,12 @@ impl<S: State> Deref for Service<S> { | |
} | ||
} | ||
|
||
impl<S: State> DerefMut for Service<S> { | ||
fn deref_mut(&mut self) -> &mut Self::Target { | ||
&mut self.state | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is used by the new I think there are two better ways to do this:
Solution 1 is more verbose, and probably more flexible than we need. So I'd prefer solution 2. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I agree with you, solution 1 seems like it might be overkill, so I will try my hand at solution 2 and see how that goes. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hannobraun , I believe I have managed to implement all of the changes that you had suggested. Please take a look and let me know if this is what you were going for! 😄 |
||
|
||
impl<S: State> Default for Service<S> | ||
where | ||
S: Default, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,16 +3,34 @@ use std::{collections::BTreeMap, error::Error, thread}; | |
use crate::{ | ||
objects::{BehindHandle, Object}, | ||
storage::ObjectId, | ||
validate::ValidationError, | ||
validate::{ValidationConfig, ValidationError}, | ||
}; | ||
|
||
use super::State; | ||
|
||
/// Errors that occurred while validating the objects inserted into the stores | ||
#[derive(Default)] | ||
pub struct Validation { | ||
/// All unhandled validation errors | ||
pub errors: BTreeMap<ObjectId, ValidationError>, | ||
/// Optional validation config | ||
config: Option<ValidationConfig>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's probably simpler to instead have But this would be a nice-to-have. Perfectly fine to merge this as-is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has also been addressed. |
||
} | ||
|
||
impl Validation { | ||
fn with_validation_config(&mut self, config: &ValidationConfig) -> &Self { | ||
self.config = Some(*config); | ||
self | ||
} | ||
} | ||
|
||
impl Default for Validation { | ||
fn default() -> Self { | ||
let errors = BTreeMap::new(); | ||
Self { | ||
errors, | ||
config: None, | ||
} | ||
} | ||
} | ||
|
||
impl Drop for Validation { | ||
|
@@ -71,6 +89,9 @@ impl State for Validation { | |
ValidationEvent::ValidationFailed { object, err } => { | ||
self.errors.insert(object.id(), err.clone()); | ||
} | ||
ValidationEvent::ConfigurationDefined { config } => { | ||
self.with_validation_config(config); | ||
} | ||
} | ||
} | ||
} | ||
|
@@ -95,4 +116,9 @@ pub enum ValidationEvent { | |
/// The validation error | ||
err: ValidationError, | ||
}, | ||
/// A validation configuration has been defined | ||
ConfigurationDefined { | ||
/// The predefined validation configuration | ||
config: ValidationConfig, | ||
}, | ||
} |
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.
Seems a bit misleading to take a
&ValidationConfig
that is then copied inside. It would be better to directly takeValidationConfig
.But this is not a blocker. Happy to merge as-is!
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 has been addressed.