From 3eccd05ecec2d5798dbef96e27add1c0ff53b48a Mon Sep 17 00:00:00 2001 From: EspenAlbert Date: Tue, 24 Nov 2020 08:40:19 +0000 Subject: [PATCH 1/6] support updating models on the many side of the one-to-many relations. --- ormar/models/modelproxy.py | 6 ++++++ tests/test_foreign_keys.py | 13 +++++++++++++ 2 files changed, 19 insertions(+) diff --git a/ormar/models/modelproxy.py b/ormar/models/modelproxy.py index 3835b41..4d5304b 100644 --- a/ormar/models/modelproxy.py +++ b/ormar/models/modelproxy.py @@ -70,6 +70,12 @@ class ModelTableProxy: f"model without pk set!" ) model_dict[field] = pk_value + elif isinstance(field_value, list): + targets = [target.get(target_pkname) for target in field_value] + if targets: + model_dict[field] = targets + else: + model_dict.pop(field) elif field_value: # nested dict model_dict[field] = field_value.get(target_pkname) else: diff --git a/tests/test_foreign_keys.py b/tests/test_foreign_keys.py index e7bf4e5..0559489 100644 --- a/tests/test_foreign_keys.py +++ b/tests/test_foreign_keys.py @@ -376,3 +376,16 @@ async def test_wrong_model_passed_as_fk(): with pytest.raises(RelationshipInstanceError): org = await Organisation.objects.create(ident="ACME Ltd") await Track.objects.create(album=org, title="Test1", position=1) + + +@pytest.mark.asyncio +async def test_bulk_update_model_with_children(): + async with database: + async with database.transaction(force_rollback=True): + album = await Album.objects.create(name="Test") + track = await Track.objects.create(album=album, title="Test1", position=1) + album.name = "Test2" + await Album.objects.bulk_update([album], columns=["name"]) + + updated_album = await Album.objects.get(id=album.id) + assert updated_album.name == "Test2" From a668b20f1a5c8d9c17b9c71868c32be51dd90de8 Mon Sep 17 00:00:00 2001 From: EspenAlbert Date: Tue, 24 Nov 2020 08:56:57 +0000 Subject: [PATCH 2/6] Add test for an Album with tracks. --- tests/test_foreign_keys.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/tests/test_foreign_keys.py b/tests/test_foreign_keys.py index 0559489..ae814d3 100644 --- a/tests/test_foreign_keys.py +++ b/tests/test_foreign_keys.py @@ -379,7 +379,7 @@ async def test_wrong_model_passed_as_fk(): @pytest.mark.asyncio -async def test_bulk_update_model_with_children(): +async def test_bulk_update_model_with_no_children(): async with database: async with database.transaction(force_rollback=True): album = await Album.objects.create(name="Test") @@ -389,3 +389,19 @@ async def test_bulk_update_model_with_children(): updated_album = await Album.objects.get(id=album.id) assert updated_album.name == "Test2" + + +@pytest.mark.asyncio +async def test_bulk_update_model_with_children(): + async with database: + async with database.transaction(force_rollback=True): + album = await Album.objects.create(name="Test") + track = await Track.objects.create(album=album, title="Test1", position=1) + album_with_tracks = await Album.objects.select_related("tracks").get( + name="Test" + ) + album_with_tracks.name = "Test2" + await Album.objects.bulk_update([album_with_tracks], columns=["name"]) + + updated_album = await Album.objects.get(id=album.id) + assert updated_album.name == "Test2" From cf3ec2e2a55058b47b154f43b3bf414299129ad1 Mon Sep 17 00:00:00 2001 From: EspenAlbert Date: Tue, 24 Nov 2020 09:09:54 +0000 Subject: [PATCH 3/6] Refactor tests to cover all lines. --- tests/test_foreign_keys.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/test_foreign_keys.py b/tests/test_foreign_keys.py index ae814d3..09e3e82 100644 --- a/tests/test_foreign_keys.py +++ b/tests/test_foreign_keys.py @@ -383,7 +383,6 @@ async def test_bulk_update_model_with_no_children(): async with database: async with database.transaction(force_rollback=True): album = await Album.objects.create(name="Test") - track = await Track.objects.create(album=album, title="Test1", position=1) album.name = "Test2" await Album.objects.bulk_update([album], columns=["name"]) @@ -396,12 +395,13 @@ async def test_bulk_update_model_with_children(): async with database: async with database.transaction(force_rollback=True): album = await Album.objects.create(name="Test") - track = await Track.objects.create(album=album, title="Test1", position=1) - album_with_tracks = await Album.objects.select_related("tracks").get( - name="Test" - ) - album_with_tracks.name = "Test2" - await Album.objects.bulk_update([album_with_tracks], columns=["name"]) + track = await Track.objects.create(title="Test1", position=1) + album.tracks = [track] + album.name = "Test2" + await Album.objects.bulk_update([album], columns=["name"]) - updated_album = await Album.objects.get(id=album.id) + updated_album = await Album.objects.select_related("tracks").get( + id=album.id + ) assert updated_album.name == "Test2" + assert len(updated_album.tracks) == 0 From 0679bb4158c3b41e7cdbe0a7ab68e24df3bb6e63 Mon Sep 17 00:00:00 2001 From: EspenAlbert Date: Tue, 24 Nov 2020 09:31:59 +0000 Subject: [PATCH 4/6] Remove else clause that was never hit. --- ormar/models/modelproxy.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ormar/models/modelproxy.py b/ormar/models/modelproxy.py index 4d5304b..af98d74 100644 --- a/ormar/models/modelproxy.py +++ b/ormar/models/modelproxy.py @@ -76,10 +76,8 @@ class ModelTableProxy: model_dict[field] = targets else: model_dict.pop(field) - elif field_value: # nested dict + else: # nested dict model_dict[field] = field_value.get(target_pkname) - else: - model_dict.pop(field, None) return model_dict @classmethod @@ -237,7 +235,9 @@ class ModelTableProxy: @staticmethod def _populate_pk_column( - model: Type["Model"], columns: List[str], use_alias: bool = False, + model: Type["Model"], + columns: List[str], + use_alias: bool = False, ) -> List[str]: pk_alias = ( model.get_column_alias(model.Meta.pkname) From 6f9b32b57590467b67ab3984d74e8b1ff7c132a3 Mon Sep 17 00:00:00 2001 From: EspenAlbert Date: Tue, 24 Nov 2020 18:52:51 +0000 Subject: [PATCH 5/6] refactor to a more sensible test. reduce cyclomatic complexity of if clause --- ormar/models/modelproxy.py | 15 +++++++------- tests/test_foreign_keys.py | 42 +++++++++++++++++++++++++++++--------- 2 files changed, 40 insertions(+), 17 deletions(-) diff --git a/ormar/models/modelproxy.py b/ormar/models/modelproxy.py index af98d74..2ea7ac3 100644 --- a/ormar/models/modelproxy.py +++ b/ormar/models/modelproxy.py @@ -70,14 +70,15 @@ class ModelTableProxy: f"model without pk set!" ) model_dict[field] = pk_value - elif isinstance(field_value, list): - targets = [target.get(target_pkname) for target in field_value] - if targets: - model_dict[field] = targets + elif field_value: # nested dict + if isinstance(field_value, list): + model_dict[field] = [ + target.get(target_pkname) for target in field_value + ] else: - model_dict.pop(field) - else: # nested dict - model_dict[field] = field_value.get(target_pkname) + model_dict[field] = field_value.get(target_pkname) + else: + model_dict.pop(field) return model_dict @classmethod diff --git a/tests/test_foreign_keys.py b/tests/test_foreign_keys.py index 09e3e82..4dafef7 100644 --- a/tests/test_foreign_keys.py +++ b/tests/test_foreign_keys.py @@ -20,6 +20,7 @@ class Album(ormar.Model): id: int = ormar.Integer(primary_key=True) name: str = ormar.String(max_length=100) + is_best_seller: bool = ormar.Boolean(default=False) class Track(ormar.Model): @@ -32,6 +33,7 @@ class Track(ormar.Model): album: Optional[Album] = ormar.ForeignKey(Album) title: str = ormar.String(max_length=100) position: int = ormar.Integer() + play_count: int = ormar.Integer(nullable=True, default=0) class Cover(ormar.Model): @@ -394,14 +396,34 @@ async def test_bulk_update_model_with_no_children(): async def test_bulk_update_model_with_children(): async with database: async with database.transaction(force_rollback=True): - album = await Album.objects.create(name="Test") - track = await Track.objects.create(title="Test1", position=1) - album.tracks = [track] - album.name = "Test2" - await Album.objects.bulk_update([album], columns=["name"]) - - updated_album = await Album.objects.select_related("tracks").get( - id=album.id + best_seller = await Album.objects.create(name="to_be_best_seller") + best_seller2 = await Album.objects.create(name="to_be_best_seller2") + not_best_seller = await Album.objects.create(name="unpopular") + await Track.objects.create( + album=best_seller, title="t1", position=1, play_count=100 ) - assert updated_album.name == "Test2" - assert len(updated_album.tracks) == 0 + await Track.objects.create( + album=best_seller2, title="t2", position=1, play_count=100 + ) + await Track.objects.create( + album=not_best_seller, title="t3", position=1, play_count=3 + ) + await Track.objects.create( + album=best_seller, title="t4", position=1, play_count=500 + ) + + tracks = await Track.objects.select_related("album").filter( + play_count__gt=10 + ).all() + best_seller_albums = {} + for track in tracks: + album = track.album + if album.id in best_seller_albums: + continue + album.is_best_seller = True + best_seller_albums[album.id] = album + await Album.objects.bulk_update( + best_seller_albums.values(), columns=["is_best_seller"] + ) + best_seller_albums_db = await Album.objects.filter(is_best_seller=True).all() + assert len(best_seller_albums_db) == 2 From 1dc05a0fe3c1ec87d4a87cc9662fbaefb538bf7f Mon Sep 17 00:00:00 2001 From: EspenAlbert Date: Wed, 25 Nov 2020 08:49:04 +0000 Subject: [PATCH 6/6] apply suggestion on adding `model_dict.pop(field, None)`` since the model might be missing the field and that would cause an error. --- ormar/models/modelproxy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ormar/models/modelproxy.py b/ormar/models/modelproxy.py index 2ea7ac3..1ae257e 100644 --- a/ormar/models/modelproxy.py +++ b/ormar/models/modelproxy.py @@ -78,7 +78,7 @@ class ModelTableProxy: else: model_dict[field] = field_value.get(target_pkname) else: - model_dict.pop(field) + model_dict.pop(field, None) return model_dict @classmethod