hannobraun/fornjot

Improve validation infrastructure

hannobraun opened this issue · 3 comments

Validation is an important part of Fornjot. The validation code checks shapes for validity, surfacing issues that would lead to problems later on, for example an invalid mesh when exporting to STL or 3MF.

The validation checks are currently organized per-object (each check checks a specific type of object), and they are all structured about the same. I have an idea for updating the validation infrastructure, organizing it per-check, which would have some advantages.

Concept

This concept essentially consists of two parts:

1. There's one type per validation check

Validation checks (of which there are multiple per object) are currently represented as methods on the object-specific validation error type. Following the new concept, each validation check would be represented by a dedicated type.

struct SomeValidationCheck {
    // fields with relevant information on possible validation failure go here
}

I think it might make sense to have a dedicated module per validation check, instead of a module per object, as it is right now. Validation check could then also be shared among several objects. For example, Sketch and Solid have related checks that make sure objects that shouldn't be referenced by multiple other objects aren't, for example, and those could be shared under that new model.

Open question: Would "validation check" be the best name for that category of type, or would we call them "validation error", since they contain error information?

2. Validate trait is parameterized per validation check

The updated validation trait could look something like this:

trait Validate<T> where T: Into<ValidationError> {
    fn validate(&self, errors: &mut Vec<ValidationError>);

    // Disregarding `ValidationConfig`, for simplicity.
    // If implemented, `ValidationConfig` might be included similarly as it is now.

And it would be implemented per-check like this:

impl Validate<SomeValidationCheck> for SomeObject {
    // ...
}

// same validation check, shared by another object
impl Validate<SomeValidationCheck> for OtherObject {
    // ...
}

Open questions:

  • Do we use ValidationError in the trait (with the T: Into<ValidationError> as specified), or does the trait method take errors: &mut Vec<T> and we do the conversion elsewhere?
  • Right now there's a list of checks in the Validate implementation for an object. Where would we have code to run all validation checks for an object in the future? Maybe there's two levels of traits? Validate<T> and ValidateAll? If so, the latter could also do the conversion into ValidationError.

Advantages

This concept would have a few advantages:

  • There is now a dedicated place, which would show up in cargo doc output, to document each validation check in as much detail as required. The documentation of the objects themselves would no longer be burdened with this, and could instead just reference the validation checks, adding context where necessary.
  • Validation checks would be more discoverable. Instead of being private code that is explained in object-specific documentation, each check that relates to an object would show up in the object's documentation page, since all trait implementations are listed there.
  • It becomes easier to share code between similar validation checks for different objects.

Next Steps

There are still open questions, so this will require more thought and experimentation. Therefore, I'm opening this as a planning issue.

I probably won't work on this for now, so if anybody's interested, feel free to speak up!

I've started working on this.

Progress here has been slow, as this issue is more of a side project for me, but work is ongoing. I've defined how validation checks work under the new infrastructure, and just ported the second validation check (#2277), and I'm happy with how that works.

From now, this is mostly a matter of porting the rest of the checks to the new infrastructure, then figuring out how to port the per-object Validate trait over. But that's not going to be much of a problem either way.

Re-tagging this issue, to reflect that it's no longer about planning, but actual implementation.

I've been picking up the pace here again this week. See pull requests referenced above for details.

Most of the validation checks have been ported to the new validation infrastructure! Just finished porting all shell validation tests, and only sketch and solid validation tests are left.