From c11c83cb7a289ce86f8efb6298e3c6afe61cbbd0 Mon Sep 17 00:00:00 2001 From: Nick Jackson Date: Wed, 2 Oct 2024 17:35:59 +0100 Subject: [PATCH] refactor(Document): remove unused overwrite method Remove `Document.overwrite` and `MarklogicApiClient.overwrite` methods, and associated move utility methods. These are never used downstream, and contained a call to an undefined method which would have raised a runtime error. BREAKING CHANGE: Remove `Document.overwrite` and `MarkLogicApiClient.overwrite` --- CHANGELOG.md | 3 +- src/caselawclient/Client.py | 7 +- .../models/documents/__init__.py | 3 - src/caselawclient/models/utilities/move.py | 72 +++---------------- tests/models/utilities/test_utilities.py | 44 +----------- 5 files changed, 14 insertions(+), 115 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7dca043d..fe188e16 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format is based on [Keep a Changelog 1.0.0]. ### BREAKING CHANGE +- Remove `Document.overwrite` and `MarkLogicApiClient.overwrite` - The `models.documents.body.CourtIdentifierString` type has been replaced with the more specific `courts.CourtCode` type from ds-caselaw-utils. ### Fix @@ -17,10 +18,10 @@ The format is based on [Keep a Changelog 1.0.0]. - **deps**: update dependency ds-caselaw-utils to v1.7.0 - **deps**: update dependency boto3 to v1.35.28 - **deps**: update dependency ds-caselaw-utils to v1.5.7 -- **deps**: update dependency boto3 to v1.35.25 ### Refactor +- **Document**: remove unused overwrite method - **DocumentBody**: replace CourtIdentifierString with utils.courts.CourtCode ## v26.0.0 (2024-09-25) diff --git a/src/caselawclient/Client.py b/src/caselawclient/Client.py index b2d31a87..4e504c96 100644 --- a/src/caselawclient/Client.py +++ b/src/caselawclient/Client.py @@ -1035,12 +1035,7 @@ def search_judgments_and_decode_response( search_parameters.collections = [DOCUMENT_COLLECTION_URI_JUDGMENT] return self.search_and_decode_response(search_parameters) - def overwrite_document(self, old_uri: str, new_citation: str) -> str: - """Move the judgment at old_uri on top of the new citation, which must already exist - Compare to update_document_uri""" - return move.overwrite_document(old_uri, new_citation, api_client=self) - - def update_document_uri(self, old_uri: str, new_citation: str) -> str: + def update_document_uri(self, old_uri: DocumentURIString, new_citation: str) -> DocumentURIString: """ Move the document at old_uri to the correct location based on the neutral citation The new neutral citation *must* not already exist (that is handled elsewhere) diff --git a/src/caselawclient/models/documents/__init__.py b/src/caselawclient/models/documents/__init__.py index 077663d0..378d9c31 100644 --- a/src/caselawclient/models/documents/__init__.py +++ b/src/caselawclient/models/documents/__init__.py @@ -441,9 +441,6 @@ def delete(self) -> None: else: raise DocumentNotSafeForDeletion - def overwrite(self, new_citation: str) -> None: - self.api_client.overwrite_document(self.uri, new_citation) - def move(self, new_citation: str) -> None: self.api_client.update_document_uri(self.uri, new_citation) diff --git a/src/caselawclient/models/utilities/move.py b/src/caselawclient/models/utilities/move.py index 4e18b0b1..e50e192f 100644 --- a/src/caselawclient/models/utilities/move.py +++ b/src/caselawclient/models/utilities/move.py @@ -1,16 +1,16 @@ -from typing import Any, Optional +from typing import TYPE_CHECKING, Any, Optional import ds_caselaw_utils as caselawutils from caselawclient.errors import MarklogicAPIError +from caselawclient.models.documents import DocumentURIString from caselawclient.models.utilities.aws import copy_assets - -class NeutralCitationToUriError(Exception): - pass +if TYPE_CHECKING: + from caselawclient.Client import MarklogicApiClient -class OverwriteJudgmentError(Exception): +class NeutralCitationToUriError(Exception): pass @@ -18,62 +18,9 @@ class MoveJudgmentError(Exception): pass -def overwrite_document( - source_uri: str, - target_citation: str, - api_client: Any, -) -> str: - """Move the document at source_uri on top of the new citation, which must already exist - Compare to update_document_uri - - :param source_uri: The URI with the contents of the document to be written. (possibly a failure url) - :param target_citation: The NCN (implying a URL) whose contents will be overwritten - :param api_client: An instance of MarklogicApiClient used to make the search request - :return: The URL associated with the `target_citation` - """ - - new_uri: Optional[str] = caselawutils.neutral_url(target_citation.strip()) - - if new_uri == source_uri: - raise RuntimeError( - f"Attempted to overwrite document {source_uri} with itself, which is not permitted.", - ) - if new_uri is None: - raise NeutralCitationToUriError( - f"Unable to form new URI for {source_uri} from neutral citation: {target_citation}", - ) - if not api_client.document_exists(new_uri): - raise OverwriteJudgmentError( - f"The URI {new_uri} generated from {target_citation} does not already exist, so cannot be overwritten", - ) - old_doc = api_client.get_document_by_uri_or_404(source_uri) - try: - old_doc_xml = old_doc.content_as_xml - api_client.update_document_xml( - new_uri, - old_doc_xml, - annotation=f"overwritten from {source_uri}", - ) - set_metadata(source_uri, new_uri, api_client) - # TODO: consider deleting existing public assets at that location - copy_assets(source_uri, new_uri) - api_client.set_judgment_this_uri(new_uri) - except MarklogicAPIError as e: - raise OverwriteJudgmentError( - f"Failure when attempting to copy judgment from {source_uri} to {new_uri}: {e}", - ) - - try: - api_client.delete_judgment(source_uri) - except MarklogicAPIError as e: - raise OverwriteJudgmentError( - f"Failure when attempting to delete judgment from {source_uri}: {e}", - ) - - return new_uri - - -def update_document_uri(source_uri: str, target_citation: str, api_client: Any) -> str: +def update_document_uri( + source_uri: DocumentURIString, target_citation: str, api_client: "MarklogicApiClient" +) -> DocumentURIString: """ Move the document at source_uri to the correct location based on the neutral citation The new neutral citation *must* not already exist (that is handled elsewhere) @@ -83,7 +30,8 @@ def update_document_uri(source_uri: str, target_citation: str, api_client: Any) :param api_client: An instance of MarklogicApiClient used to make the search request :return: The URL associated with the `target_citation` """ - new_uri: Optional[str] = caselawutils.neutral_url(target_citation.strip()) + new_ncn_based_uri = caselawutils.neutral_url(target_citation.strip()) + new_uri: Optional[DocumentURIString] = DocumentURIString(new_ncn_based_uri) if new_ncn_based_uri else None if new_uri is None: raise NeutralCitationToUriError( f"Unable to form new URI for {source_uri} from neutral citation: {target_citation}", diff --git a/tests/models/utilities/test_utilities.py b/tests/models/utilities/test_utilities.py index 3d8f08f1..3981c0ac 100644 --- a/tests/models/utilities/test_utilities.py +++ b/tests/models/utilities/test_utilities.py @@ -1,10 +1,9 @@ import io import os -from unittest.mock import ANY, MagicMock, Mock, patch +from unittest.mock import MagicMock, Mock, patch import boto3 import ds_caselaw_utils -import pytest from moto import mock_aws from caselawclient.models.utilities import extract_version, move, render_versions @@ -14,8 +13,6 @@ copy_assets, ) -from ...factories import JudgmentFactory - class TestVersionUtils: def test_extract_version_uri(self): @@ -83,45 +80,6 @@ def test_copy_assets(self, client): ) -class TestOverwrite: - @patch.dict(os.environ, {"PRIVATE_ASSET_BUCKET": "MY_BUCKET"}) - @patch("boto3.session.Session.client") - def test_overwrite_success(self, fake_boto3_client): - """Given the target judgment does not exist, - we continue to move the judgment to the new location - (where moving is copy + delete)""" - ds_caselaw_utils.neutral_url = MagicMock(return_value="new/uri") - fake_api_client = MagicMock() - fake_api_client.judgment_exists.return_value = True - fake_api_client.copy_judgment.return_value = True - fake_api_client.delete_judgment.return_value = True - fake_boto3_client.list_objects.return_value = [] - - fake_judgment = JudgmentFactory.build() - fake_judgment.return_value.content_as_xml = "b" - fake_api_client.get_document_by_uri_or_404.return_value = fake_judgment - - result = move.overwrite_document("old/uri", "[2002] EAT 1", fake_api_client) - fake_api_client.update_document_xml.assert_called_with( - "new/uri", - ANY, - annotation="overwritten from old/uri", - ) - fake_api_client.delete_judgment.assert_called_with("old/uri") - assert result == "new/uri" - - def test_overwrite_unparseable_citation(self): - ds_caselaw_utils.neutral_url = MagicMock(return_value=None) - fake_api_client = MagicMock() - - with pytest.raises(move.NeutralCitationToUriError): - move.overwrite_document( - "old/uri", - "Wrong neutral citation", - fake_api_client, - ) - - class TestMove: @patch.dict(os.environ, {"PRIVATE_ASSET_BUCKET": "MY_BUCKET"}) @patch("boto3.session.Session.client")