Skip to content
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

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
14 changes: 13 additions & 1 deletion crates/fj-core/src/services/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ mod validation;

use crate::{
objects::{Object, Objects, WithHandle},
validate::ValidationErrors,
validate::{ValidationConfig, ValidationErrors},
};

pub use self::{
Expand Down Expand Up @@ -42,6 +42,18 @@ impl Services {
}
}

/// Construct an instance of `Services` with a pre-defined configuration for the validation service
pub fn with_validation_config(config: &ValidationConfig) -> Self {
Copy link
Owner

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 take ValidationConfig.

But this is not a blocker. Happy to merge as-is!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been addressed.

let objects = Service::<Objects>::default();
let mut validation = Service::<Validation>::default();
validation
.evolve(&ValidationEvent::ConfigurationDefined { config: *config });
Self {
objects,
validation,
}
}

/// Insert an object into the stores
pub fn insert_object(&mut self, object: Object<WithHandle>) {
let mut object_events = Vec::new();
Expand Down
8 changes: 7 additions & 1 deletion crates/fj-core/src/services/service.rs
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
///
Expand Down Expand Up @@ -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
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think this is used by the new Services constructor, which in itself seems fine. But this provides direct access to service state to all code, which seems error-prone.

I think there are two better ways to do this:

  1. Define a command for updating the validation config (see the existing ValidationCommand), make the execution of that command emit ValidationEvent::ConfigurationDefined, and use Service::execute to update the validation.
  2. In the new Services constructor, instead of using Service::<Validation>::default(), initialize the validation service with Service::new, which can be provided with a Validation that has the configuration set properly.

Solution 1 is more verbose, and probably more flexible than we need. So I'd prefer solution 2. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
Expand Down
30 changes: 28 additions & 2 deletions crates/fj-core/src/services/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Copy link
Owner

Choose a reason for hiding this comment

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

It's probably simpler to instead have config: ValidationConfig, and initialize that with its default value, if not provided.

But this would be a nice-to-have. Perfectly fine to merge this as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 {
Expand Down Expand Up @@ -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);
}
}
}
}
Expand All @@ -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,
},
}