Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HistoricOneToOneField #1031

Open
valberg opened this issue Aug 24, 2022 · 8 comments
Open

HistoricOneToOneField #1031

valberg opened this issue Aug 24, 2022 · 8 comments

Comments

@valberg
Copy link
Contributor

valberg commented Aug 24, 2022

Problem Statement
We have just gotten HistoricForeignKey which is great! It would be nice to have a similar for OneToOneField.

Describe the solution you'd like
Implementation of HistoricOneToOneField which does the same as HistoricForeignKey just within the bounds of a OneToOneField.

@quadracik
Copy link

quadracik commented Apr 12, 2023

Hi, here's my implementation, basing on HistoricForeignKey. I'm still testing it, but feel free to take a look.

class HistoricForwardOneToOneDescriptor(ForwardOneToOneDescriptor):
    """
    Overrides get_queryset to provide historic query support, should the
    instance be historic (and therefore was generated by a timepoint query)
    and the other side of the relation also uses a history manager.
    """

    def get_queryset(self, **hints) -> QuerySet:
        instance = hints.get("instance")
        if instance:
            history = getattr(instance, SIMPLE_HISTORY_REVERSE_ATTR_NAME, None)
            histmgr = getattr(
                self.field.remote_field.model,
                getattr(
                    self.field.remote_field.model._meta,
                    "simple_history_manager_attribute",
                    "_notthere",
                ),
                None,
            )
            if history and histmgr:
                return histmgr.as_of(getattr(history, "_as_of", history.history_date))
        return super().get_queryset(**hints)


class HistoricReverseOneToOneDescriptor(ReverseOneToOneDescriptor):
    """
    Overrides get_queryset to provide historic query support, should the
    instance be historic (and therefore was generated by a timepoint query)
    and the other side of the relation also uses a history manager.
    """

    def get_queryset(self, **hints):
        instance = hints.get("instance")
        if instance:
            try:
                return instance._prefetched_objects_cache[
                    self.related.field.remote_field.get_cache_name()
                ]
            except (AttributeError, KeyError):
                history = getattr(
                    instance, SIMPLE_HISTORY_REVERSE_ATTR_NAME, None
                )
                histmgr = getattr(
                    self.related.model,
                    getattr(
                        self.related.model._meta,
                        "simple_history_manager_attribute",
                        "_notthere",
                    ),
                    None,
                )
                if history and histmgr:
                    queryset = histmgr.as_of(
                        getattr(history, "_as_of", history.history_date)
                    )
        return super().get_queryset(**hints)


class HistoricOneToOneField(models.OneToOneField):
    """
    Allows one to one foreign keys to work properly from a historic
    instance.

    If you use as_of queries to extract historical instances from
    a model, and you have other models that are related by one to
    one foreign key and also historic, changing them to
    a HistoricOneToOneField field type will allow you to naturally
    cross the relationship boundary at the same point in time
    as the origin instance.

    A historic instance maintains an attribute ("_historic") when
    it is historic, holding the historic record instance and the
    timepoint used to query it ("_as_of").  HistoricOneToOneField
    looks for this and uses an as_of query against the related
    object so the relationship is assessed at the same timepoint.
    """

    forward_related_accessor_class = HistoricForwardOneToOneDescriptor
    related_accessor_class = HistoricReverseOneToOneDescriptor

@JordanHyatt
Copy link

Sorry I didn't see this when I opened issue #1389

@quadracik
Copy link

No problem. We've been using it in production for over a year and I didn't experience any problems with it. Unfortunately I don't have enought free time to write propper tests and open a good pull request.

@JordanHyatt
Copy link

So I am trying to write the pull request now. The HistoricForwardOneToOneDescriptor you wrote seems to be working fine. However, I think you have a bug in your HistoricReverseOneToOneDescriptor class.

if history and histmgr:
      queryset = histmgr.as_of(
          getattr(history, "_as_of", history.history_date)
      )               

I think you should be returning here and not setting it to the variable "queryset". So I think you are always just getting the non-historical queryset because the super is always called.

when I return instead, there are errors. So I am working on those now.

@JordanHyatt
Copy link

Found it....you I think you needed self.related.related_model instead of self.related.model

@quadracik
Copy link

@JordanHyatt , actually now you mentioned it, I checked and the final solution we use also returns the queryset instead assigning it and uses self.related.related_model.
There is one more difference - the final code doesn't do try .. except. Instead, it only does what is in the except clause. I don't remember why, but it must have been causing some problems.. Or maybe it only applies to reverse FKs as they may have prefetch_related used on them and for 1to1 it never goes this way. Not sure now. But please have it in mind.
My final code for HistoricReverseOneToOneDescriptor below:

class HistoricReverseOneToOneDescriptor(ReverseOneToOneDescriptor):
    """
    Overrides get_queryset to provide historic query support, should the
    instance be historic (and therefore was generated by a timepoint query)
    and the other side of the relation also uses a history manager.
    """

    def get_queryset(self, **hints):
        instance = hints.get("instance")
        if instance:
            history = getattr(
                instance, SIMPLE_HISTORY_REVERSE_ATTR_NAME, None
            )
            histmgr = getattr(
                self.related.related_model,
                getattr(
                    self.related.related_model._meta,
                    "simple_history_manager_attribute",
                    "_notthere",
                ),
                None,
            )
            if history and histmgr:
                return histmgr.as_of(
                    getattr(history, "_as_of", history.history_date)
                )
        return super().get_queryset(**hints)

@JordanHyatt
Copy link

That's funny, all 3 of those things I noticed and adjusted. See the PR I submitted. Thanks for posting, it definitely gave me a good headstart

@quadracik
Copy link

Great, haven't noticed it. Thanks for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants