🦺 Initial request validation

[evaline-ju]

• Update API documentation to reflect updated error handling. Auto-generated Fast API objects no longer needed
• Provide some upfront request validation of masks and required parameters fields, with some unit tests
• Update types - fields that cannot be less than 0 are now updated to be unsigned. models are required (removed Option) under input and output and will now result in a validation error if not present

Part of: #35

[Please squash merge!]

[declark1]

Hmm, I'm personally not a fan of adding garde for validation unless it provides some feature(s) that we really need. Rust's type system can be leaned on for many things, such as:

#[garde(range(min = 0))]
pub max_new_tokens: Option<i32>,

Rather, this should just be an unsigned Option<u32> instead of checking min = 0.

Below, a type is being marked as "required" despite it being an Option field, so the intent isn't clear. We should just leverage Rust's Option when things are optional, all else being required.

    #[serde(rename = "models")]
    #[serde(skip_serializing_if = "Option::is_none")]
    #[garde(required)] // output field must have `models`
    pub models: Option<HashMap<String, DetectorParams>>,

Also, the garde derive macros add (even more) clutter to the struct fields (especially #[garde(skip) and the mysterious #[garde(dive)] which I see added to many fields).

For stuff like below, I'd say implementing simple rules in the validate method is better.

instead of this:

   #[garde(length(min = 1))]
    pub model_id: String,

we can do this:

pub fn validate(&self) -> Result<(), Error> {
   if self.model_id.is_empty() {
      return Error::Validation("`model_id` must be provided")
   }
   // other rules
}

The "newtype" pattern is also common in idiomatic Rust for cases like the non-empty String (if reused a lot), e.g. you may create a NonEmptyString newtype unit struct that wraps a String to provide the non-empty guarantee.

• Of course there are crates for the common ones like this, e.g. https://docs.rs/non-empty-string/latest/non_empty_string/struct.NonEmptyString.html, but it's easy enough to implement too.

As a side-effect of deriving garde::Validate, I see you had to name the validate method upfront_validate() and call into self.validate() to run garde's validation. IMO, it feels kind of blackbox'ish vs. a chain of explicit rules.

[evaline-ju]

Below, a type is being marked as "required" despite it being an Option field, so the intent isn't clear. We should just leverage Rust's Option when things are optional, all else being required.

Sure, part of the thinking here had been if some of these API structs were to be updated potentially via auto-generation when the API yamls were updated, the structs themselves with Option could kept, while the requirements of the orchestrator API itself could be enforced through validation. We could certainly encode some of the rules into the types themselves (unsigned, no Option etc.)

I agree that the verboseness of the skip needed on fields to not validate was not ideal in the case where we had fairly large structs. [garde(dive)] was meant to provide validation for nested/inner structs.

[declark1]

Below, a type is being marked as "required" despite it being an Option field, so the intent isn't clear. We should just leverage Rust's Option when things are optional, all else being required.

Sure, part of the thinking here had been if some of these API structs were to be updated potentially via auto-generation when the API yamls were updated, the structs themselves with Option could kept, while the requirements of the orchestrator API itself could be enforced through validation. We could certainly encode some of the rules into the types themselves (unsigned, no Option etc.)

I agree that the verboseness of the skip needed on fields to not validate was not ideal in the case where we had fairly large structs. [garde(dive)] was meant to provide validation for nested/inner structs.

Yea, the auto-generation is also bad as it's generating a lot of garbage such as unnecessary field renames (the fields already have these names), unnecessary Option's, unnecessary derives, signed integers for things that should be unsigned, etc. models.rs could be a lot cleaner. Considering these types don't appear hard to maintain at all, i.e. they don't have 100 fields, do we really need to auto-generate going forward? I guess it depends on how frequently the API is going to change?

[declark1]

Just thought of something. Won’t auto-generation overwrite all of these changes to the model types, such as the garde derives, validation method, etc?

[evaline-ju]

Just thought of something. Won’t auto-generation overwrite all of these changes to the model types, such as the garde derives, validation method, etc?

I suppose it depends on how completely auto-generated the updates are, but regardless I've gone ahead and refactored without the extra crate, depending instead on updated types

[declark1]

This looks good to me. I think this makes sense to keep it simple instead of using a validation crate.

I typically avoid using errors from other modules (server::Error), but it doesn’t really matter here as the models module can really be considered part of the server.

Another option is to crate a ValidationError type in models.rs:

#[derive(Debug, thiserror::Error)]
pub enum ValidationError {
    #[error("`{0}` is required")]
    Required(String),
    #[error("{0}")]
    Invalid(String),
    // etc
}
//...

and then in server.rs, impl a conversion into server::Error so that you can still call request.validate()?:

//...
impl From<models::ValidationError> for Error {
    fn from(value: models::ValidationError) -> Self {
        Self::Validation(value.to_string())
    }
}

[evaline-ju]

but it doesn’t really matter here as the models module can really be considered part of the server.

I had generally assumed this, so I think I'll merge as is - certainly open to having the refactored in the future