ImmutableModel mixin

[relrod]

ImmutableModel will:

• Prevent a model from being save()'d if it already has a pk.
• Also remove modified_on/modified_by fields by setting them to None in case we are inheriting from CommonModel.

This is because we have some models (such as activity stream entries and some RBAC models) that should never change because they are used as a form of audit log. This prevents them from changing preventing save() from working after there's a PK on the object.

It also insures that ImmutableModel is the first base class so MRO is satisfied.

cc @AlanCoding because maybe it's worth squeezing this into #45

[AlanCoding]

Cross-referencing with #45, I want to volunteer the fact that there is one model definition I could apply this to. That is the abstract base model for user & team role assignments. Your class could be used there, because this model inherits from CommonModel. The two other cases where I have an immutable model, ObjectRole and RoleEvaluation, the model is not a common model to begin with. By that, I mean it doesn't have a created_by field and they are very performance-sensitive. Assignments are created by the user and should be expected to have all the standard API fields.

That's all fine and all, I just want to say that the overlap exists, but is fairly small between these two things.

[AlanCoding]

Oh, or I guess this could be used as the first class for a model using the basic Django Model? That maybe I could use it in those other cases too?

[AlanCoding]

^ assuming that works as expected (I don't think it is covered by tests here), then I think it would work well with the RBAC models. But right now I really want to get that branch merged ASAP and am looking for reviews on it. At this point I would prefer to merge RBAC and then rebase & apply this to those models.

[john-westcott-iv]

It would be nice if this was in its own file and the super().save() will append modified_on/by to the model as instance variables so we may to clear those when coming back from the save.

[AlanCoding]

@john-westcott-iv 's comments about the .save method remind me of things which are very concerning.

In my 1 model that does something similar, I do:

    def __init__(self, *args, **kwargs):
        """
        Because through models are created via a bulk_create, the save method is usually not called
        to get around this, we populate the user model after initialization
        """
        super().__init__(*args, **kwargs)
        if not self.id:
            user = get_current_user()
            if user:
                # Hazard: user can be a SimpleLazyObject, so use id
                self.created_by_id = user.id
        # Cache fields from the associated object_role
        if self.object_role_id and not self.object_id:
            self.object_id = self.object_role.object_id
            self.content_type_id = self.object_role.content_type_id
            self.role_definition_id = self.object_role.role_definition_id

    def save(self, *args, **kwargs):
        if self.id:
            raise RuntimeError(f'{self._meta.verbose_name.title()} model is immutable, use RoleDefinition.give_permission method')
        # skip over CommonModel save because it would error due to missing modified_by and created
        return super(CommonModel, self).save(*args, **kwargs)

Note the super(CommonModel, self), which I've said before is, unfortunately, intended. This model bypasses all of the save logic on the CommonModel because it was a nightmare to work with. It's going to set modified_by. I don't understand what the strategy of the current PR for that is.

django-ansible-base/ansible_base/lib/abstract_models/common.py

Line 98 in 4429801

self.modified_by = user

Because of this line, I expected at least some change to CommonModel.save, if not a complete restructuring of the class inheritance.

[relrod]

It's going to set modified_by

But that's just the thing. It is going to set modified_by, but the field won't actually exist, so it won't get saved anywhere. Yes, there will be a class variable called modified_by and it will be None. When CommonModel#save does self.modified_by = ..., it's ends up just setting the instance variable modified_by to some value.

That doesn't throw an error on save().

In [1]: x = ImmutableLogEntry(message="hey")

In [2]: x.this_is_not_a_field = 42

In [3]: x.save()

In [4]:

We can make CommonModel#save check _meta.fields to see if modified_by/modified exist before running that logic at all, as a micro-optimization, but I think presentation wise it wouldn't change anything.

[relrod]

Oh, or I guess this could be used as the first class for a model using the basic Django Model? That maybe I could use it in those other cases too?

Yes the idea is that this mixin works with CommonModel and without CommonModel. The save() logic still is relevant in the non-CommonModel case.

[relrod]

At this point I would prefer to merge RBAC and then rebase & apply this to those models.

That's fine, there's no real reason to block RBAC on something like this.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[AlanCoding]

@pytest.mark.django_db
def test_created_by_immutable_model(user):
    from crum import impersonate

    with impersonate(user):
        instance = ImmutableLogEntry(message="Oh no! An important message!")

    assert instance.created_by == user

gives

__________________________________________________________________________ test_created_by_immutable_model __________________________________________________________________________

user = <User: user>

    @pytest.mark.django_db
    def test_created_by_immutable_model(user):
        from crum import impersonate
    
        with impersonate(user):
            instance = ImmutableLogEntry(message="Oh no! An important message!")
    
>       assert instance.created_by == user
E       assert None == <User: user>
E        +  where None = <ImmutableLogEntry: ImmutableLogEntry object (None)>.created_by

test_app/tests/lib/abstract_models/test_immutable.py:61: AssertionError

My 2 cents here is that if it's possible to avoid doing "fancy" things like __getattribute__ or __init_subclass__, then it would be better to use the simpler approach. It seems like you are not modifying CommonModel.save. This problem, philosophically, can not be solved without making changes there. If self.modified_by is going to give AttributeError, then you have to avoid referencing it in CommonModel but still run ordinary logic on self.created_by. I don't know if a mixin is a good approach for this. The 2 approaches I would first consider are:

• a subclass of CommonModel that undoes some of what it does for modified/modified_by. Say it doesn't set self.modified_by on some if check, I don't know what, but it's possible, and likely tolerable.
• break up CommonModel into the created and modified functionality. Those two classes would combine into CommonModel but only the "created" class would be used in the immutable models.

[relrod]

E assert None == <User: user>
E + where None = <ImmutableLogEntry: ImmutableLogEntry object (None)>.created_by

But this happens even before this change.

On devel:

from test_app.models import RelatedFieldsTestModel

@pytest.mark.django_db
def test_created_by_immutable_model(user):
    from crum import impersonate

    with impersonate(user):
        instance = RelatedFieldsTestModel()

    assert instance.created_by == user

fails with the same error, because created_by only gets set after .save() is called.

Your test passes if you instance.save() before the assert.

[AlanCoding]

Oh, I should have done objects.create, yeah

[relrod]

a subclass of CommonModel that undoes some of what it does for modified/modified_by. Say it doesn't set self.modified_by on some if check, I don't know what, but it's possible, and likely tolerable.

If you know of a way to prevent the field from getting created early enough (before Django's metaclass logic runs on the model class), then I'm open to that. I was unable to figure out a way to do that.

break up CommonModel into the created and modified functionality. Those two classes would combine into CommonModel but only the "created" class would be used in the immutable models.

This could work if we did some refactoring of CommonModel where we abstract out the summary/related field stuff, too. We'd probably need an AbstractCommonModel that has that logic, and CommonModel inherits that, too.

[relrod]

This could work if we did some refactoring of CommonModel where we abstract out the summary/related field stuff, too. We'd probably need an AbstractCommonModel that has that logic, and CommonModel inherits that, too.

I put up #225 for that, if you want to go that route instead.

[AlanCoding]

I think it would be good to add that test that created_by gets set correctly after a save. That's pretty central to what this accomplishes. I don't necessarily want to request changes here, I just don't understand it. If setting modified_by raises an exception... then how is this working? How does created_by get set, when it's set by basically the same mechanism as modified_by?

[relrod]

I almost prefer the approach in #225 now. Maybe @john-westcott-iv has an opinion.

[AlanCoding]

Clarified in call - in this implementation self.modified_by does get set. The AttributeError only happens on a getattr and not on a setattr.

That answers my question above. I sill ask that this adds a test to assert created_by is set before merging this or the other PR.

That said, right now I have no particular preference between the two. For this one, I'm not sure if it's worth it to override __getattribute__ to get the exception. But that's a mild preference.

[relrod]

Closing in favor of #225, I think it's better in the long run.