praekeltfoundation / vumi

Messaging engine for the delivery of SMS, Star Menu and chat messages to diverse audiences in emerging markets and beyond.
BSD 3-Clause "New" or "Revised" License
420 stars 131 forks source link

Computed value fields for models #944

Closed jerith closed 9 years ago

jerith commented 9 years ago

We currently do this very hackily in places and a cleaner solution would be good.

jerith commented 9 years ago

This approach doesn't handle multiple index values, so I need to do something different here. Sorry. :-/

jerith commented 9 years ago

This is ready for another review.

JayH5 commented 9 years ago

:+1: from me :)

hodgestar commented 9 years ago

Why don't we just update the value on get or save instead of every time someone updates any field on the model?

jerith commented 9 years ago

Why don't we just update the value on get or save instead of every time someone updates any field on the model?

I tried that before switching to update-on-value-change and ran into problems trying to intercept all accesses to object data (including things that look into the the underlying Riak object directly) to make sure updates didn't get missed. It ended up being cleaner doing it this way.

hodgestar commented 9 years ago

I tried that before switching to update-on-value-change and ran into problems trying to intercept all accesses to object data (including things that look into the the underlying Riak object directly) to make sure updates didn't get missed. It ended up being cleaner doing it this way.

But the only get we need to worry about is getting the computed value itself? Having a pre-save hook might also allow us to add cross-field validation later, which would be cool.

jerith commented 9 years ago

But the only get we need to worry about is getting the computed value itself? Having a pre-save hook might also allow us to add cross-field validation later, which would be cool.

We need to make sure that everything we do with the object data, including the low-level operations that bypass fields (saving, migration, etc.) has the right values and metadata for all fields. Hooking into the various things that read that is harder than triggering updates on writes through the model object. (As I said, I tried this and it ended up being more complicated than I was happy with.)

More importantly, it's a good idea to catch validation or computation errors when the data is modified instead of some future time when we try to read a field or save the object.

hodgestar commented 9 years ago

One can't do cross-field validation when a single field is updated.

jerith commented 9 years ago

This PR isn't about cross-field validation. Even if it were, we'd want the validation to happen before the update and not at the next read or save at some arbitrary point in the future.

hodgestar commented 9 years ago

:+1: