From 110af26e1fb7c9835aa70d417c19dd439fdd04fc Mon Sep 17 00:00:00 2001 From: Manuel Jeckelmann Date: Tue, 27 Jan 2015 20:56:33 +0100 Subject: [PATCH 1/4] Added an attempt of changing FK pointer to object ID; M2M rels don't follow yet --- versions/models.py | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/versions/models.py b/versions/models.py index 4f69a4b..c41137c 100644 --- a/versions/models.py +++ b/versions/models.py @@ -479,6 +479,33 @@ def get_extra_restriction(self, where_class, alias, remote_alias): return where_class([VersionedExtraWhere(historic_sql=historic_sql, current_sql=current_sql, alias=alias, remote_alias=remote_alias)]) + def get_joining_columns(self, reverse_join=False): + """ + Get and return joining columns defined by this foreign key relationship + + :return: A tuple containing the column names of the tables to be joined (, ) + :rtype: tuple + """ + source = self.reverse_related_fields if reverse_join else self.related_fields + joining_columns = tuple() + for lhs_field, rhs_field in source: + lhs_col_name = lhs_field.column + rhs_col_name = rhs_field.column + if self is lhs_field: + # self is the current ForeignKey relationship + if rhs_col_name == Versionable.VERSION_IDENTIFIER_FIELD: + rhs_col_name = Versionable.OBJECT_IDENTIFIER_FIELD + elif self is rhs_field: + if lhs_col_name == Versionable.VERSION_IDENTIFIER_FIELD: + lhs_col_name = Versionable.OBJECT_IDENTIFIER_FIELD + joining_columns = joining_columns + ((lhs_col_name, rhs_col_name),) + # joining_columns = super(VersionedForeignKey, self).get_joining_columns(reverse_join) + # TODO: + # if remote col is id and remote col is primary key and remote model is versioned: + # then replace id by identity + return joining_columns + + class VersionedManyToManyField(ManyToManyField): def __init__(self, *args, **kwargs): @@ -917,6 +944,9 @@ class Versionable(models.Model): This is pretty much the central point for versioning objects. """ + VERSION_IDENTIFIER_FIELD = 'id' + OBJECT_IDENTIFIER_FIELD = 'identity' + id = models.CharField(max_length=36, primary_key=True) """id stands for ID and is the primary key; sometimes also referenced as the surrogate key""" From 64734ec8a4c6882efc4273d24460fe4addf50bda Mon Sep 17 00:00:00 2001 From: Manuel Jeckelmann Date: Tue, 27 Jan 2015 21:48:38 +0100 Subject: [PATCH 2/4] Added test case --- versions_tests/tests/test_models.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/versions_tests/tests/test_models.py b/versions_tests/tests/test_models.py index f4175d0..c578b30 100644 --- a/versions_tests/tests/test_models.py +++ b/versions_tests/tests/test_models.py @@ -1696,3 +1696,15 @@ def test_prefetch_related_via_many_to_many(self): old = len(award_players[i].awards.all()) new = len(updated_award_players[i].awards.all()) self.assertTrue(new == old - 1) + + +class FilterOnRelationTest(TestCase): + def test_filter_on_relation(self): + team = Team.objects.create(name='team') + player = Player.objects.create(name='player', team=team) + t1 = get_utc_now() + sleep(0.1) + l1 = len(Player.objects.as_of(t1).filter(team__name='team')) + team.clone() + l2 = len(Player.objects.as_of(t1).filter(team__name='team')) + self.assertEqual(l1, l2) From 8636abed3084ee11ed1daf59dbe72f95b70477a3 Mon Sep 17 00:00:00 2001 From: Manuel Jeckelmann Date: Thu, 29 Jan 2015 15:17:13 +0100 Subject: [PATCH 3/4] Added the 'auto_created' flag to VersionedForeignKeys being part of VM2M-relationships in order to be able to make the difference between the two of them. This enables us to JOIN on the identity field instead of the id field in case of a VersionedForeignKey --- versions/models.py | 16 +++++++--------- versions_tests/tests/test_models.py | 8 ++++---- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/versions/models.py b/versions/models.py index fcf0cc7..9fdea02 100644 --- a/versions/models.py +++ b/versions/models.py @@ -491,18 +491,16 @@ def get_joining_columns(self, reverse_join=False): for lhs_field, rhs_field in source: lhs_col_name = lhs_field.column rhs_col_name = rhs_field.column - if self is lhs_field: - # self is the current ForeignKey relationship + # Test whether + # - self is the current ForeignKey relationship + # - self was not auto_created (e.g. is not part of a M2M relationship) + if self is lhs_field and not self.auto_created: if rhs_col_name == Versionable.VERSION_IDENTIFIER_FIELD: rhs_col_name = Versionable.OBJECT_IDENTIFIER_FIELD - elif self is rhs_field: + elif self is rhs_field and not self.auto_created: if lhs_col_name == Versionable.VERSION_IDENTIFIER_FIELD: lhs_col_name = Versionable.OBJECT_IDENTIFIER_FIELD joining_columns = joining_columns + ((lhs_col_name, rhs_col_name),) - # joining_columns = super(VersionedForeignKey, self).get_joining_columns(reverse_join) - # TODO: - # if remote col is id and remote col is primary key and remote model is versioned: - # then replace id by identity return joining_columns @@ -584,8 +582,8 @@ def create_versioned_many_to_many_intermediary_model(field, cls, field_name): return type(str(name), (Versionable,), { 'Meta': meta, '__module__': cls.__module__, - from_: VersionedForeignKey(cls, related_name='%s+' % name), - to_field_name: VersionedForeignKey(to, related_name='%s+' % name), + from_: VersionedForeignKey(cls, related_name='%s+' % name, auto_created=name), + to_field_name: VersionedForeignKey(to, related_name='%s+' % name, auto_created=name), }) diff --git a/versions_tests/tests/test_models.py b/versions_tests/tests/test_models.py index fd4a45a..877fed0 100644 --- a/versions_tests/tests/test_models.py +++ b/versions_tests/tests/test_models.py @@ -793,7 +793,7 @@ def test_query_created_by_filtering_for_deleted_player_at_t5(self): FROM "{team_table}" INNER JOIN "{player_table}" ON ( - "{team_table}"."id" = "{player_table}"."team_id" + "{team_table}"."identity" = "{player_table}"."team_id" AND (( {player_table}.version_start_date <= {ts} AND ( @@ -1588,7 +1588,7 @@ def test_select_related_query_sqlite(self): "{team_table}"."name", "{team_table}"."city_id" FROM "{player_table}" - LEFT OUTER JOIN "{team_table}" ON ("{player_table}"."team_id" = "{team_table}"."id" + LEFT OUTER JOIN "{team_table}" ON ("{player_table}"."team_id" = "{team_table}"."identity" AND (({team_table}.version_start_date <= {ts} AND ({team_table}.version_end_date > {ts} OR {team_table}.version_end_date IS NULL)))) @@ -1733,8 +1733,8 @@ def test_prefetch_related_via_many_to_many(self): self.assertTrue(new == old - 1) -class FilterOnRelationTest(TestCase): - def test_filter_on_relation(self): +class FilterOnForeignKeyRelationTest(TestCase): + def test_filter_on_fk_relation(self): team = Team.objects.create(name='team') player = Player.objects.create(name='player', team=team) t1 = get_utc_now() From 30b86d2ba16919dc9322199d7dbe439f4868e8df Mon Sep 17 00:00:00 2001 From: Manuel Jeckelmann Date: Thu, 29 Jan 2015 16:24:06 +0100 Subject: [PATCH 4/4] Added a condition such that querytime filters get added only a single time; extended unittest documentation --- versions/models.py | 6 ++++-- versions_tests/tests/test_models.py | 29 +++++++++++++++++++++++++++-- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/versions/models.py b/versions/models.py index 9fdea02..a40fc2c 100644 --- a/versions/models.py +++ b/versions/models.py @@ -309,7 +309,7 @@ def get_compiler(self, *args, **kwargs): (e.g. by adding a filter to the queryset) does not allow the caching of related object to work (they are attached to a queryset; filter() returns a new queryset). """ - if self.querytime.active: + if self.querytime.active and (not hasattr(self, '_querytime_filter_added') or not self._querytime_filter_added): time = self.querytime.time if time is None: self.add_q(Q(version_end_date__isnull=True)) @@ -318,6 +318,9 @@ def get_compiler(self, *args, **kwargs): (Q(version_end_date__gt=time) | Q(version_end_date__isnull=True)) & Q(version_start_date__lte=time) ) + # Ensure applying these filters happens only a single time (even if it doesn't falsify the query, it's + # just not very comfortable to read) + self._querytime_filter_added = True return super(VersionedQuery, self).get_compiler(*args, **kwargs) @@ -504,7 +507,6 @@ def get_joining_columns(self, reverse_join=False): return joining_columns - class VersionedManyToManyField(ManyToManyField): def __init__(self, *args, **kwargs): super(VersionedManyToManyField, self).__init__(*args, **kwargs) diff --git a/versions_tests/tests/test_models.py b/versions_tests/tests/test_models.py index 877fed0..628bc4a 100644 --- a/versions_tests/tests/test_models.py +++ b/versions_tests/tests/test_models.py @@ -678,6 +678,10 @@ def setUp(self): self.t1 = get_utc_now() sleep(0.1) + # State at t1 + # Players: [p1.v1, p2.v1] + # Teams: [t.v1] + # t.player_set = [p1, p2] team.player_set.remove(p2) @@ -687,6 +691,10 @@ def setUp(self): self.t2 = get_utc_now() sleep(0.1) + # State at t2 + # Players: [p1.v1, p2.v1, p2.v2] + # Teams: [t.v1] + # t.player_set = [p1] team.player_set.remove(p1) @@ -696,6 +704,10 @@ def setUp(self): self.t3 = get_utc_now() sleep(0.1) + # State at t3 + # Players: [p1.v1, p2.v1, p2.v2, p1.v2] + # Teams: [t.v1] + # t.player_set = [] # Let's get those players back into the game! team.player_set.add(p1) @@ -711,10 +723,18 @@ def setUp(self): self.t4 = get_utc_now() sleep(0.1) + # State at t4 + # Players: [p1.v1, p2.v1, p2.v2, p1.v2, p2.v3, p1.v3] + # Teams: [t.v1] + # t.player_set = [p1, p2] p1.delete() self.t5 = get_utc_now() + # State at t4 + # Players: [p1.v1, p2.v1, p2.v2, p1.v2, p2.v3, p1.v3] + # Teams: [t.v1] + # t.player_set = [p2] def test_filtering_on_the_other_side_of_the_relation(self): self.assertEqual(1, Team.objects.all().count()) @@ -774,6 +794,8 @@ def test_filtering_for_deleted_player_at_t5(self): @skipUnless(connection.vendor == 'sqlite', 'SQL is database specific, only sqlite is tested here.') def test_query_created_by_filtering_for_deleted_player_at_t5(self): team_none_queryset = Team.objects.as_of(self.t5).filter(player__name__startswith='p1') + # Validating the current query prior to analyzing the generated SQL + self.assertEqual([], list(team_none_queryset)) team_none_query = str(team_none_queryset.query) team_table = Team._meta.db_table @@ -1566,7 +1588,10 @@ def test_select_related(self): @skipUnless(connection.vendor == 'sqlite', 'SQL is database specific, only sqlite is tested here.') def test_select_related_query_sqlite(self): - select_related_query = str(Player.objects.as_of(self.t1).select_related('team').all().query) + select_related_queryset = Player.objects.as_of(self.t1).select_related('team').all() + # Validating the query before verifying the SQL string + self.assertEqual(['pl1.v1', 'pl2.v1'], [player.name for player in select_related_queryset]) + select_related_query = str(select_related_queryset.query) team_table = Team._meta.db_table player_table = Player._meta.db_table @@ -1625,7 +1650,7 @@ def test_select_related_query_postgresql(self): "{team_table}"."name", "{team_table}"."city_id" FROM "{player_table}" - LEFT OUTER JOIN "{team_table}" ON ("{player_table}"."team_id" = "{team_table}"."id" + LEFT OUTER JOIN "{team_table}" ON ("{player_table}"."team_id" = "{team_table}"."identity" AND (({team_table}.version_start_date <= {ts} AND ({team_table}.version_end_date > {ts} OR {team_table}.version_end_date IS NULL))))