Only trigger has_model_changed when changes are actually made

[dannosaur]

It seems the has_model_changed is triggered in the __setattr__ and doesn't consider what the value was beforehand. Take this;

class Test(ChangeDetectionMixin, BaseModel):
    field1: str

obj = Test(field1='test')
obj.field1 = 'test'
obj.has_model_changed  # True

Ideally this flag would only flip if the value being set is not an exact match to what's already against that field in the model. This could facilitate code blocks that blindly set values against the model instead of having to check itself first;

incoming_data: dict  # some data coming from someplace else
obj = MyModel(**values_from_cache)
for field, value in incoming_data.dict():
    if getattr(obj, field) != value:  # this check for equality could be removed
        setattr(obj, field, value)
assert obj.has_model_changed

I might have a crack at this at some point. Just wanted to throw this out there.

[wolfskaempf]

@dannosaur I can see how has_model_changed being true in this scenario could be confusing. I would let @ddanier comment on whether this is intentional or not (and then, would be fixed).

A bit tangential: Could pydantic-apply solve your use-case in the meantime and make that part of the code a bit more elegant at the same time?

https://github.com/team23/pydantic-apply

Then, it could look like this.

incoming_data: dict  # some data coming from someplace else
obj = MyModel(**values_from_cache)
obj.model_apply(incoming_data)

[dannosaur]

See, this is why I love the open source community. Another pydantic module that's incredibly useful that I didn't know about!

It doesn't solve the use case though beyond making the code a lot nicer to look at, effectively the 2-3 lines in your sample, but if the values in incoming_data are the same as what's in values_from_cache then model_has_changed is still True.

[ddanier]

This is actually the intended behaviour. Reason for this is that we see all assignments as a change, no matter if the value did actually change. This is by design, as detecting the value as really being different may not be that simple.

Let me elaborate on this:
For simple values like strings, integers or similar the check if the value was changes is pretty much no issue. But pydantic allows us to use other models or even arbitrary classes as values. Detecting if a value has changes will create some challenger in this case:

• I have seen people using their own __eq__ implementation only checking the id field
• There may be internal state in the object that is not compared but might make a difference (like pydantic private attributes)
• Also __setattr__ is called before the field has been validated (see validate_assignment option in pydantic). This means the value we get here might be different, but the same after the validation process
• And maybe other reasons I fail to remember right now...

As of this we decided to always assume that when you write obj.attr = "value" your intention was to change that value, so we set it as changed - regardless of the value actually being different.

I know this might not be the intended behaviour for some cases, but at least each and every one of those cases can easily be found and managed (like with your code above). On the contrary if we did test the value for equality this might introduce side effects that may be hard to find and debug and lead to more headaches.

Having all that said it is actually very easy to extend the ChangeDetectionMixin to check for the value, for example like this:

class MyOwnChangeDetectionMixin(ChangeDetectionMixin):
    @no_type_check
    def __setattr__(self, name, value) -> None:  # noqa: ANN001
        # Note: You may need to also handle private attributes here

        if value == self.__dict__[name]:
            # Just return and do nothing if the value has not changed. Note this will
            # also ensure we never mess things up with values defining their own `__eq__`
            # implementation - while this also means those may not be set if the
            # implementation is somewhat strange (like only checking the `id` field).
            return

        super().__setattr__(name, value)

This also has some quirks but may be just enough for most cases and will certainly be enough for all those cases you just have strings, integers and stuff.

One additional note about pydantic-apply and its use:
I mentioned the use of validate_assignment above. When updating your model state this might get tricky when you update the fields one by one - like you do in your code. If you have a root/model validator that validates multiple fields this approach might fail. Say for example you have a validator that ensures a field x is always double the value of a field y (like assert self.x == self.y * 2). If you are updating field by field the validator will raise an issue mid update - which you cannot solve as obj.attrname = "something" only works for one field. pydantic-apply will allow this to work + still do the model validation for the complete set of field changes. ;-)

[ddanier]

Having that all said: I'm open to having this behaviour changed if we come up with a good solution. The current implementation is a little bit naive at the moment, but still is in my opinion the best reasonable compromise when thinking about all those "special" values we could have here.

[ddanier]

Some options we have:

• Have this behaviour configurable
• Only check values for simple types (string, int, ...)
• Just check value equality and see what breaks 🙈

I'm currently in favour of adding an equality check for simple types and document the behaviour.

[ddanier]

I have created #19 to implement this idea.

@dannosaur Want to test this?

[ddanier]

@dannosaur Does this work for you? Do you have any feedback?

[dannosaur]

@ddanier I haven't been ignoring you! Since I wrote this ticket things took a turn and I've ended up focusing on other things for a few days. As it happens the exact use case I had for this no longer applies, BUT I have another project (or 2) where this could be very useful so I intend to check our your proposed changes shortly (though it's a holiday in the US this weekend, so might be Monday now).

I'll post back soon :)

[ddanier]

@dannosaur No issue at all and for sure I don't want to apply any pressure on you testing this. I just thought this would make a nice addition to 0.6.0 and thus wanted to ensure it will really solve your use case. Feedback will be very much appreciated :)

[ddanier]

Released with 0.6.0