From 74be25a32afc093f735f6d309490790a87c06e5a Mon Sep 17 00:00:00 2001 From: fsimonjetz Date: Wed, 24 Jan 2024 17:08:57 +0000 Subject: [PATCH 1/5] add test for invalid field updates --- ebl/tests/fragmentarium/test_fragment_repository.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/ebl/tests/fragmentarium/test_fragment_repository.py b/ebl/tests/fragmentarium/test_fragment_repository.py index 2286cfb2f..a76fce598 100644 --- a/ebl/tests/fragmentarium/test_fragment_repository.py +++ b/ebl/tests/fragmentarium/test_fragment_repository.py @@ -446,12 +446,19 @@ def test_update_script(fragment_repository: FragmentRepository): assert result == updated_fragment -def test_update_update_lemmatization_not_found(fragment_repository): +def test_update_lemmatization_not_found(fragment_repository): transliterated_fragment = TransliteratedFragmentFactory.build() with pytest.raises(NotFoundError): fragment_repository.update_field("lemmatization", transliterated_fragment) +def test_update_invalid_field_fails(fragment_repository): + with pytest.raises(ValueError, match="Unexpected update field"): + fragment_repository.update_field( + "some invalid field name", FragmentFactory.build() + ) + + def test_statistics(database, fragment_repository): database[COLLECTION].insert_many( [ From 44cc9e9753554f4861158913ff0997e9a3247c76 Mon Sep 17 00:00:00 2001 From: fsimonjetz Date: Wed, 24 Jan 2024 17:09:30 +0000 Subject: [PATCH 2/5] add UpdatableField type --- .../application/fragment_repository.py | 16 ++++++++++++++-- .../infrastructure/mongo_fragment_repository.py | 12 ++++++++---- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/ebl/fragmentarium/application/fragment_repository.py b/ebl/fragmentarium/application/fragment_repository.py index 2aa66840a..65746305d 100644 --- a/ebl/fragmentarium/application/fragment_repository.py +++ b/ebl/fragmentarium/application/fragment_repository.py @@ -1,5 +1,5 @@ from abc import ABC, abstractmethod -from typing import List, Sequence, Optional +from typing import List, Sequence, Optional, Literal from ebl.common.domain.scopes import Scope from ebl.common.query.query_result import QueryResult, AfORegisterToFragmentQueryResult @@ -10,6 +10,18 @@ from ebl.transliteration.domain.museum_number import MuseumNumber from ebl.fragmentarium.domain.date import Date +UpdatableField = Literal[ + "introduction", + "text", + "genres", + "references", + "script", + "notes", + "archaeology", + "date", + "dates_in_text", +] + class FragmentRepository(ABC): @abstractmethod @@ -90,7 +102,7 @@ def query_next_and_previous_fragment( ... @abstractmethod - def update_field(self, field: str, fragment: Fragment) -> None: + def update_field(self, field: UpdatableField, fragment: Fragment) -> None: ... @abstractmethod diff --git a/ebl/fragmentarium/infrastructure/mongo_fragment_repository.py b/ebl/fragmentarium/infrastructure/mongo_fragment_repository.py index 8e111269b..cd7a0560c 100644 --- a/ebl/fragmentarium/infrastructure/mongo_fragment_repository.py +++ b/ebl/fragmentarium/infrastructure/mongo_fragment_repository.py @@ -1,4 +1,4 @@ -from typing import List, Optional, Sequence, Iterator +from typing import List, Optional, Sequence, Iterator, get_args import pymongo from marshmallow import EXCLUDE @@ -13,7 +13,10 @@ ) from ebl.errors import NotFoundError from ebl.fragmentarium.application.fragment_info_schema import FragmentInfoSchema -from ebl.fragmentarium.application.fragment_repository import FragmentRepository +from ebl.fragmentarium.application.fragment_repository import ( + FragmentRepository, + UpdatableField, +) from ebl.fragmentarium.application.fragment_schema import FragmentSchema, ScriptSchema from ebl.fragmentarium.application.joins_schema import JoinSchema from ebl.fragmentarium.application.line_to_vec import LineToVecEntry @@ -274,7 +277,7 @@ def query_by_transliterated_not_revised_by_other( ) return FragmentInfoSchema(many=True).load(cursor) - def update_field(self, field, fragment): + def update_field(self, field: UpdatableField, fragment: Fragment): fields_to_update = { "introduction": ("introduction",), "lemmatization": ("text",), @@ -294,8 +297,9 @@ def update_field(self, field, fragment): } if field not in fields_to_update: + valid_fields = ",".join(get_args(UpdatableField)) raise ValueError( - f"Unexpected update field {field}, must be one of {','.join(fields_to_update)}" + f"Unexpected update field {field}, must be one of {valid_fields}" ) query = FragmentSchema(only=fields_to_update[field]).dump(fragment) self._fragments.update_one( From 2b6b49ab6162fe8a67ad9f41cb1a6a902f68e8a3 Mon Sep 17 00:00:00 2001 From: fsimonjetz Date: Wed, 24 Jan 2024 17:12:48 +0000 Subject: [PATCH 3/5] refactor lemmatization update key --- ebl/fragmentarium/application/fragment_updater.py | 2 +- ebl/fragmentarium/infrastructure/mongo_fragment_repository.py | 2 +- ebl/tests/fragmentarium/test_fragment_repository.py | 4 ++-- ebl/tests/fragmentarium/test_fragment_updater.py | 4 +--- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/ebl/fragmentarium/application/fragment_updater.py b/ebl/fragmentarium/application/fragment_updater.py index 888d179d9..65c498f71 100644 --- a/ebl/fragmentarium/application/fragment_updater.py +++ b/ebl/fragmentarium/application/fragment_updater.py @@ -123,7 +123,7 @@ def update_lemmatization( updated_fragment = fragment.update_lemmatization(lemmatization) self._create_changelog(user, fragment, updated_fragment) - self._repository.update_field("lemmatization", updated_fragment) + self._repository.update_field("text", updated_fragment) return self._create_result(updated_fragment) diff --git a/ebl/fragmentarium/infrastructure/mongo_fragment_repository.py b/ebl/fragmentarium/infrastructure/mongo_fragment_repository.py index cd7a0560c..865557bc8 100644 --- a/ebl/fragmentarium/infrastructure/mongo_fragment_repository.py +++ b/ebl/fragmentarium/infrastructure/mongo_fragment_repository.py @@ -280,7 +280,7 @@ def query_by_transliterated_not_revised_by_other( def update_field(self, field: UpdatableField, fragment: Fragment): fields_to_update = { "introduction": ("introduction",), - "lemmatization": ("text",), + "text": ("text",), "genres": ("genres",), "references": ("references",), "script": ("script",), diff --git a/ebl/tests/fragmentarium/test_fragment_repository.py b/ebl/tests/fragmentarium/test_fragment_repository.py index a76fce598..eb455ac53 100644 --- a/ebl/tests/fragmentarium/test_fragment_repository.py +++ b/ebl/tests/fragmentarium/test_fragment_repository.py @@ -410,7 +410,7 @@ def test_update_lemmatization(fragment_repository): lemmatization = Lemmatization(tokens) updated_fragment = transliterated_fragment.update_lemmatization(lemmatization) - fragment_repository.update_field("lemmatization", updated_fragment) + fragment_repository.update_field("text", updated_fragment) result = fragment_repository.query_by_museum_number(transliterated_fragment.number) assert result == updated_fragment @@ -449,7 +449,7 @@ def test_update_script(fragment_repository: FragmentRepository): def test_update_lemmatization_not_found(fragment_repository): transliterated_fragment = TransliteratedFragmentFactory.build() with pytest.raises(NotFoundError): - fragment_repository.update_field("lemmatization", transliterated_fragment) + fragment_repository.update_field("text", transliterated_fragment) def test_update_invalid_field_fails(fragment_repository): diff --git a/ebl/tests/fragmentarium/test_fragment_updater.py b/ebl/tests/fragmentarium/test_fragment_updater.py index 2069e365d..0e6d012b9 100644 --- a/ebl/tests/fragmentarium/test_fragment_updater.py +++ b/ebl/tests/fragmentarium/test_fragment_updater.py @@ -208,9 +208,7 @@ def test_update_lemmatization( {"_id": str(number), **SCHEMA.dump(transliterated_fragment)}, {"_id": str(number), **SCHEMA.dump(lemmatized_fragment)}, ).thenReturn() - when(fragment_repository).update_field( - "lemmatization", lemmatized_fragment - ).thenReturn() + when(fragment_repository).update_field("text", lemmatized_fragment).thenReturn() result = fragment_updater.update_lemmatization(number, lemmatization, user) assert result == (injected_fragment, False) From 8a1a5215aa899dc1559e6e7e2154eab4c83fa161 Mon Sep 17 00:00:00 2001 From: fsimonjetz Date: Wed, 24 Jan 2024 17:34:43 +0000 Subject: [PATCH 4/5] extract transliteration update into dedicated method --- .../application/fragment_repository.py | 4 ++++ .../application/fragment_updater.py | 2 +- .../mongo_fragment_repository.py | 23 +++++++++++++------ .../fragmentarium/test_fragment_repository.py | 4 ++-- .../fragmentarium/test_fragment_updater.py | 2 +- 5 files changed, 24 insertions(+), 11 deletions(-) diff --git a/ebl/fragmentarium/application/fragment_repository.py b/ebl/fragmentarium/application/fragment_repository.py index 65746305d..8ad8fcf8e 100644 --- a/ebl/fragmentarium/application/fragment_repository.py +++ b/ebl/fragmentarium/application/fragment_repository.py @@ -101,6 +101,10 @@ def query_next_and_previous_fragment( ) -> FragmentPagerInfo: ... + @abstractmethod + def update_transliteration(self, fragment: Fragment) -> None: + ... + @abstractmethod def update_field(self, field: UpdatableField, fragment: Fragment) -> None: ... diff --git a/ebl/fragmentarium/application/fragment_updater.py b/ebl/fragmentarium/application/fragment_updater.py index 65c498f71..6b0b464f7 100644 --- a/ebl/fragmentarium/application/fragment_updater.py +++ b/ebl/fragmentarium/application/fragment_updater.py @@ -48,7 +48,7 @@ def update_transliteration( else fragment.update_lowest_join_transliteration(transliteration, user) ) self._create_changelog(user, fragment, updated_fragment) - self._repository.update_field("transliteration", updated_fragment) + self._repository.update_transliteration(updated_fragment) return self._create_result(updated_fragment) diff --git a/ebl/fragmentarium/infrastructure/mongo_fragment_repository.py b/ebl/fragmentarium/infrastructure/mongo_fragment_repository.py index 865557bc8..1eb60568c 100644 --- a/ebl/fragmentarium/infrastructure/mongo_fragment_repository.py +++ b/ebl/fragmentarium/infrastructure/mongo_fragment_repository.py @@ -277,6 +277,21 @@ def query_by_transliterated_not_revised_by_other( ) return FragmentInfoSchema(many=True).load(cursor) + def update_transliteration(self, fragment: Fragment): + self._fragments.update_one( + fragment_is(fragment), + { + "$set": FragmentSchema( + only=( + "text", + "signs", + "record", + "line_to_vec", + ) + ).dump(fragment) + }, + ) + def update_field(self, field: UpdatableField, fragment: Fragment): fields_to_update = { "introduction": ("introduction",), @@ -286,12 +301,6 @@ def update_field(self, field: UpdatableField, fragment: Fragment): "script": ("script",), "notes": ("notes",), "archaeology": ("archaeology",), - "transliteration": ( - "text", - "signs", - "record", - "line_to_vec", - ), "date": ("date",), "dates_in_text": ("dates_in_text",), } @@ -304,7 +313,7 @@ def update_field(self, field: UpdatableField, fragment: Fragment): query = FragmentSchema(only=fields_to_update[field]).dump(fragment) self._fragments.update_one( fragment_is(fragment), - {"$set": query if query else {field: None}}, + {"$set": query or {field: None}}, ) def query_next_and_previous_folio(self, folio_name, folio_number, number): diff --git a/ebl/tests/fragmentarium/test_fragment_repository.py b/ebl/tests/fragmentarium/test_fragment_repository.py index eb455ac53..463c02f10 100644 --- a/ebl/tests/fragmentarium/test_fragment_repository.py +++ b/ebl/tests/fragmentarium/test_fragment_repository.py @@ -358,7 +358,7 @@ def test_update_transliteration_with_record(fragment_repository, user): TransliterationUpdate(parse_atf_lark("$ (the transliteration)")), user ) - fragment_repository.update_field("transliteration", updated_fragment) + fragment_repository.update_transliteration(updated_fragment) result = fragment_repository.query_by_museum_number(fragment.number) assert result == updated_fragment @@ -367,7 +367,7 @@ def test_update_transliteration_with_record(fragment_repository, user): def test_update_update_transliteration_not_found(fragment_repository): transliterated_fragment = TransliteratedFragmentFactory.build() with pytest.raises(NotFoundError): - fragment_repository.update_field("transliteration", transliterated_fragment) + fragment_repository.update_transliteration(transliterated_fragment) def test_update_genres(fragment_repository): diff --git a/ebl/tests/fragmentarium/test_fragment_updater.py b/ebl/tests/fragmentarium/test_fragment_updater.py index 0e6d012b9..52f7769c1 100644 --- a/ebl/tests/fragmentarium/test_fragment_updater.py +++ b/ebl/tests/fragmentarium/test_fragment_updater.py @@ -64,7 +64,7 @@ def test_update_transliteration( ).thenReturn() ( when(fragment_repository) - .update_field("transliteration", transliterated_fragment) + .update_transliteration(transliterated_fragment) .thenReturn() ) From 4c78a2bb13426bcf1bcd52ed194648c7af72df22 Mon Sep 17 00:00:00 2001 From: fsimonjetz Date: Wed, 24 Jan 2024 17:42:15 +0000 Subject: [PATCH 5/5] validate with UpdatableField type --- .../infrastructure/mongo_fragment_repository.py | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/ebl/fragmentarium/infrastructure/mongo_fragment_repository.py b/ebl/fragmentarium/infrastructure/mongo_fragment_repository.py index 1eb60568c..b2176db90 100644 --- a/ebl/fragmentarium/infrastructure/mongo_fragment_repository.py +++ b/ebl/fragmentarium/infrastructure/mongo_fragment_repository.py @@ -293,24 +293,11 @@ def update_transliteration(self, fragment: Fragment): ) def update_field(self, field: UpdatableField, fragment: Fragment): - fields_to_update = { - "introduction": ("introduction",), - "text": ("text",), - "genres": ("genres",), - "references": ("references",), - "script": ("script",), - "notes": ("notes",), - "archaeology": ("archaeology",), - "date": ("date",), - "dates_in_text": ("dates_in_text",), - } - - if field not in fields_to_update: - valid_fields = ",".join(get_args(UpdatableField)) + if field not in (valid_fields := get_args(UpdatableField)): raise ValueError( f"Unexpected update field {field}, must be one of {valid_fields}" ) - query = FragmentSchema(only=fields_to_update[field]).dump(fragment) + query = FragmentSchema(only=(field,)).dump(fragment) self._fragments.update_one( fragment_is(fragment), {"$set": query or {field: None}},