Model.Update should not inject version condition

[ikonst]

The problem

The VersionAttribute design pattern consists of

• incrementing the version with every change — to trip others so they don't overwrite your change
• conditioning on the version — to trip yourself so you don't overwrite others' change

There are two methods that can modify an existing model:

• Model.save
• Model.update

We've observed it to be a common pattern to change just a few fields and then call Model.save. In this case, a version condition is a good precaution to avoid overwriting more attributes than you meant to. Once users start seeing high % of conditional failures ("conflicts"), a more nuanced approach is called for, using Model.update. Often "last write wins" is good enough as long as you don't write unrelated attributes. Other times, a more nuanced condition can be used (e.g. update field X only if it had value Y before).

Unfortunately when we introduced VersionAttribute, we’ve made Model.update append a condition on the version attribute. This is probably counter-productive to what I described above.

The proposal

In PynamoDB 6.0, we can allow changing this behavior.

Options:

1. Stop appending the condition in Model.update. This risks silently breaking existing apps.
2. Make it optional and default to append. This drags the bad behavior as a default into green-field apps.
3. Make it optional and default to not appending. This risks silently breaking existing apps.
4. Ask user to decide, provide no default. 📌

I'm inclined towards option (4). Adding a parameter to Model.update is disruptive, and also risks adding a hidden crash, since not every Model.update call is necessarily exercised by tests. Instead, we can add a parameter to VersionAttribute, since it's executed at class load / module load, and likely prevent an app from starting up until its maintainer makes a conscious decision.

We should name it something like legacy_update_behavior: bool as to discourage setting it to True in green-field apps:

-version = VersionAttribute()
+version = VersionAttribute(legacy_update_behavior=False)

In future versions we could switch to Option 1 by removing it.

[garrettheel]

#4 seems the best option to me too, though I'd suggest we just call this enforce_on_update or similar, because:

1. It's not obvious to me that it's always more correct to omit the condition from updates. This seems fairly use-case dependent and I can imagine some might want a stricter enforcement of versions here (e.g. maybe a set of updates still affect a majority of attributes and last-write-win is not sufficient)
2. The parameter name "legacy_behavior" doesn't reveal much about what the actual behaviour is

[ikonst]

Interesting point, re 'not always more correct'. I'd like green-field uses to get a better default, but perhaps I'm wrong thinking it's a "better default"? If we think it would be safer for newbies to have the condition check even for update, then perhaps the right approach is a non-breaking one — extra parameter to Model.update allowing opting out of the condition?

-def update(self, ...):
+def update(self, ..., add_version_condition: bool = True):