django-fsm icon indicating copy to clipboard operation
django-fsm copied to clipboard

Override protection mechanism

Open Nnonexistent opened this issue 8 years ago • 4 comments

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.

Nnonexistent avatar May 19 '17 14:05 Nnonexistent

refs #89

Nnonexistent avatar Jun 06 '17 15:06 Nnonexistent

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 avatar Jun 08 '17 09:06 kmmbvnr

@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_db and clean_fields with such decorator in contribute_to_class hook

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.

Nnonexistent avatar Jun 08 '17 16:06 Nnonexistent

I would like to second that

  • I consider protected=True worthy (and a great protection against accidental bypass of the FSM)
  • I am fighting with ways of getting .refresh_fo_db to work despite protected=True as 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=True and 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).

hartwork avatar Dec 05 '19 20:12 hartwork

That's kind of things are thread and async unsafe and unfortunately can't be in a library that hides this from the users

kmmbvnr avatar Dec 21 '23 08:12 kmmbvnr