From 170461b1c6bb329b980bd8ba25bf57639170f1f4 Mon Sep 17 00:00:00 2001 From: VerdantForge <123564396+VerdantForge@users.noreply.github.com> Date: Thu, 22 Feb 2024 17:34:00 +0100 Subject: [PATCH 1/5] Add ExcludedPrefixes option to VersioningConfig --- minio/versioningconfig.py | 29 +++++++++++++++++++++++++---- tests/unit/versioningconfig_test.py | 25 ++++++++++++++++++++++++- 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/minio/versioningconfig.py b/minio/versioningconfig.py index bfae36104..adead27ce 100644 --- a/minio/versioningconfig.py +++ b/minio/versioningconfig.py @@ -34,16 +34,20 @@ class VersioningConfig: """Versioning configuration.""" def __init__( - self, - status: str | None = None, - mfa_delete: str | None = None, + self, + status: str | None = None, + mfa_delete: str | None = None, + excluded_prefixes: list[str] | None = None, ): if status is not None and status not in [ENABLED, SUSPENDED]: raise ValueError(f"status must be {ENABLED} or {SUSPENDED}") if mfa_delete is not None and mfa_delete not in [ENABLED, DISABLED]: raise ValueError(f"MFA delete must be {ENABLED} or {DISABLED}") + if excluded_prefixes is not None and not isinstance(excluded_prefixes, list): + raise ValueError(f"Excluded prefixes must be a list") self._status = status self._mfa_delete = mfa_delete + self._excluded_prefixes = excluded_prefixes @property def status(self) -> str: @@ -55,12 +59,25 @@ def mfa_delete(self) -> str | None: """Get MFA delete.""" return self._mfa_delete + @property + def excluded_prefixes(self) -> str | None: + """Get MFA delete.""" + return self._excluded_prefixes + @classmethod def fromxml(cls: Type[A], element: ET.Element) -> A: """Create new object with values from XML element.""" status = findtext(element, "Status") mfa_delete = findtext(element, "MFADelete") - return cls(status, mfa_delete) + + excluded_prefixes_tag = element.find("ExcludedPrefixes") + excluded_prefixes = None + if excluded_prefixes_tag: + excluded_prefixes = [ + tag.text for tag in excluded_prefixes_tag.findall("ExcludedPrefix") + ] + + return cls(status, mfa_delete, excluded_prefixes) def toxml(self, element: ET.Element | None) -> ET.Element: """Convert to XML.""" @@ -69,4 +86,8 @@ def toxml(self, element: ET.Element | None) -> ET.Element: SubElement(element, "Status", self._status) if self._mfa_delete: SubElement(element, "MFADelete", self._mfa_delete) + if self._excluded_prefixes: + SubElement(element, "ExcludedPrefixes") + for prefix in self._excluded_prefixes: + SubElement(element, "ExcludedPrefix", prefix) return element diff --git a/tests/unit/versioningconfig_test.py b/tests/unit/versioningconfig_test.py index 8de9751e3..dd6a47b3d 100644 --- a/tests/unit/versioningconfig_test.py +++ b/tests/unit/versioningconfig_test.py @@ -18,7 +18,11 @@ from minio import xml from minio.commonconfig import DISABLED, ENABLED -from minio.versioningconfig import OFF, SUSPENDED, VersioningConfig +from minio.versioningconfig import ( + OFF, + SUSPENDED, + VersioningConfig, +) class VersioningConfigTest(TestCase): @@ -53,3 +57,22 @@ def test_config(self): xml.marshal(config) self.assertEqual(config.status, SUSPENDED) self.assertEqual(config.mfa_delete, DISABLED) + + def test_config_with_excluded_prefixes(self): + + config = xml.unmarshal( + VersioningConfig, + """ + + Enabled + Disabled + + prefix + + + """, + ) + xml.marshal(config) + self.assertEqual(config.status, ENABLED) + self.assertEqual(config.mfa_delete, DISABLED) + self.assertListEqual(config.excluded_prefixes, ["prefix"]) From 96cd85a8966acd8dc3da4f2b65d4b9f8b1693b94 Mon Sep 17 00:00:00 2001 From: Nicholas Greensmith <123564396+VerdantForge@users.noreply.github.com> Date: Fri, 23 Feb 2024 12:44:41 +0100 Subject: [PATCH 2/5] fix empty formatting string in raise ValueError --- minio/versioningconfig.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/minio/versioningconfig.py b/minio/versioningconfig.py index adead27ce..6c7e94b25 100644 --- a/minio/versioningconfig.py +++ b/minio/versioningconfig.py @@ -44,7 +44,7 @@ def __init__( if mfa_delete is not None and mfa_delete not in [ENABLED, DISABLED]: raise ValueError(f"MFA delete must be {ENABLED} or {DISABLED}") if excluded_prefixes is not None and not isinstance(excluded_prefixes, list): - raise ValueError(f"Excluded prefixes must be a list") + raise ValueError("Excluded prefixes must be a list") self._status = status self._mfa_delete = mfa_delete self._excluded_prefixes = excluded_prefixes From 530d98c2678afcf7e93ceffd623147f71936bfe5 Mon Sep 17 00:00:00 2001 From: Bala FA Date: Fri, 23 Feb 2024 17:18:52 +0530 Subject: [PATCH 3/5] Apply suggestions from code review --- minio/versioningconfig.py | 50 ++++++++++++++++++----------- tests/unit/versioningconfig_test.py | 24 -------------- 2 files changed, 31 insertions(+), 43 deletions(-) diff --git a/minio/versioningconfig.py b/minio/versioningconfig.py index 6c7e94b25..5ac1b8714 100644 --- a/minio/versioningconfig.py +++ b/minio/versioningconfig.py @@ -34,10 +34,11 @@ class VersioningConfig: """Versioning configuration.""" def __init__( - self, - status: str | None = None, - mfa_delete: str | None = None, - excluded_prefixes: list[str] | None = None, + self, + status: str | None = None, + mfa_delete: str | None = None, + excluded_prefixes: list[str] | None = None, + exclude_folders: bool = False, ): if status is not None and status not in [ENABLED, SUSPENDED]: raise ValueError(f"status must be {ENABLED} or {SUSPENDED}") @@ -48,6 +49,7 @@ def __init__( self._status = status self._mfa_delete = mfa_delete self._excluded_prefixes = excluded_prefixes + self._exclude_folders = exclude_folders @property def status(self) -> str: @@ -60,24 +62,32 @@ def mfa_delete(self) -> str | None: return self._mfa_delete @property - def excluded_prefixes(self) -> str | None: - """Get MFA delete.""" + def excluded_prefixes(self) -> list[str] | None: + """Get excluded prefixes.""" return self._excluded_prefixes + @property + def exclude_folders(self) -> bool: + """Get exclude folders.""" + return self._exclude_folders + @classmethod def fromxml(cls: Type[A], element: ET.Element) -> A: """Create new object with values from XML element.""" status = findtext(element, "Status") mfa_delete = findtext(element, "MFADelete") - - excluded_prefixes_tag = element.find("ExcludedPrefixes") - excluded_prefixes = None - if excluded_prefixes_tag: - excluded_prefixes = [ - tag.text for tag in excluded_prefixes_tag.findall("ExcludedPrefix") - ] - - return cls(status, mfa_delete, excluded_prefixes) + excluded_prefixes = [ + prefix.text for prefix in findall( + element, "ExcludedPrefixes/Prefix", + ) + ] + exclude_folders = findtext(element, "ExcludeFolders") == "true" + return cls( + status, + mfa_delete, + cast(Union[List[str], None], excluded_prefixes), + exclude_folders, + ) def toxml(self, element: ET.Element | None) -> ET.Element: """Convert to XML.""" @@ -86,8 +96,10 @@ def toxml(self, element: ET.Element | None) -> ET.Element: SubElement(element, "Status", self._status) if self._mfa_delete: SubElement(element, "MFADelete", self._mfa_delete) - if self._excluded_prefixes: - SubElement(element, "ExcludedPrefixes") - for prefix in self._excluded_prefixes: - SubElement(element, "ExcludedPrefix", prefix) + for prefix in self._excluded_prefixes or []: + SubElement( + SubElement(element, "ExcludedPrefixes"), "Prefix", prefix, + ) + if self._exclude_folders: + SubElement(element, "ExcludeFolders", "true") return element diff --git a/tests/unit/versioningconfig_test.py b/tests/unit/versioningconfig_test.py index dd6a47b3d..165ca1b46 100644 --- a/tests/unit/versioningconfig_test.py +++ b/tests/unit/versioningconfig_test.py @@ -18,11 +18,6 @@ from minio import xml from minio.commonconfig import DISABLED, ENABLED -from minio.versioningconfig import ( - OFF, - SUSPENDED, - VersioningConfig, -) class VersioningConfigTest(TestCase): @@ -57,22 +52,3 @@ def test_config(self): xml.marshal(config) self.assertEqual(config.status, SUSPENDED) self.assertEqual(config.mfa_delete, DISABLED) - - def test_config_with_excluded_prefixes(self): - - config = xml.unmarshal( - VersioningConfig, - """ - - Enabled - Disabled - - prefix - - - """, - ) - xml.marshal(config) - self.assertEqual(config.status, ENABLED) - self.assertEqual(config.mfa_delete, DISABLED) - self.assertListEqual(config.excluded_prefixes, ["prefix"]) From 9a2ed27dabb67981893cd68912b3c173dfd973c6 Mon Sep 17 00:00:00 2001 From: VerdantForge <123564396+VerdantForge@users.noreply.github.com> Date: Wed, 28 Feb 2024 09:08:04 +0100 Subject: [PATCH 4/5] Bugfixes following review changes --- minio/versioningconfig.py | 14 +++++++++----- tests/unit/versioningconfig_test.py | 1 + 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/minio/versioningconfig.py b/minio/versioningconfig.py index 5ac1b8714..86ff0d6db 100644 --- a/minio/versioningconfig.py +++ b/minio/versioningconfig.py @@ -18,11 +18,11 @@ from __future__ import absolute_import, annotations -from typing import Type, TypeVar +from typing import List, Type, TypeVar, Union, cast from xml.etree import ElementTree as ET from .commonconfig import DISABLED, ENABLED -from .xml import Element, SubElement, findtext +from .xml import Element, SubElement, findall, findtext OFF = "Off" SUSPENDED = "Suspended" @@ -77,8 +77,10 @@ def fromxml(cls: Type[A], element: ET.Element) -> A: status = findtext(element, "Status") mfa_delete = findtext(element, "MFADelete") excluded_prefixes = [ - prefix.text for prefix in findall( - element, "ExcludedPrefixes/Prefix", + prefix.text + for prefix in findall( + element, + "ExcludedPrefixes/ExcludedPrefix", ) ] exclude_folders = findtext(element, "ExcludeFolders") == "true" @@ -98,7 +100,9 @@ def toxml(self, element: ET.Element | None) -> ET.Element: SubElement(element, "MFADelete", self._mfa_delete) for prefix in self._excluded_prefixes or []: SubElement( - SubElement(element, "ExcludedPrefixes"), "Prefix", prefix, + SubElement(element, "ExcludedPrefixes"), + "ExcludedPrefix", + prefix, ) if self._exclude_folders: SubElement(element, "ExcludeFolders", "true") diff --git a/tests/unit/versioningconfig_test.py b/tests/unit/versioningconfig_test.py index 165ca1b46..8de9751e3 100644 --- a/tests/unit/versioningconfig_test.py +++ b/tests/unit/versioningconfig_test.py @@ -18,6 +18,7 @@ from minio import xml from minio.commonconfig import DISABLED, ENABLED +from minio.versioningconfig import OFF, SUSPENDED, VersioningConfig class VersioningConfigTest(TestCase): From 97067c8946f74842ead43a3701af3efe42ddf9e7 Mon Sep 17 00:00:00 2001 From: Bala FA Date: Wed, 28 Feb 2024 20:13:38 +0530 Subject: [PATCH 5/5] Update minio/versioningconfig.py --- minio/versioningconfig.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/minio/versioningconfig.py b/minio/versioningconfig.py index 86ff0d6db..13613595e 100644 --- a/minio/versioningconfig.py +++ b/minio/versioningconfig.py @@ -44,8 +44,6 @@ def __init__( raise ValueError(f"status must be {ENABLED} or {SUSPENDED}") if mfa_delete is not None and mfa_delete not in [ENABLED, DISABLED]: raise ValueError(f"MFA delete must be {ENABLED} or {DISABLED}") - if excluded_prefixes is not None and not isinstance(excluded_prefixes, list): - raise ValueError("Excluded prefixes must be a list") self._status = status self._mfa_delete = mfa_delete self._excluded_prefixes = excluded_prefixes