multi table inheritance

[rrauenza]

This isn't so much of a request, but to just log my findings of getting this to work with multi table inheritance.

First, you will need to pass related_name='+' to track() in order to get pgh_obj from having conflicting related names.

Second, we need to limit the fields tracked. One way would be to change pghistory to pass include_parents=False to _meta.get_fields(), but it should also be able to just do this at the track() level as well with a decorator to wrap the pghistory decorator.

Another possibility... but I think it would not be wanted .. is to automatically walk _meta.get_parent_list and decorate the parents. I think it is probably best to make that a manual step in the hierarchy.

And best of all -- avoid multitable inheritance :)

(OH! You can't call get_fields() yet: AppRegistryNotReady ... so still trying to figure out what to so here.)

[rrauenza]

This seems to be working at the moment -- working through unit tests. I have to use model.__name__ because model.__class__ seems to always return <class 'polymorphic.base.PolymorphicModelBase'> in my environment.

def _enable_pghistory(self, model):                                                   
     event = pghistory.Snapshot('%sSnapshot' % model.__name__)                         
     model_name = "%s_PGHistory" % model.__name__                                      
     fields = [f.name for f in model._meta.fields if f.name not in self.exclude_fields and f.model is model]
     return pghistory.track(event, model_name=model_name, fields=fields, related_name='+')(model)

[pierreben]

Hi,

I encountered a bug while using django-pghistory with model inheritance as explained by @rrauenza in this issue.

I've worked on a fix in that branch : master...pierreben:fix-6-multi-table-inheritance.
I'd like to create a pull-request to handle this specific new case in the package.

Existing tests are running OK.
I've added some new models with inheritance to validate that the errors occurring with the last release version were not present any more, but I think that it would be good to add some more tests for the aggregator queries.
I'm able to do so, but I'd prefer to check with you where to add such tests (new tests in test_models ?) to be sure that I don't do a mess in the code.

I'd like also to have your feedback on the work I've made to be sure that I haven't miss something by improving the package.

Many thanks for this amazing package, and for your help to finish my work / create a PR to improve it !

[wesleykendall]

Thanks for the suggestions @rrauenza and the PR @pierreben ! I'm still wrapping my head around #36 a bit and will get back. I think the changes to make it work for child models are straightforward, but you're right that we get into a weird scenario when talking about tracking changes to child and parent models. IMO users should explicitly track all of them if that's what they want, but it may be difficult to piece them all together.

@pierreben give me a little more time to try to understand how your aggregate event changes solves for the inherited tables. My understanding is that it joins them all together, but I haven't dug into the code enough yet. Been going through other trigger/history PRs today and ended with this one.

[wesleykendall]

FYI this issue is still on my radar. I'm taking some time this week to really polish pghistory and handle some more advanced use cases like multi-table inheritance. Will report back here when I have a feature planned for this. I do agree it's import that this library can support this use case

[duebbert]

Would these changes allow me to define a base model with tracking which can then be used by child models?

I'm planning to switch from Django Simple History and have an abstract model where it's enabled and all other models inherit from it. Something like this:

class TrackedBaseModel(models.Model):
    class Meta:
         abstract = True

    history: Manager = HistoricalRecords(inherit=True, related_name="history_set")

Then I'm using it in other models:

class Project(TrackedBaseModel):
   ...

I'm hoping to avoid having to define the decorator for all my models.

Thanks!

[pierreben]

Would these changes allow me to define a base model with tracking which can then be used by child models?

I'm planning to switch from Django Simple History and have an abstract model where it's enabled and all other models inherit from it. Something like this:

class TrackedBaseModel(models.Model):
    class Meta:
         abstract = True

    history: Manager = HistoricalRecords(inherit=True, related_name="history_set")

Then I'm using it in other models:

class Project(TrackedBaseModel):
   ...

I'm hoping to avoid having to define the decorator for all my models.

Thanks!

No, the changes discussed in this issue don't concern abstract models inheritance (as in your example) but "normal" models inheritance.

For now, as far as I know you effectively need to register all of your child models with the decorator.

[olivierdalang]

@wesleykendall Hey !! Thanks for the package.

I've read through this issue as well as the docs, but here I can't make it work with multi-table inheritance at all (while the docs seems to say that while not well supported, it should work if all involved tables are tracked).

The following very much stripped down example errors when I run makemigrations

@pghistory.track()
class Parent(models.Model):
    field_a = models.CharField(default="unknown")


@pghistory.track()
class Child(Parent):
    field_b = models.CharField(default="unknown")

Error:

mmp_metadata.ChildEvent.pgh_obj: (fields.E304) Reverse accessor 'Child.events' for 'mmp_metadata.ChildEvent.pgh_obj' clashes with reverse accessor for 'mmp_metadata.ParentEvent.pgh_obj'.
        HINT: Add or change a related_name argument to the definition for 'mmp_metadata.ChildEvent.pgh_obj' or 'mmp_metadata.ParentEvent.pgh_obj'.
mmp_metadata.ChildEvent.pgh_obj: (fields.E305) Reverse query name for 'mmp_metadata.ChildEvent.pgh_obj' clashes with reverse query name for 'mmp_metadata.ParentEvent.pgh_obj'.
        HINT: Add or change a related_name argument to the definition for 'mmp_metadata.ChildEvent.pgh_obj' or 'mmp_metadata.ParentEvent.pgh_obj'.

OP mentionned adding related_name='+' to track(), but that's not an expected argument.

Any pointer as how to move forward ?

Let me know if you'd rather like a new issue (not really sure from the name of this one what its scope is).

[xaitec]

This worked for me, hope it helps. Please use as part of documentation if this is the prefered method.

@pghistory.track(
    obj_field=pghistory.ObjForeignKey(
        related_name="parent_event",
        related_query_name="parent_event_query",
    )
)
class Parent(models.Model):
    field_a = models.CharField(default="unknown")


@pghistory.track(
    obj_field=pghistory.ObjForeignKey(
        related_name="child_event",
        related_query_name="child_event_query",
    )
)
class Child(Parent):
    field_b = models.CharField(default="unknown")

[wesleykendall]

Following up here, thanks @xaitec . Your doc changes in the FAQ are deployed here in v3.3.0

I'm going to close out this thread for now and associated PRs. I hope that some of the inline configuration changes (i.e. obj_field= can help with the boilerplate associated with concrete inheritance.

It's been a while since this original thread. If there are any thoughts for better support for concrete inheritance, I've added discussions to pghistory, so feel free to open a discussion about it.

[xaitec]

Sure, you can close it. Thanks for being my first commit to a project 😁