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

Discussion: Model Validation #335

Closed
gm0stache opened this issue May 31, 2023 · 2 comments
Closed

Discussion: Model Validation #335

gm0stache opened this issue May 31, 2023 · 2 comments

Comments

@gm0stache
Copy link
Collaborator

gm0stache commented May 31, 2023

Hi there!
Am I the only one who would appreciate the models to implement some kine of model validation options?
Stumbled upon IValidatableObject - wouldn't it be nice to have that?

I stumbled upon an issues in our system where a Marktlokation contained a Messlokation with a different Sparte than the Marktlokation itself. Such logical restrictions could easily be enforced with such an universal validation mechanis as provided with the IValidatableObject interface.

Let me know what you think!

BR
Gabriel

@hf-kklein
Copy link
Collaborator

hf-kklein commented May 31, 2023

Yeah, generally great idea. The problem - as always - is in the details.

Historically we had a lot of more validation going on but back then we did it just wrong instead of right. I think to some degrees in the "older" models this wrong validation approach is still in the code today. We used the Required-Attriubtes (and wild wild methods called on deserialization) from our JSON-Framework to validate single models. Meaning: You could not (de)serialize anything where e.g. certain properties weren't set. The validation and the serialization were coupled to each other.

it was annoying for a couple of reasons:

  • Validation and Serialization should not be coupled. To validate you had to try to serialize, to serialize you had to obey the validation rules. It was hella annyoing.
  • Generally it turned out that users prefer to serialize whatever they like and only care about validation (properties being set), when they're explicitly do a validation (e.g. in a services middleware). It's hard to design a library whose one validation approach fits all use cases. This is why we (for newer objects) totally moved away from required fields. For newer models basically every property is nullable. This allows users of the library to write their own validation without being "gegängelt" by our assumptions on what might be required. Fachliche Pflichtfelder komplett streichen bo4e/BO4E-python#465
  • But this also means, that all the model validation we originally had, is now gone as there are no required fields left in the newer models.

The thing with a validation is: how could you in this library foresee which validations are actually relevant to different users and use cases? Maybe in someones use case connecting a Malo from Gas with a MeLo from Strom is actually a valid data constellation. So I wouldn't like to go back to mandatory validation here.

My suggestion would be: Build your own composite models and write your own validations for those.

public class MyCompositeFoo : IValidatableObject
{
    public Messlokation MeLo {get;set;}
    public Marktlokation MaLo {get;set;}
    public IEnuerable<ValidationResult> Validate(...)
    {
    }
}

BTW: There is an old ticket from 3 years ago, that has the same idea as you: #33

@gm0stache
Copy link
Collaborator Author

gm0stache commented May 31, 2023

Get your point!
Was also thinking about the composite approach.

Thanks for your response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants