Should properties validate against their model?

[rwb27]

Currently, lt.property only validates values sent over HTTP. Values set in Python are accepted as-is, so we do no validation.

We could change this behaviour to use the model for the property to validate it when it's set. This would be a change to behaviour and might incur a small performance penalty. However, in exchange we'd get two advantages:

1. We'd know that the value of a property has always been validated against the model (which includes e.g. numeric constraints).
2. Properties whose value is a BaseModel subclass could be set by supplying a dict as well as supplying a BaseModel (though mypy would complain if this is done explicitly).

Opinions welcome.

[julianstirling]

I think this is a really bad idea. Pydantic causing weird hard to track down error messages dominates dev time already. Type hinting and type checking should be enough to check something is valid, in the edge case of a poorly defined type hinting the server falling over with a oodles of Pydantic warnings is the worst possible scenario.

Sorry I read this as "lt.actions should validate their model when returning". This is not what it says! Sorry!

If we do this it would need to be checked that any errors that are raised by pydantic are caught by LabThings and context is added. Otherwise we tend to end up with a stack trace that points to all the labthings code an never mentions and check that the line causeing the error really is in the stack trace

[rwb27]

sure, we'd definitely want to do some checking that the error message is (1) helpful and (2) appears to come from the right place. Probably handling the ValidationError and raising a custom ValueError subclass that clearly indicates the thing and property, and makes it relatively obvious what the validation error actually was.

Error handling in pydantic/fastapi (and especially the combination) often makes really long and horrible stack traces, most of which is inside pydantic or fastapi. I'm comfortable I can make the exception sensible, what I have less of a handle on is how appropriate (or not) it is to try to suppress the irrelevant bits of the stack trace. I think that would deliver a much-improved user experience, but I'm also aware that manipulating the stack trace of an error could cause confusion in the event that there is a bug in LabThings or an underlying library...

I think it makes sense to consider this alongside adding side-effects to properties, which is probably lower-priority than a couple of other things that you're likely to need for making scanning more modular in the OFM.

[julianstirling]

I think in this case if an action says "self.my_property = 70" and we get a validation error then at least that line will be somewhere in the stack trace. I think it is where things are called over HTTP and the pydantic error happens after all project code finishes, then the offending line is not in the stack trace as that bit of the stack has resolved successfully. That is a different issue though.

[rwb27]

As of #269 we have a validate() function for properties, so this would be quite simple to implement.

[rwb27]

Discussion with @julianstirling elsewhere brought up that #258 validates settings on write indirectly, because they're validated when we use the model to dump the setting to disk. I think that suggests it would be a good idea to validate settings and properties when set from Python as well as when set from HTTP.

I'd have no issue with providing an opt-out to skip validation if it turns out to be needed.

[rwb27]

Validation on write is now implemented in !278 as an optional feature. I think this may be closed for now, we can have another discussion in a dev cycle or two if we propose to make that behaviour default.