django-fsm
django-fsm copied to clipboard
Override protection mechanism
Refs #174
Fix for refresh_from_db and Model.full_clean with protected fields.
I was unable to found better solution than to wrap refresh_from_db to set hidden flag on instance's __dict__ and look for this flag in protection checking.
Test provided.
Update:
Renamed PR as it now solves more general issue.
Moved flags that indicates overriding to a registry under instance.__dict__. This looks much better now. Also added context manager for convenient protection overriding.
refs #89
I'm still thinking about should this be changed or not. At leat incompatibility of refresh_from_db and protected=True is in the documentation.
And the proposed solution looks too magical and to intruding to the user defined models.
If I would like to see that fixed, I'll implement the refresh_from_db as mixin for a model class. This would be explicit and no surprise solution for developers.
@kmmbvnr thanks for reviewing.
1. It's not all about refresh_from_db. There are more problems with inability to override protection. For example, Model.full_clean will also fail with protected field.
But the main reason is that any third-party django app will be hard to use with protected fields without handy override mechanics. This PR, for example, starts with investigation of interference of django-fsm and modeltranslation. Though this case seems rare I sure there are a lot more possible scenarios.
I personally like "protected" feature, though it's not very pythonic. But inability to use it in real projects upsets me greatly.
2. About magic and implicitness. To my point of view the proposed solution is mostly straightforward:
- to override protection set the flag in model instance
__dict__. To minimize namespace pollution use one registry for all possible fields in model - in value assignment look for flags in model instance to determine if protection is overridden
- cleanup and handle case of multiple wraps
- provide context manager/decorator to override protection for limited scope
- to automate process, wraps
refresh_from_dbandclean_fieldswith such decorator incontribute_to_classhook
If you dislike the level of complexity or code style I'm ready to rework my proposal. But to do this I need at least to understand what seems to be unclear.
I would like to second that
- I consider
protected=Trueworthy (and a great protection against accidental bypass of the FSM) - I am fighting with ways of getting
.refresh_fo_dbto work despiteprotected=Trueas well (and it's little fun that it's necessary) - Use of django-fsm and a truly RESTful API (e.g. using DRF serializers) also needs extra magic to work with
protected=Trueand to not bypass the FSM
From a quick look through the pull request it is not clear to me what "overriding protection" would really mean and I would ask to find less abstract naming and/or concept for this thing (and maybe also a pull request title that explains itself more — from the title alone, I had no idea this was even related to .refresh_from_db, for instance).
That's kind of things are thread and async unsafe and unfortunately can't be in a library that hides this from the users