From d08b031b3c1f16690393680381efce598dd30d12 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Mon, 13 Jan 2025 16:44:14 -0500 Subject: [PATCH 1/4] Do not update share during version creation --- api/preprints/serializers.py | 7 ++++--- api/subjects/serializers.py | 10 +++++----- api/taxonomies/serializers.py | 6 +++--- osf/models/mixins.py | 8 ++++---- 4 files changed, 16 insertions(+), 15 deletions(-) diff --git a/api/preprints/serializers.py b/api/preprints/serializers.py index d4b7e9bc820..163133026ad 100644 --- a/api/preprints/serializers.py +++ b/api/preprints/serializers.py @@ -296,7 +296,7 @@ def get_preprint_doi_url(self, obj): doi = client.build_doi(preprint=obj) if client else None return f'https://doi.org/{doi}' if doi else None - def update(self, preprint, validated_data): + def update(self, preprint, validated_data, update_search=True): assert isinstance(preprint, Preprint), 'You must specify a valid preprint to be updated' auth = get_user_auth(self.context['request']) @@ -475,7 +475,7 @@ def require_admin_permission(): if 'subjects' in validated_data: subjects = validated_data.pop('subjects', None) - self.update_subjects(preprint, subjects, auth) + self.update_subjects(preprint, subjects, auth, update_search=update_search) save_preprint = True if 'title' in validated_data: @@ -598,7 +598,8 @@ def create(self, validated_data): if not preprint: raise NotFound(detail='Failed to create a new preprint version due to source preprint not found.') if data_to_update: - return self.update(preprint, data_to_update) + # Share should not be updated during version creation + return self.update(preprint, data_to_update, update_search=False) return preprint diff --git a/api/subjects/serializers.py b/api/subjects/serializers.py index 59e2a1c6bfe..e4bc9712c18 100644 --- a/api/subjects/serializers.py +++ b/api/subjects/serializers.py @@ -15,11 +15,11 @@ class UpdateSubjectsMixin: - def update_subjects_method(self, resource, subjects, auth): + def update_subjects_method(self, resource, subjects, auth, update_search=True): # Method to update subjects on resource raise NotImplementedError() - def update_subjects(self, resource, subjects, auth): + def update_subjects(self, resource, subjects, auth, update_search=True): """Updates subjects on resource and handles errors. :param object resource: Object for which you want to update subjects @@ -27,7 +27,7 @@ def update_subjects(self, resource, subjects, auth): :param object Auth object """ try: - self.update_subjects_method(resource, subjects, auth) + self.update_subjects_method(resource, subjects, auth, update_search=update_search) except PermissionsError as e: raise exceptions.PermissionDenied(detail=str(e)) except ValueError as e: @@ -106,9 +106,9 @@ def make_instance_obj(self, obj): def format_subjects(self, subjects): return [subj['_id'] for subj in subjects] - def update_subjects_method(self, resource, subjects, auth): + def update_subjects_method(self, resource, subjects, auth, update_search=True): # Overrides UpdateSubjectsMixin - return resource.set_subjects_from_relationships(subjects, auth) + return resource.set_subjects_from_relationships(subjects, auth, update_search=update_search) def update(self, instance, validated_data): resource = instance['self'] diff --git a/api/taxonomies/serializers.py b/api/taxonomies/serializers.py index 8fb64d76115..04044dfe1e6 100644 --- a/api/taxonomies/serializers.py +++ b/api/taxonomies/serializers.py @@ -98,7 +98,7 @@ def get_subjects(self, obj): ] # Overrides UpdateSubjectsMixin - def update_subjects_method(self, resource, subjects, auth): + def update_subjects_method(self, resource, subjects, auth, update_search=True): """Depending on the request's version, runs a different method to update the resource's subjects. Will expect request to be formatted differently, depending on the version. @@ -108,8 +108,8 @@ def update_subjects_method(self, resource, subjects, auth): :param object Auth object """ if self.expect_subjects_as_relationships(self.context['request']): - return resource.set_subjects_from_relationships(subjects, auth) - return resource.set_subjects(subjects, auth) + return resource.set_subjects_from_relationships(subjects, auth, update_search=update_search) + return resource.set_subjects(subjects, auth, update_search=update_search) def expect_subjects_as_relationships(self, request): """Determines whether subjects should be serialized as a relationship. diff --git a/osf/models/mixins.py b/osf/models/mixins.py index 2584b05cdd3..75a3a7fa529 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -1104,7 +1104,7 @@ def assert_subject_format(self, subj_list, expect_list, error_msg): if (expect_list and not is_list) or (not expect_list and is_list): raise ValidationValueError(f'Subjects are improperly formatted. {error_msg}') - def set_subjects(self, new_subjects, auth, add_log=True): + def set_subjects(self, new_subjects, auth, add_log=True, update_search=True): """ Helper for setting M2M subjects field from list of hierarchies received from UI. Only authorized admins may set subjects. @@ -1134,10 +1134,10 @@ def set_subjects(self, new_subjects, auth, add_log=True): self.add_subjects_log(old_subjects, auth) self.save() - if hasattr(self, 'update_search'): + if update_search and hasattr(self, 'update_search'): self.update_search() - def set_subjects_from_relationships(self, subjects_list, auth, add_log=True): + def set_subjects_from_relationships(self, subjects_list, auth, add_log=True, update_search=True): """ Helper for setting M2M subjects field from list of flattened subjects received from UI. Only authorized admins may set subjects. @@ -1161,7 +1161,7 @@ def set_subjects_from_relationships(self, subjects_list, auth, add_log=True): self.add_subjects_log(old_subjects, auth) self.save() - if hasattr(self, 'update_search'): + if update_search and hasattr(self, 'update_search'): self.update_search() def map_subjects_between_providers(self, old_provider, new_provider, auth=None): From 17cac234f7d2eeb3f4cb3f9da918d50ea51b9afa Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Mon, 13 Jan 2025 21:56:20 -0500 Subject: [PATCH 2/4] Skip SHARE only but still update legacy OSF search --- api/preprints/serializers.py | 6 +++--- api/subjects/serializers.py | 10 +++++----- api/taxonomies/serializers.py | 6 +++--- osf/models/mixins.py | 12 ++++++------ osf/models/preprint.py | 8 ++++++-- 5 files changed, 23 insertions(+), 19 deletions(-) diff --git a/api/preprints/serializers.py b/api/preprints/serializers.py index 163133026ad..5199fb78848 100644 --- a/api/preprints/serializers.py +++ b/api/preprints/serializers.py @@ -296,7 +296,7 @@ def get_preprint_doi_url(self, obj): doi = client.build_doi(preprint=obj) if client else None return f'https://doi.org/{doi}' if doi else None - def update(self, preprint, validated_data, update_search=True): + def update(self, preprint, validated_data, skip_share=False): assert isinstance(preprint, Preprint), 'You must specify a valid preprint to be updated' auth = get_user_auth(self.context['request']) @@ -475,7 +475,7 @@ def require_admin_permission(): if 'subjects' in validated_data: subjects = validated_data.pop('subjects', None) - self.update_subjects(preprint, subjects, auth, update_search=update_search) + self.update_subjects(preprint, subjects, auth, skip_share=skip_share) save_preprint = True if 'title' in validated_data: @@ -599,7 +599,7 @@ def create(self, validated_data): raise NotFound(detail='Failed to create a new preprint version due to source preprint not found.') if data_to_update: # Share should not be updated during version creation - return self.update(preprint, data_to_update, update_search=False) + return self.update(preprint, data_to_update, skip_share=True) return preprint diff --git a/api/subjects/serializers.py b/api/subjects/serializers.py index e4bc9712c18..5dff36bc38e 100644 --- a/api/subjects/serializers.py +++ b/api/subjects/serializers.py @@ -15,11 +15,11 @@ class UpdateSubjectsMixin: - def update_subjects_method(self, resource, subjects, auth, update_search=True): + def update_subjects_method(self, resource, subjects, auth, skip_share=False): # Method to update subjects on resource raise NotImplementedError() - def update_subjects(self, resource, subjects, auth, update_search=True): + def update_subjects(self, resource, subjects, auth, skip_share=False): """Updates subjects on resource and handles errors. :param object resource: Object for which you want to update subjects @@ -27,7 +27,7 @@ def update_subjects(self, resource, subjects, auth, update_search=True): :param object Auth object """ try: - self.update_subjects_method(resource, subjects, auth, update_search=update_search) + self.update_subjects_method(resource, subjects, auth, skip_share=skip_share) except PermissionsError as e: raise exceptions.PermissionDenied(detail=str(e)) except ValueError as e: @@ -106,9 +106,9 @@ def make_instance_obj(self, obj): def format_subjects(self, subjects): return [subj['_id'] for subj in subjects] - def update_subjects_method(self, resource, subjects, auth, update_search=True): + def update_subjects_method(self, resource, subjects, auth, skip_share=False): # Overrides UpdateSubjectsMixin - return resource.set_subjects_from_relationships(subjects, auth, update_search=update_search) + return resource.set_subjects_from_relationships(subjects, auth, skip_share=skip_share) def update(self, instance, validated_data): resource = instance['self'] diff --git a/api/taxonomies/serializers.py b/api/taxonomies/serializers.py index 04044dfe1e6..ab23fee10e8 100644 --- a/api/taxonomies/serializers.py +++ b/api/taxonomies/serializers.py @@ -98,7 +98,7 @@ def get_subjects(self, obj): ] # Overrides UpdateSubjectsMixin - def update_subjects_method(self, resource, subjects, auth, update_search=True): + def update_subjects_method(self, resource, subjects, auth, skip_share=False): """Depending on the request's version, runs a different method to update the resource's subjects. Will expect request to be formatted differently, depending on the version. @@ -108,8 +108,8 @@ def update_subjects_method(self, resource, subjects, auth, update_search=True): :param object Auth object """ if self.expect_subjects_as_relationships(self.context['request']): - return resource.set_subjects_from_relationships(subjects, auth, update_search=update_search) - return resource.set_subjects(subjects, auth, update_search=update_search) + return resource.set_subjects_from_relationships(subjects, auth, skip_share=skip_share) + return resource.set_subjects(subjects, auth, skip_share=skip_share) def expect_subjects_as_relationships(self, request): """Determines whether subjects should be serialized as a relationship. diff --git a/osf/models/mixins.py b/osf/models/mixins.py index 75a3a7fa529..8a339defd67 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -1104,7 +1104,7 @@ def assert_subject_format(self, subj_list, expect_list, error_msg): if (expect_list and not is_list) or (not expect_list and is_list): raise ValidationValueError(f'Subjects are improperly formatted. {error_msg}') - def set_subjects(self, new_subjects, auth, add_log=True, update_search=True): + def set_subjects(self, new_subjects, auth, add_log=True, skip_share=False): """ Helper for setting M2M subjects field from list of hierarchies received from UI. Only authorized admins may set subjects. @@ -1134,10 +1134,10 @@ def set_subjects(self, new_subjects, auth, add_log=True, update_search=True): self.add_subjects_log(old_subjects, auth) self.save() - if update_search and hasattr(self, 'update_search'): - self.update_search() + if hasattr(self, 'update_search'): + self.update_search(skip_share=skip_share) - def set_subjects_from_relationships(self, subjects_list, auth, add_log=True, update_search=True): + def set_subjects_from_relationships(self, subjects_list, auth, add_log=True, skip_share=False): """ Helper for setting M2M subjects field from list of flattened subjects received from UI. Only authorized admins may set subjects. @@ -1161,8 +1161,8 @@ def set_subjects_from_relationships(self, subjects_list, auth, add_log=True, upd self.add_subjects_log(old_subjects, auth) self.save() - if update_search and hasattr(self, 'update_search'): - self.update_search() + if hasattr(self, 'update_search'): + self.update_search(skip_share=skip_share) def map_subjects_between_providers(self, old_provider, new_provider, auth=None): """ diff --git a/osf/models/preprint.py b/osf/models/preprint.py index 8505d864f90..c1109c00c04 100644 --- a/osf/models/preprint.py +++ b/osf/models/preprint.py @@ -1141,8 +1141,12 @@ def bulk_update_search(cls, preprints, index=None): logger.exception(e) log_exception(e) - def update_search(self): - update_share(self) + def update_search(self, skip_share=False): + """Update SHARE and OSF search. + """ + if not skip_share: + update_share(self) + from website import search try: search.search.update_preprint(self, bulk=False, async_update=True) From 1838633686d0884a0e33d216ed272af038059504 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Tue, 14 Jan 2025 13:57:43 -0500 Subject: [PATCH 3/4] Fix update_search for subclasses of VersionedGuidMixin --- osf/models/base.py | 4 ++++ osf/models/mixins.py | 10 ++++++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/osf/models/base.py b/osf/models/base.py index 36d34a4a038..1d01ede4032 100644 --- a/osf/models/base.py +++ b/osf/models/base.py @@ -602,6 +602,10 @@ def get_semantic_iri(self): raise ValueError(f'no osfid for {self} (cannot build semantic iri)') return osfid_iri(_osfid) + def update_search(self, skip_share=False): + """Subclasses must implement `update_search()` with kwarg `skip_share=False`.""" + raise NotImplementedError() + @receiver(post_save) def ensure_guid(sender, instance, **kwargs): """Generate guid if it doesn't exist for subclasses of GuidMixin except for subclasses of VersionedGuidMixin diff --git a/osf/models/mixins.py b/osf/models/mixins.py index 8a339defd67..1d6a846b8be 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -1135,7 +1135,10 @@ def set_subjects(self, new_subjects, auth, add_log=True, skip_share=False): self.save() if hasattr(self, 'update_search'): - self.update_search(skip_share=skip_share) + from osf.models.base import VersionedGuidMixin + if isinstance(self, VersionedGuidMixin): + self.update_search(skip_share=skip_share) + self.update_search() def set_subjects_from_relationships(self, subjects_list, auth, add_log=True, skip_share=False): """ Helper for setting M2M subjects field from list of flattened subjects received from UI. @@ -1162,7 +1165,10 @@ def set_subjects_from_relationships(self, subjects_list, auth, add_log=True, ski self.save() if hasattr(self, 'update_search'): - self.update_search(skip_share=skip_share) + from osf.models.base import VersionedGuidMixin + if isinstance(self, VersionedGuidMixin): + self.update_search(skip_share=skip_share) + self.update_search() def map_subjects_between_providers(self, old_provider, new_provider, auth=None): """ From b80855b8ca7137540ad52889144c9e3e6b102b89 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Tue, 14 Jan 2025 15:30:22 -0500 Subject: [PATCH 4/4] Fix set_subjects --- api/subjects/serializers.py | 1 + osf/models/mixins.py | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/api/subjects/serializers.py b/api/subjects/serializers.py index 5dff36bc38e..085659dc3cf 100644 --- a/api/subjects/serializers.py +++ b/api/subjects/serializers.py @@ -15,6 +15,7 @@ class UpdateSubjectsMixin: + def update_subjects_method(self, resource, subjects, auth, skip_share=False): # Method to update subjects on resource raise NotImplementedError() diff --git a/osf/models/mixins.py b/osf/models/mixins.py index 1d6a846b8be..aa6c43fa525 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -1138,7 +1138,8 @@ def set_subjects(self, new_subjects, auth, add_log=True, skip_share=False): from osf.models.base import VersionedGuidMixin if isinstance(self, VersionedGuidMixin): self.update_search(skip_share=skip_share) - self.update_search() + else: + self.update_search() def set_subjects_from_relationships(self, subjects_list, auth, add_log=True, skip_share=False): """ Helper for setting M2M subjects field from list of flattened subjects received from UI. @@ -1168,7 +1169,8 @@ def set_subjects_from_relationships(self, subjects_list, auth, add_log=True, ski from osf.models.base import VersionedGuidMixin if isinstance(self, VersionedGuidMixin): self.update_search(skip_share=skip_share) - self.update_search() + else: + self.update_search() def map_subjects_between_providers(self, old_provider, new_provider, auth=None): """