From bb64e9c669fd1f812a4f6602d78dbeec082238f2 Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Thu, 25 May 2023 15:39:20 -0700 Subject: [PATCH 01/16] Abstract SamConfig and decouple TOML logic --- samcli/commands/deploy/guided_config.py | 11 +- samcli/lib/config/exceptions.py | 4 + samcli/lib/config/samconfig.py | 139 ++++++++++++------ tests/unit/cli/test_cli_config_file.py | 63 +++++++- .../unit/commands/samconfig/test_samconfig.py | 20 ++- tests/unit/lib/samconfig/test_samconfig.py | 14 +- 6 files changed, 194 insertions(+), 57 deletions(-) diff --git a/samcli/commands/deploy/guided_config.py b/samcli/commands/deploy/guided_config.py index b9d8ea59b5..10b80189db 100644 --- a/samcli/commands/deploy/guided_config.py +++ b/samcli/commands/deploy/guided_config.py @@ -7,6 +7,7 @@ from samcli.cli.context import get_cmd_names from samcli.commands.deploy.exceptions import GuidedDeployFailedError +from samcli.lib.config.exceptions import SamConfigFileReadException from samcli.lib.config.samconfig import DEFAULT_CONFIG_FILE_NAME, DEFAULT_ENV, SamConfig @@ -26,13 +27,17 @@ def get_config_ctx(self, config_file=None): return ctx, samconfig def read_config_showcase(self, config_file=None): - _, samconfig = self.get_config_ctx(config_file) - - status = "Found" if samconfig.exists() else "Not found" msg = ( "Syntax invalid in samconfig.toml; save values " "through sam deploy --guided to overwrite file with a valid set of values." ) + try: + _, samconfig = self.get_config_ctx(config_file) + except SamConfigFileReadException: + raise GuidedDeployFailedError(msg) + + status = "Found" if samconfig.exists() else "Not found" + config_sanity = samconfig.sanity_check() click.secho("\nConfiguring SAM deploy\n======================", fg="yellow") click.echo(f"\n\tLooking for config file [{config_file}] : {status}") diff --git a/samcli/lib/config/exceptions.py b/samcli/lib/config/exceptions.py index 50297ce722..6bf993de9b 100644 --- a/samcli/lib/config/exceptions.py +++ b/samcli/lib/config/exceptions.py @@ -5,3 +5,7 @@ class SamConfigVersionException(Exception): pass + + +class SamConfigFileReadException(Exception): + pass diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index e48e53d625..3703babfd1 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -4,12 +4,13 @@ import logging import os +from abc import ABC, abstractmethod from pathlib import Path from typing import Any, Iterable import tomlkit -from samcli.lib.config.exceptions import SamConfigVersionException +from samcli.lib.config.exceptions import SamConfigFileReadException, SamConfigVersionException from samcli.lib.config.version import SAM_CONFIG_VERSION, VERSION_KEY LOG = logging.getLogger(__name__) @@ -20,12 +21,56 @@ DEFAULT_GLOBAL_CMDNAME = "global" +class FileManager(ABC): + """ + Abstract class to be overridden by file managers for specific file extensions. + """ + + @staticmethod + @abstractmethod + def read(filepath: Path) -> dict: + pass + + @staticmethod + @abstractmethod + def write(document: dict, filepath: Path): + pass + + +class TomlFileManager(FileManager): + """ + Static class to read and write toml files. + """ + + @staticmethod + def read(filepath: Path) -> dict: + document: dict = {} + try: + txt = filepath.read_text() + document = dict(tomlkit.loads(txt)) + except OSError: + document = {} + except tomlkit.exceptions.TOMLKitError as e: + raise SamConfigFileReadException(e) + + return document + + @staticmethod + def write(document: dict, filepath: Path): + if not document: + return + + filepath.write_text(tomlkit.dumps(document)) + + class SamConfig: """ - Class to interface with `samconfig.toml` file. + Class to represent `samconfig` config options. """ - document = None + FILE_MANAGER_MAPPER: dict = { + "toml": TomlFileManager, + } def __init__(self, config_dir, filename=None): """ @@ -39,11 +84,15 @@ def __init__(self, config_dir, filename=None): Optional. Name of the configuration file. It is recommended to stick with default so in the future we could automatically support auto-resolving multiple config files within same directory. """ + self.document = {} self.filepath = Path(config_dir, filename or DEFAULT_CONFIG_FILE_NAME) + self.file_manager = self.FILE_MANAGER_MAPPER.get(self.filepath.suffix[1:], TomlFileManager) # default to TOML + if not self.file_manager: + LOG.warning(f"The config file extension '{self.filepath.suffix[1:]}' is not supported. TOML will be used.") + self._read() def get_stage_configuration_names(self): - self._read() - if isinstance(self.document, dict): + if self.document: return [stage for stage, value in self.document.items() if isinstance(value, dict)] return [] @@ -70,22 +119,21 @@ def get_all(self, cmd_names, section, env=DEFAULT_ENV): KeyError If the config file does *not* have the specific section - tomlkit.exceptions.TOMLKitError + SamConfigFileReadException If the configuration file is invalid """ env = env or DEFAULT_ENV - self._read() - if isinstance(self.document, dict): - toml_content = self.document.get(env, {}) - params = toml_content.get(self._to_key(cmd_names), {}).get(section, {}) - if DEFAULT_GLOBAL_CMDNAME in toml_content: - global_params = toml_content.get(DEFAULT_GLOBAL_CMDNAME, {}).get(section, {}) - global_params.update(params.copy()) - params = global_params.copy() - return params - return {} + self.document = self.file_manager.read(self.filepath) + + toml_content = self.document.get(env, {}) + params = toml_content.get(self._to_key(cmd_names), {}).get(section, {}) + if DEFAULT_GLOBAL_CMDNAME in toml_content: + global_params = toml_content.get(DEFAULT_GLOBAL_CMDNAME, {}).get(section, {}) + global_params.update(params.copy()) + params = global_params.copy() + return params def put(self, cmd_names, section, key, value, env=DEFAULT_ENV): """ @@ -108,14 +156,9 @@ def put(self, cmd_names, section, key, value, env=DEFAULT_ENV): Raises ------ - tomlkit.exceptions.TOMLKitError + SamConfigFileReadException If the data is invalid """ - - if self.document is None: - # Checking for None here since a TOMLDocument can include a - # 'body' property but still be falsy without a 'value' property - self._read() # Empty document prepare the initial structure. # self.document is a nested dict, we need to check each layer and add new tables, otherwise duplicated key # in parent layer will override the whole child layer @@ -144,10 +187,8 @@ def put_comment(self, comment): comment: str A comment to write to the samconfg file """ - if self.document is None: - self._read() - self.document.add(tomlkit.comment(comment)) + self.document.update({"__comment__": comment}) def flush(self): """ @@ -155,11 +196,12 @@ def flush(self): Raises ------ - tomlkit.exceptions.TOMLKitError + SamConfigFileReadException If the data is invalid """ self._write() + print(self.document) def sanity_check(self): """ @@ -167,7 +209,7 @@ def sanity_check(self): """ try: self._read() - except tomlkit.exceptions.TOMLKitError: + except SamConfigFileReadException: return False else: return True @@ -194,31 +236,46 @@ def config_dir(template_file_path=None): return os.getcwd() def _read(self): + # if not self.document: + # try: + # txt = self.filepath.read_text() + # self.document = tomlkit.loads(txt) + # self._version_sanity_check(self._version()) + # except OSError: + # self.document = tomlkit.document() + + # if self.document.body: + # self._version_sanity_check(self._version()) + # return self.document + if not self.document: - try: - txt = self.filepath.read_text() - self.document = tomlkit.loads(txt) - self._version_sanity_check(self._version()) - except OSError: - self.document = tomlkit.document() - - if self.document.body: + self.document = self.file_manager.read(self.filepath) + if self.document: self._version_sanity_check(self._version()) return self.document def _write(self): + # if not self.document: + # return + + # self._ensure_exists() + + # current_version = self._version() if self._version() else SAM_CONFIG_VERSION + # try: + # self.document.add(VERSION_KEY, current_version) + # except tomlkit.exceptions.KeyAlreadyPresent: + # # NOTE(TheSriram): Do not attempt to re-write an existing version + # pass + # self.filepath.write_text(tomlkit.dumps(self.document)) if not self.document: return self._ensure_exists() current_version = self._version() if self._version() else SAM_CONFIG_VERSION - try: - self.document.add(VERSION_KEY, current_version) - except tomlkit.exceptions.KeyAlreadyPresent: - # NOTE(TheSriram): Do not attempt to re-write an existing version - pass - self.filepath.write_text(tomlkit.dumps(self.document)) + self.document.update({VERSION_KEY: current_version}) + + self.file_manager.write(self.document, self.filepath) def _version(self): return self.document.get(VERSION_KEY, None) diff --git a/tests/unit/cli/test_cli_config_file.py b/tests/unit/cli/test_cli_config_file.py index 606e0a004e..8177b3b9da 100644 --- a/tests/unit/cli/test_cli_config_file.py +++ b/tests/unit/cli/test_cli_config_file.py @@ -5,9 +5,12 @@ from unittest import TestCase, skipIf from unittest.mock import MagicMock, patch +import tomlkit + from samcli.commands.exceptions import ConfigException from samcli.cli.cli_config_file import TomlProvider, configuration_option, configuration_callback, get_ctx_defaults -from samcli.lib.config.samconfig import DEFAULT_ENV +from samcli.lib.config.exceptions import SamConfigFileReadException, SamConfigVersionException +from samcli.lib.config.samconfig import DEFAULT_ENV, TomlFileManager from tests.testing_utils import IS_WINDOWS @@ -40,14 +43,14 @@ def test_toml_valid_with_no_version(self): config_dir = tempfile.gettempdir() config_path = Path(config_dir, "samconfig.toml") config_path.write_text("[config_env.topic.parameters]\nword='clarity'\n") - with self.assertRaises(ConfigException): + with self.assertRaises(SamConfigVersionException): TomlProvider(section=self.parameters)(config_path, self.config_env, [self.cmd_name]) def test_toml_valid_with_invalid_version(self): config_dir = tempfile.gettempdir() config_path = Path(config_dir, "samconfig.toml") config_path.write_text("version='abc'\n[config_env.topic.parameters]\nword='clarity'\n") - with self.assertRaises(ConfigException): + with self.assertRaises(SamConfigVersionException): TomlProvider(section=self.parameters)(config_path, self.config_env, [self.cmd_name]) def test_toml_invalid_empty_dict(self): @@ -63,7 +66,7 @@ def test_toml_invalid_file_name(self): config_path.write_text("version=0.1\n[config_env.topic.parameters]\nword='clarity'\n") config_path_invalid = Path(config_dir, "samconfig.toml") - with self.assertRaises(ConfigException): + with self.assertRaises(SamConfigFileReadException): self.toml_provider(config_path_invalid, self.config_env, [self.cmd_name]) def test_toml_invalid_syntax(self): @@ -71,7 +74,7 @@ def test_toml_invalid_syntax(self): config_path = Path(config_dir, "samconfig.toml") config_path.write_text("version=0.1\n[config_env.topic.parameters]\nword=_clarity'\n") - with self.assertRaises(ConfigException): + with self.assertRaises(SamConfigFileReadException): self.toml_provider(config_path, self.config_env, [self.cmd_name]) @@ -231,3 +234,53 @@ def test_get_ctx_defaults_nested(self): get_ctx_defaults("intent-answer", provider, mock_context4, "default") provider.assert_called_with(None, "default", ["local", "generate-event", "alexa-skills-kit", "intent-answer"]) + + +class TestTomlFileManager(TestCase): + def test_read_toml(self): + config_dir = tempfile.gettempdir() + config_path = Path(config_dir, "samconfig.toml") + config_path.write_text("version=0.1\n[config_env.topic1.parameters]\nword='clarity'\n") + self.assertEqual( + TomlFileManager.read(config_path), + {"version": 0.1, "config_env": {"topic1": {"parameters": {"word": "clarity"}}}}, + ) + + def test_read_toml_invalid_toml(self): + config_dir = tempfile.gettempdir() + config_path = Path(config_dir, "samconfig.toml") + config_path.write_text("fake='not real'\nimproper toml file\n") + with self.assertRaises(SamConfigFileReadException): + TomlFileManager.read(config_path) + + def test_read_toml_file_path_not_valid(self): + config_dir = "path/that/doesnt/exist" + config_path = Path(config_dir, "samconfig.toml") + self.assertEqual(TomlFileManager.read(config_path), {}) + + def test_write_toml(self): + config_dir = tempfile.gettempdir() + config_path = Path(config_dir, "samconfig.toml") + toml = {"version": 0.1, "config_env": {"topic2": {"parameters": {"word": "clarity"}}}} + + TomlFileManager.write(toml, config_path) + + txt = config_path.read_text() + self.assertIn("version = 0.1", txt) + self.assertIn("[config_env.topic2.parameters]", txt) + self.assertIn('word = "clarity"', txt) + + def test_dont_write_toml_if_empty(self): + config_dir = tempfile.gettempdir() + config_path = Path(config_dir, "samconfig.toml") + config_path.write_text("nothing to see here\n") + toml = {} + + TomlFileManager.write(toml, config_path) + + self.assertEqual(config_path.read_text(), "nothing to see here\n") + + def test_write_toml_bad_path(self): + config_path = Path("path/to/some", "file_that_doesnt_exist.toml") + with self.assertRaises(FileNotFoundError): + TomlFileManager.write({"some key": "some value"}, config_path) diff --git a/tests/unit/commands/samconfig/test_samconfig.py b/tests/unit/commands/samconfig/test_samconfig.py index 1ff1217138..6f69ae87eb 100644 --- a/tests/unit/commands/samconfig/test_samconfig.py +++ b/tests/unit/commands/samconfig/test_samconfig.py @@ -8,7 +8,6 @@ import tempfile from pathlib import Path from contextlib import contextmanager -from samcli.lib.config.samconfig import SamConfig, DEFAULT_ENV from click.testing import CliRunner @@ -16,6 +15,7 @@ from unittest.mock import patch, ANY import logging +from samcli.lib.config.samconfig import SamConfig, DEFAULT_ENV, TomlFileManager from samcli.lib.utils.packagetype import ZIP, IMAGE LOG = logging.getLogger() @@ -1245,6 +1245,24 @@ def test_secondary_option_name_template_validate(self, do_cli_mock): do_cli_mock.assert_called_with(ANY, str(Path(os.getcwd(), "mytemplate.yaml")), False) +class TestSamConfigFileManager(TestCase): + def test_file_manager_defaults_to_toml(self): + config_dir = tempfile.gettempdir() + config_path = Path(config_dir, "samconfig") + + samconfig = SamConfig(config_path, filename="samconfig") + + self.assertIs(samconfig.file_manager, TomlFileManager) + + def test_file_manager_toml(self): + config_dir = tempfile.gettempdir() + config_path = Path(config_dir, "samconfig.toml") + + samconfig = SamConfig(config_path, filename="samconfig.toml") + + self.assertIs(samconfig.file_manager, TomlFileManager) + + @contextmanager def samconfig_parameters(cmd_names, config_dir=None, env=None, **kwargs): """ diff --git a/tests/unit/lib/samconfig/test_samconfig.py b/tests/unit/lib/samconfig/test_samconfig.py index 7a86e6f97d..b3a3a7e858 100644 --- a/tests/unit/lib/samconfig/test_samconfig.py +++ b/tests/unit/lib/samconfig/test_samconfig.py @@ -182,27 +182,27 @@ def test_check_sanity(self): def test_check_version_non_supported_type(self): self._setup_config() - self.samconfig.document.remove(VERSION_KEY) - self.samconfig.document.add(VERSION_KEY, "aadeff") + self.samconfig.document.pop(VERSION_KEY) + self.samconfig.document.update({VERSION_KEY: "aadeff"}) with self.assertRaises(SamConfigVersionException): self.samconfig.sanity_check() def test_check_version_no_version_exists(self): self._setup_config() - self.samconfig.document.remove(VERSION_KEY) + self.samconfig.document.pop(VERSION_KEY) with self.assertRaises(SamConfigVersionException): self.samconfig.sanity_check() def test_check_version_float(self): self._setup_config() - self.samconfig.document.remove(VERSION_KEY) - self.samconfig.document.add(VERSION_KEY, 0.2) + self.samconfig.document.pop(VERSION_KEY) + self.samconfig.document.update({VERSION_KEY: 0.2}) self.samconfig.sanity_check() def test_write_config_file_non_standard_version(self): self._setup_config() - self.samconfig.document.remove(VERSION_KEY) - self.samconfig.document.add(VERSION_KEY, 0.2) + self.samconfig.document.pop(VERSION_KEY) + self.samconfig.document.update({VERSION_KEY: 0.2}) self.samconfig.put(cmd_names=["local", "start", "api"], section="parameters", key="skip_pull_image", value=True) self.samconfig.sanity_check() self.assertEqual(self.samconfig.document.get(VERSION_KEY), 0.2) From 16ba0ed6ddbba3d73061776415e22e35f49a9bd7 Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Thu, 25 May 2023 15:48:55 -0700 Subject: [PATCH 02/16] Fix documentation and comments --- samcli/lib/config/samconfig.py | 34 +++++----------------------------- 1 file changed, 5 insertions(+), 29 deletions(-) diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index 3703babfd1..441754e014 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -127,10 +127,10 @@ def get_all(self, cmd_names, section, env=DEFAULT_ENV): self.document = self.file_manager.read(self.filepath) - toml_content = self.document.get(env, {}) - params = toml_content.get(self._to_key(cmd_names), {}).get(section, {}) - if DEFAULT_GLOBAL_CMDNAME in toml_content: - global_params = toml_content.get(DEFAULT_GLOBAL_CMDNAME, {}).get(section, {}) + config_content = self.document.get(env, {}) + params = config_content.get(self._to_key(cmd_names), {}).get(section, {}) + if DEFAULT_GLOBAL_CMDNAME in config_content: + global_params = config_content.get(DEFAULT_GLOBAL_CMDNAME, {}).get(section, {}) global_params.update(params.copy()) params = global_params.copy() return params @@ -150,7 +150,7 @@ def put(self, cmd_names, section, key, value, env=DEFAULT_ENV): key : str Key to write the data under value : Any - Value to write. Could be any of the supported TOML types. + Value to write. Could be any of the supported types. env : str Optional, Name of the environment @@ -236,18 +236,6 @@ def config_dir(template_file_path=None): return os.getcwd() def _read(self): - # if not self.document: - # try: - # txt = self.filepath.read_text() - # self.document = tomlkit.loads(txt) - # self._version_sanity_check(self._version()) - # except OSError: - # self.document = tomlkit.document() - - # if self.document.body: - # self._version_sanity_check(self._version()) - # return self.document - if not self.document: self.document = self.file_manager.read(self.filepath) if self.document: @@ -255,18 +243,6 @@ def _read(self): return self.document def _write(self): - # if not self.document: - # return - - # self._ensure_exists() - - # current_version = self._version() if self._version() else SAM_CONFIG_VERSION - # try: - # self.document.add(VERSION_KEY, current_version) - # except tomlkit.exceptions.KeyAlreadyPresent: - # # NOTE(TheSriram): Do not attempt to re-write an existing version - # pass - # self.filepath.write_text(tomlkit.dumps(self.document)) if not self.document: return From da302673a470fb2cbaffd44331a14c7eb16c8d82 Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Fri, 26 May 2023 14:24:22 -0700 Subject: [PATCH 03/16] Generalize exception for FileManager --- samcli/lib/config/exceptions.py | 17 +++++++++++++++-- samcli/lib/config/samconfig.py | 9 ++++++--- tests/unit/cli/test_cli_config_file.py | 4 ++-- 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/samcli/lib/config/exceptions.py b/samcli/lib/config/exceptions.py index 6bf993de9b..1845411017 100644 --- a/samcli/lib/config/exceptions.py +++ b/samcli/lib/config/exceptions.py @@ -3,9 +3,22 @@ """ -class SamConfigVersionException(Exception): +from samcli.commands.exceptions import UserException + + +class SamConfigVersionException(UserException): + """Exception for the `samconfig` file being not present or in unrecognized format""" + + pass + + +class FileParseException(UserException): + """Exception when a file is incorrectly parsed by a FileManager object.""" + pass -class SamConfigFileReadException(Exception): +class SamConfigFileReadException(UserException): + """Exception when a `samconfig` file is read incorrectly.""" + pass diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index 441754e014..30f56772a7 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -10,7 +10,7 @@ import tomlkit -from samcli.lib.config.exceptions import SamConfigFileReadException, SamConfigVersionException +from samcli.lib.config.exceptions import FileParseException, SamConfigFileReadException, SamConfigVersionException from samcli.lib.config.version import SAM_CONFIG_VERSION, VERSION_KEY LOG = logging.getLogger(__name__) @@ -51,7 +51,7 @@ def read(filepath: Path) -> dict: except OSError: document = {} except tomlkit.exceptions.TOMLKitError as e: - raise SamConfigFileReadException(e) + raise FileParseException(e) from e return document @@ -237,7 +237,10 @@ def config_dir(template_file_path=None): def _read(self): if not self.document: - self.document = self.file_manager.read(self.filepath) + try: + self.document = self.file_manager.read(self.filepath) + except FileParseException as e: + raise SamConfigFileReadException(e) from e if self.document: self._version_sanity_check(self._version()) return self.document diff --git a/tests/unit/cli/test_cli_config_file.py b/tests/unit/cli/test_cli_config_file.py index 8177b3b9da..3373aac123 100644 --- a/tests/unit/cli/test_cli_config_file.py +++ b/tests/unit/cli/test_cli_config_file.py @@ -9,7 +9,7 @@ from samcli.commands.exceptions import ConfigException from samcli.cli.cli_config_file import TomlProvider, configuration_option, configuration_callback, get_ctx_defaults -from samcli.lib.config.exceptions import SamConfigFileReadException, SamConfigVersionException +from samcli.lib.config.exceptions import FileParseException, SamConfigFileReadException, SamConfigVersionException from samcli.lib.config.samconfig import DEFAULT_ENV, TomlFileManager from tests.testing_utils import IS_WINDOWS @@ -250,7 +250,7 @@ def test_read_toml_invalid_toml(self): config_dir = tempfile.gettempdir() config_path = Path(config_dir, "samconfig.toml") config_path.write_text("fake='not real'\nimproper toml file\n") - with self.assertRaises(SamConfigFileReadException): + with self.assertRaises(FileParseException): TomlFileManager.read(config_path) def test_read_toml_file_path_not_valid(self): From c352d7e995765d3d90f5da69d2c205d2283edb4e Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Fri, 26 May 2023 14:47:53 -0700 Subject: [PATCH 04/16] Remove FileManager logic to its own file --- samcli/lib/config/file_manager.py | 99 +++++++++++++++++++ samcli/lib/config/samconfig.py | 50 +--------- tests/unit/cli/test_cli_config_file.py | 50 ---------- tests/unit/lib/samconfig/test_file_manager.py | 56 +++++++++++ 4 files changed, 158 insertions(+), 97 deletions(-) create mode 100644 samcli/lib/config/file_manager.py create mode 100644 tests/unit/lib/samconfig/test_file_manager.py diff --git a/samcli/lib/config/file_manager.py b/samcli/lib/config/file_manager.py new file mode 100644 index 0000000000..766400af7e --- /dev/null +++ b/samcli/lib/config/file_manager.py @@ -0,0 +1,99 @@ +""" +Class to represent the parsing of different file types into Python objects. +""" + + +from abc import ABC, abstractmethod +from pathlib import Path + +import tomlkit + +from samcli.lib.config.exceptions import FileParseException + + +class FileManager(ABC): + """ + Abstract class to be overridden by file managers for specific file extensions. + """ + + @staticmethod + @abstractmethod + def read(filepath: Path) -> dict: + """ + Read a file at a given path. + + Parameters + ---------- + filepath: Path + The Path object that points to the file to be read. + + Returns + ------- + dict + The dictionary representation of the contents at the filepath location. + """ + raise NotImplementedError("Read method not implemented.") + + @staticmethod + @abstractmethod + def write(document: dict, filepath: Path): + """ + Write a dictionary to a given file. + + Parameters + ---------- + document: dict + The object to write. + filepath: Path + The final location for the document to be written. + """ + raise NotImplementedError("Write method not implemented.") + + +class TomlFileManager(FileManager): + """ + Static class to read and write toml files. + """ + + @staticmethod + def read(filepath: Path) -> dict: + """ + Read a TOML file at the given path. + + Parameters + ---------- + filepath: Path + The Path object that points to the file to be read. + + Returns + ------- + dict + A Python dictionary representation of the contents of the TOML file at the provided location. + """ + document: dict = {} + try: + txt = filepath.read_text() + document = dict(tomlkit.loads(txt)) + except OSError: + document = {} + except tomlkit.exceptions.TOMLKitError as e: + raise FileParseException(e) from e + + return document + + @staticmethod + def write(document: dict, filepath: Path): + """ + Write the contents of a dictionary to a TOML file at the provided location. + + Parameters + ---------- + document: dict + The object to write. + filepath: Path + The final location for the TOML file to be written. + """ + if not document: + return + + filepath.write_text(tomlkit.dumps(document)) diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index 30f56772a7..ca2fae54b2 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -4,13 +4,11 @@ import logging import os -from abc import ABC, abstractmethod from pathlib import Path -from typing import Any, Iterable - -import tomlkit +from typing import Any, Dict, Iterable, Type from samcli.lib.config.exceptions import FileParseException, SamConfigFileReadException, SamConfigVersionException +from samcli.lib.config.file_manager import FileManager, TomlFileManager from samcli.lib.config.version import SAM_CONFIG_VERSION, VERSION_KEY LOG = logging.getLogger(__name__) @@ -21,54 +19,12 @@ DEFAULT_GLOBAL_CMDNAME = "global" -class FileManager(ABC): - """ - Abstract class to be overridden by file managers for specific file extensions. - """ - - @staticmethod - @abstractmethod - def read(filepath: Path) -> dict: - pass - - @staticmethod - @abstractmethod - def write(document: dict, filepath: Path): - pass - - -class TomlFileManager(FileManager): - """ - Static class to read and write toml files. - """ - - @staticmethod - def read(filepath: Path) -> dict: - document: dict = {} - try: - txt = filepath.read_text() - document = dict(tomlkit.loads(txt)) - except OSError: - document = {} - except tomlkit.exceptions.TOMLKitError as e: - raise FileParseException(e) from e - - return document - - @staticmethod - def write(document: dict, filepath: Path): - if not document: - return - - filepath.write_text(tomlkit.dumps(document)) - - class SamConfig: """ Class to represent `samconfig` config options. """ - FILE_MANAGER_MAPPER: dict = { + FILE_MANAGER_MAPPER: Dict[str, Type[FileManager]] = { "toml": TomlFileManager, } diff --git a/tests/unit/cli/test_cli_config_file.py b/tests/unit/cli/test_cli_config_file.py index 3373aac123..2b79e01efc 100644 --- a/tests/unit/cli/test_cli_config_file.py +++ b/tests/unit/cli/test_cli_config_file.py @@ -234,53 +234,3 @@ def test_get_ctx_defaults_nested(self): get_ctx_defaults("intent-answer", provider, mock_context4, "default") provider.assert_called_with(None, "default", ["local", "generate-event", "alexa-skills-kit", "intent-answer"]) - - -class TestTomlFileManager(TestCase): - def test_read_toml(self): - config_dir = tempfile.gettempdir() - config_path = Path(config_dir, "samconfig.toml") - config_path.write_text("version=0.1\n[config_env.topic1.parameters]\nword='clarity'\n") - self.assertEqual( - TomlFileManager.read(config_path), - {"version": 0.1, "config_env": {"topic1": {"parameters": {"word": "clarity"}}}}, - ) - - def test_read_toml_invalid_toml(self): - config_dir = tempfile.gettempdir() - config_path = Path(config_dir, "samconfig.toml") - config_path.write_text("fake='not real'\nimproper toml file\n") - with self.assertRaises(FileParseException): - TomlFileManager.read(config_path) - - def test_read_toml_file_path_not_valid(self): - config_dir = "path/that/doesnt/exist" - config_path = Path(config_dir, "samconfig.toml") - self.assertEqual(TomlFileManager.read(config_path), {}) - - def test_write_toml(self): - config_dir = tempfile.gettempdir() - config_path = Path(config_dir, "samconfig.toml") - toml = {"version": 0.1, "config_env": {"topic2": {"parameters": {"word": "clarity"}}}} - - TomlFileManager.write(toml, config_path) - - txt = config_path.read_text() - self.assertIn("version = 0.1", txt) - self.assertIn("[config_env.topic2.parameters]", txt) - self.assertIn('word = "clarity"', txt) - - def test_dont_write_toml_if_empty(self): - config_dir = tempfile.gettempdir() - config_path = Path(config_dir, "samconfig.toml") - config_path.write_text("nothing to see here\n") - toml = {} - - TomlFileManager.write(toml, config_path) - - self.assertEqual(config_path.read_text(), "nothing to see here\n") - - def test_write_toml_bad_path(self): - config_path = Path("path/to/some", "file_that_doesnt_exist.toml") - with self.assertRaises(FileNotFoundError): - TomlFileManager.write({"some key": "some value"}, config_path) diff --git a/tests/unit/lib/samconfig/test_file_manager.py b/tests/unit/lib/samconfig/test_file_manager.py new file mode 100644 index 0000000000..95f7d65c3a --- /dev/null +++ b/tests/unit/lib/samconfig/test_file_manager.py @@ -0,0 +1,56 @@ +from pathlib import Path +import tempfile +from unittest import TestCase +from samcli.lib.config.exceptions import FileParseException + +from samcli.lib.config.file_manager import TomlFileManager + + +class TestTomlFileManager(TestCase): + def test_read_toml(self): + config_dir = tempfile.gettempdir() + config_path = Path(config_dir, "samconfig.toml") + config_path.write_text("version=0.1\n[config_env.topic1.parameters]\nword='clarity'\n") + self.assertEqual( + TomlFileManager.read(config_path), + {"version": 0.1, "config_env": {"topic1": {"parameters": {"word": "clarity"}}}}, + ) + + def test_read_toml_invalid_toml(self): + config_dir = tempfile.gettempdir() + config_path = Path(config_dir, "samconfig.toml") + config_path.write_text("fake='not real'\nimproper toml file\n") + with self.assertRaises(FileParseException): + TomlFileManager.read(config_path) + + def test_read_toml_file_path_not_valid(self): + config_dir = "path/that/doesnt/exist" + config_path = Path(config_dir, "samconfig.toml") + self.assertEqual(TomlFileManager.read(config_path), {}) + + def test_write_toml(self): + config_dir = tempfile.gettempdir() + config_path = Path(config_dir, "samconfig.toml") + toml = {"version": 0.1, "config_env": {"topic2": {"parameters": {"word": "clarity"}}}} + + TomlFileManager.write(toml, config_path) + + txt = config_path.read_text() + self.assertIn("version = 0.1", txt) + self.assertIn("[config_env.topic2.parameters]", txt) + self.assertIn('word = "clarity"', txt) + + def test_dont_write_toml_if_empty(self): + config_dir = tempfile.gettempdir() + config_path = Path(config_dir, "samconfig.toml") + config_path.write_text("nothing to see here\n") + toml = {} + + TomlFileManager.write(toml, config_path) + + self.assertEqual(config_path.read_text(), "nothing to see here\n") + + def test_write_toml_bad_path(self): + config_path = Path("path/to/some", "file_that_doesnt_exist.toml") + with self.assertRaises(FileNotFoundError): + TomlFileManager.write({"some key": "some value"}, config_path) From 7bb0dc3ad8ff4f9ab9ffb5395984788362b662c9 Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Fri, 26 May 2023 14:58:30 -0700 Subject: [PATCH 05/16] Fix bug in setting a default FileManager --- samcli/lib/config/samconfig.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index ca2fae54b2..d294d63c05 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -42,9 +42,10 @@ def __init__(self, config_dir, filename=None): """ self.document = {} self.filepath = Path(config_dir, filename or DEFAULT_CONFIG_FILE_NAME) - self.file_manager = self.FILE_MANAGER_MAPPER.get(self.filepath.suffix[1:], TomlFileManager) # default to TOML + self.file_manager = self.FILE_MANAGER_MAPPER.get(self.filepath.suffix[1:], None) # default to TOML if not self.file_manager: LOG.warning(f"The config file extension '{self.filepath.suffix[1:]}' is not supported. TOML will be used.") + self.file_manager = TomlFileManager self._read() def get_stage_configuration_names(self): From 0b7e613897a2597c11e1443c8edc4bdfee131ea0 Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Mon, 29 May 2023 11:54:26 -0700 Subject: [PATCH 06/16] Implement requested changes This includes additional logging messages, as well as explicitly requiring file extensions --- samcli/lib/config/file_manager.py | 7 ++++++- samcli/lib/config/samconfig.py | 6 ++++-- tests/unit/cli/test_cli_config_file.py | 3 ++- tests/unit/commands/samconfig/test_samconfig.py | 8 ++++---- tests/unit/lib/samconfig/test_samconfig.py | 2 +- 5 files changed, 17 insertions(+), 9 deletions(-) diff --git a/samcli/lib/config/file_manager.py b/samcli/lib/config/file_manager.py index 766400af7e..eadb6207f1 100644 --- a/samcli/lib/config/file_manager.py +++ b/samcli/lib/config/file_manager.py @@ -3,6 +3,7 @@ """ +import logging from abc import ABC, abstractmethod from pathlib import Path @@ -10,6 +11,8 @@ from samcli.lib.config.exceptions import FileParseException +LOG = logging.getLogger(__name__) + class FileManager(ABC): """ @@ -74,7 +77,8 @@ def read(filepath: Path) -> dict: try: txt = filepath.read_text() document = dict(tomlkit.loads(txt)) - except OSError: + except OSError as e: + LOG.debug(f"OSError occurred while reading TOML file: {str(e)}") document = {} except tomlkit.exceptions.TOMLKitError as e: raise FileParseException(e) from e @@ -94,6 +98,7 @@ def write(document: dict, filepath: Path): The final location for the TOML file to be written. """ if not document: + LOG.debug("No document given for TomlFileManager to write.") return filepath.write_text(tomlkit.dumps(document)) diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index d294d63c05..836aca7678 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -44,8 +44,10 @@ def __init__(self, config_dir, filename=None): self.filepath = Path(config_dir, filename or DEFAULT_CONFIG_FILE_NAME) self.file_manager = self.FILE_MANAGER_MAPPER.get(self.filepath.suffix[1:], None) # default to TOML if not self.file_manager: - LOG.warning(f"The config file extension '{self.filepath.suffix[1:]}' is not supported. TOML will be used.") - self.file_manager = TomlFileManager + LOG.warning(f"The config file extension '{self.filepath.suffix[1:]}' is not supported.") + raise SamConfigFileReadException( + f"The config file {self.filepath} uses an unsupported extension, and cannot be read." + ) self._read() def get_stage_configuration_names(self): diff --git a/tests/unit/cli/test_cli_config_file.py b/tests/unit/cli/test_cli_config_file.py index 2b79e01efc..3395c12786 100644 --- a/tests/unit/cli/test_cli_config_file.py +++ b/tests/unit/cli/test_cli_config_file.py @@ -58,7 +58,8 @@ def test_toml_invalid_empty_dict(self): config_path = Path(config_dir, "samconfig.toml") config_path.write_text("[topic]\nword=clarity\n") - self.assertEqual(self.toml_provider(config_dir, self.config_env, [self.cmd_name]), {}) + with self.assertRaises(SamConfigFileReadException): + self.toml_provider(config_path, self.config_env, [self.cmd_name]) def test_toml_invalid_file_name(self): config_dir = tempfile.gettempdir() diff --git a/tests/unit/commands/samconfig/test_samconfig.py b/tests/unit/commands/samconfig/test_samconfig.py index 6f69ae87eb..c9d7e442fc 100644 --- a/tests/unit/commands/samconfig/test_samconfig.py +++ b/tests/unit/commands/samconfig/test_samconfig.py @@ -14,6 +14,7 @@ from unittest import TestCase from unittest.mock import patch, ANY import logging +from samcli.lib.config.exceptions import SamConfigFileReadException from samcli.lib.config.samconfig import SamConfig, DEFAULT_ENV, TomlFileManager from samcli.lib.utils.packagetype import ZIP, IMAGE @@ -1246,13 +1247,12 @@ def test_secondary_option_name_template_validate(self, do_cli_mock): class TestSamConfigFileManager(TestCase): - def test_file_manager_defaults_to_toml(self): + def test_file_manager_not_declared(self): config_dir = tempfile.gettempdir() config_path = Path(config_dir, "samconfig") - samconfig = SamConfig(config_path, filename="samconfig") - - self.assertIs(samconfig.file_manager, TomlFileManager) + with self.assertRaises(SamConfigFileReadException): + SamConfig(config_path, filename="samconfig") def test_file_manager_toml(self): config_dir = tempfile.gettempdir() diff --git a/tests/unit/lib/samconfig/test_samconfig.py b/tests/unit/lib/samconfig/test_samconfig.py index b3a3a7e858..346353501b 100644 --- a/tests/unit/lib/samconfig/test_samconfig.py +++ b/tests/unit/lib/samconfig/test_samconfig.py @@ -210,7 +210,7 @@ def test_write_config_file_non_standard_version(self): def test_write_config_file_will_create_the_file_if_not_exist(self): with osutils.mkdir_temp(ignore_errors=True) as tempdir: non_existing_dir = os.path.join(tempdir, "non-existing-dir") - non_existing_file = "non-existing-file" + non_existing_file = "non-existing-file.toml" samconfig = SamConfig(config_dir=non_existing_dir, filename=non_existing_file) self.assertFalse(samconfig.exists()) From d4549cc772782b1c875d124f760f2d84cd06159f Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Mon, 29 May 2023 13:23:16 -0700 Subject: [PATCH 07/16] Include supported extensions in log call --- samcli/lib/config/samconfig.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index 836aca7678..bb5dceecb5 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -44,7 +44,10 @@ def __init__(self, config_dir, filename=None): self.filepath = Path(config_dir, filename or DEFAULT_CONFIG_FILE_NAME) self.file_manager = self.FILE_MANAGER_MAPPER.get(self.filepath.suffix[1:], None) # default to TOML if not self.file_manager: - LOG.warning(f"The config file extension '{self.filepath.suffix[1:]}' is not supported.") + LOG.warning( + f"The config file extension '{self.filepath.suffix[1:]}' is not supported. " + f"Supported formats are: [.{'|.'.join(self.FILE_MANAGER_MAPPER.keys())}]" + ) raise SamConfigFileReadException( f"The config file {self.filepath} uses an unsupported extension, and cannot be read." ) From 58c270ef733b7b0474d2a09483242df465bb485b Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Mon, 29 May 2023 14:45:56 -0700 Subject: [PATCH 08/16] Implement requested changes --- samcli/lib/config/exceptions.py | 6 ------ samcli/lib/config/file_manager.py | 10 +++++++++- samcli/lib/config/samconfig.py | 5 ++--- tests/unit/lib/samconfig/test_file_manager.py | 7 ++++++- 4 files changed, 17 insertions(+), 11 deletions(-) diff --git a/samcli/lib/config/exceptions.py b/samcli/lib/config/exceptions.py index 1845411017..273a5fd183 100644 --- a/samcli/lib/config/exceptions.py +++ b/samcli/lib/config/exceptions.py @@ -9,16 +9,10 @@ class SamConfigVersionException(UserException): """Exception for the `samconfig` file being not present or in unrecognized format""" - pass - class FileParseException(UserException): """Exception when a file is incorrectly parsed by a FileManager object.""" - pass - class SamConfigFileReadException(UserException): """Exception when a `samconfig` file is read incorrectly.""" - - pass diff --git a/samcli/lib/config/file_manager.py b/samcli/lib/config/file_manager.py index eadb6207f1..06b52c31b4 100644 --- a/samcli/lib/config/file_manager.py +++ b/samcli/lib/config/file_manager.py @@ -101,4 +101,12 @@ def write(document: dict, filepath: Path): LOG.debug("No document given for TomlFileManager to write.") return - filepath.write_text(tomlkit.dumps(document)) + doc_no_comments = {k: v for k, v in document.items() if k != "__comment__"} + toml_doc = tomlkit.document() + + if document.get("__comment__", None): # Comment appears at the top of doc + toml_doc.add(tomlkit.comment(document["__comment__"])) + for k, v in doc_no_comments.items(): + toml_doc.add(k, v) + + filepath.write_text(tomlkit.dumps(toml_doc)) diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index bb5dceecb5..7d13915a5a 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -42,7 +42,7 @@ def __init__(self, config_dir, filename=None): """ self.document = {} self.filepath = Path(config_dir, filename or DEFAULT_CONFIG_FILE_NAME) - self.file_manager = self.FILE_MANAGER_MAPPER.get(self.filepath.suffix[1:], None) # default to TOML + self.file_manager = self.FILE_MANAGER_MAPPER.get(self.filepath.suffix[1:], None) if not self.file_manager: LOG.warning( f"The config file extension '{self.filepath.suffix[1:]}' is not supported. " @@ -87,7 +87,7 @@ def get_all(self, cmd_names, section, env=DEFAULT_ENV): env = env or DEFAULT_ENV - self.document = self.file_manager.read(self.filepath) + self.document = self._read() config_content = self.document.get(env, {}) params = config_content.get(self._to_key(cmd_names), {}).get(section, {}) @@ -163,7 +163,6 @@ def flush(self): """ self._write() - print(self.document) def sanity_check(self): """ diff --git a/tests/unit/lib/samconfig/test_file_manager.py b/tests/unit/lib/samconfig/test_file_manager.py index 95f7d65c3a..bb48c42a22 100644 --- a/tests/unit/lib/samconfig/test_file_manager.py +++ b/tests/unit/lib/samconfig/test_file_manager.py @@ -31,7 +31,11 @@ def test_read_toml_file_path_not_valid(self): def test_write_toml(self): config_dir = tempfile.gettempdir() config_path = Path(config_dir, "samconfig.toml") - toml = {"version": 0.1, "config_env": {"topic2": {"parameters": {"word": "clarity"}}}} + toml = { + "version": 0.1, + "config_env": {"topic2": {"parameters": {"word": "clarity"}}}, + "__comment__": "This is a comment", + } TomlFileManager.write(toml, config_path) @@ -39,6 +43,7 @@ def test_write_toml(self): self.assertIn("version = 0.1", txt) self.assertIn("[config_env.topic2.parameters]", txt) self.assertIn('word = "clarity"', txt) + self.assertIn("# This is a comment", txt) def test_dont_write_toml_if_empty(self): config_dir = tempfile.gettempdir() From 1e6bde8a87b8e65332cb4226705e567642a5e984 Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Mon, 29 May 2023 16:43:11 -0700 Subject: [PATCH 09/16] Update docstrings --- samcli/lib/config/samconfig.py | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index 7d13915a5a..fb5515ba21 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -80,9 +80,6 @@ def get_all(self, cmd_names, section, env=DEFAULT_ENV): ------ KeyError If the config file does *not* have the specific section - - SamConfigFileReadException - If the configuration file is invalid """ env = env or DEFAULT_ENV @@ -115,11 +112,6 @@ def put(self, cmd_names, section, key, value, env=DEFAULT_ENV): Value to write. Could be any of the supported types. env : str Optional, Name of the environment - - Raises - ------ - SamConfigFileReadException - If the data is invalid """ # Empty document prepare the initial structure. # self.document is a nested dict, we need to check each layer and add new tables, otherwise duplicated key @@ -155,12 +147,6 @@ def put_comment(self, comment): def flush(self): """ Write the data back to file - - Raises - ------ - SamConfigFileReadException - If the data is invalid - """ self._write() From 29b7e94708c94f695d9ace9167ce5025071e69e6 Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Tue, 30 May 2023 16:20:34 -0700 Subject: [PATCH 10/16] Refactor changes to preserve TOML comments --- samcli/lib/config/exceptions.py | 9 ++-- samcli/lib/config/file_manager.py | 54 ++++++++++++++++--- samcli/lib/config/samconfig.py | 43 ++++++++------- tests/unit/lib/samconfig/test_file_manager.py | 38 +++++++++++-- tests/unit/lib/samconfig/test_samconfig.py | 22 ++++---- 5 files changed, 119 insertions(+), 47 deletions(-) diff --git a/samcli/lib/config/exceptions.py b/samcli/lib/config/exceptions.py index 273a5fd183..c179b4a13c 100644 --- a/samcli/lib/config/exceptions.py +++ b/samcli/lib/config/exceptions.py @@ -3,16 +3,13 @@ """ -from samcli.commands.exceptions import UserException - - -class SamConfigVersionException(UserException): +class SamConfigVersionException(Exception): """Exception for the `samconfig` file being not present or in unrecognized format""" -class FileParseException(UserException): +class FileParseException(Exception): """Exception when a file is incorrectly parsed by a FileManager object.""" -class SamConfigFileReadException(UserException): +class SamConfigFileReadException(Exception): """Exception when a `samconfig` file is read incorrectly.""" diff --git a/samcli/lib/config/file_manager.py b/samcli/lib/config/file_manager.py index 06b52c31b4..7326745519 100644 --- a/samcli/lib/config/file_manager.py +++ b/samcli/lib/config/file_manager.py @@ -6,6 +6,7 @@ import logging from abc import ABC, abstractmethod from pathlib import Path +from typing import Any, Optional, Tuple import tomlkit @@ -21,7 +22,7 @@ class FileManager(ABC): @staticmethod @abstractmethod - def read(filepath: Path) -> dict: + def read(filepath: Path) -> Tuple[dict, Optional[Any]]: """ Read a file at a given path. @@ -32,8 +33,9 @@ def read(filepath: Path) -> dict: Returns ------- - dict - The dictionary representation of the contents at the filepath location. + Tuple[dict, Optional[Any]] + The dictionary representation of the contents at the filepath location, along with a specialized + representation of the file that was read, if there is a specialization of it. """ raise NotImplementedError("Read method not implemented.") @@ -52,6 +54,21 @@ def write(document: dict, filepath: Path): """ raise NotImplementedError("Write method not implemented.") + @staticmethod + @abstractmethod + def write_document(document: Any, filepath: Path): + """ + Write the contents of a document object to a file at the provided location. + + Parameters + ---------- + document: Any + The object to write. + filepath: Path + The final location for the file to be written. + """ + raise NotImplementedError("Write document method not implemented.") + class TomlFileManager(FileManager): """ @@ -59,7 +76,7 @@ class TomlFileManager(FileManager): """ @staticmethod - def read(filepath: Path) -> dict: + def read(filepath: Path) -> Tuple[dict, Optional[Any]]: """ Read a TOML file at the given path. @@ -70,20 +87,23 @@ def read(filepath: Path) -> dict: Returns ------- - dict - A Python dictionary representation of the contents of the TOML file at the provided location. + Tuple[dict, Optional[Any]] + A Python dictionary representation of the contents of the TOML file at the provided location, as well as a + tomlkit document object of the TOML contents. """ document: dict = {} + toml_doc = None try: txt = filepath.read_text() - document = dict(tomlkit.loads(txt)) + toml_doc = tomlkit.loads(txt) + document = dict(toml_doc) except OSError as e: LOG.debug(f"OSError occurred while reading TOML file: {str(e)}") document = {} except tomlkit.exceptions.TOMLKitError as e: raise FileParseException(e) from e - return document + return document, toml_doc @staticmethod def write(document: dict, filepath: Path): @@ -110,3 +130,21 @@ def write(document: dict, filepath: Path): toml_doc.add(k, v) filepath.write_text(tomlkit.dumps(toml_doc)) + + @staticmethod + def write_document(document: Any, filepath: Path): + """ + Write the contents of a tomlkit.TOMLDocument object to a TOML file at the provided location. + + Parameters + ---------- + document: Any + The object to write. + filepath: Path + The final location for the TOML file to be written. + """ + if not document: + LOG.debug("No TOMLDocument given for TomlFileManager to write.") + return + + filepath.write_text(tomlkit.dumps(document)) diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index fb5515ba21..c5d87656a1 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -40,7 +40,8 @@ def __init__(self, config_dir, filename=None): Optional. Name of the configuration file. It is recommended to stick with default so in the future we could automatically support auto-resolving multiple config files within same directory. """ - self.document = {} + self.data = {} + self.file_document = None self.filepath = Path(config_dir, filename or DEFAULT_CONFIG_FILE_NAME) self.file_manager = self.FILE_MANAGER_MAPPER.get(self.filepath.suffix[1:], None) if not self.file_manager: @@ -54,8 +55,8 @@ def __init__(self, config_dir, filename=None): self._read() def get_stage_configuration_names(self): - if self.document: - return [stage for stage, value in self.document.items() if isinstance(value, dict)] + if self.data: + return [stage for stage, value in self.data.items() if isinstance(value, dict)] return [] def get_all(self, cmd_names, section, env=DEFAULT_ENV): @@ -84,9 +85,9 @@ def get_all(self, cmd_names, section, env=DEFAULT_ENV): env = env or DEFAULT_ENV - self.document = self._read() + self.data = self._read() - config_content = self.document.get(env, {}) + config_content = self.data.get(env, {}) params = config_content.get(self._to_key(cmd_names), {}).get(section, {}) if DEFAULT_GLOBAL_CMDNAME in config_content: global_params = config_content.get(DEFAULT_GLOBAL_CMDNAME, {}).get(section, {}) @@ -117,7 +118,7 @@ def put(self, cmd_names, section, key, value, env=DEFAULT_ENV): # self.document is a nested dict, we need to check each layer and add new tables, otherwise duplicated key # in parent layer will override the whole child layer cmd_name_key = self._to_key(cmd_names) - env_content = self.document.get(env, {}) + env_content = self.data.get(env, {}) cmd_content = env_content.get(cmd_name_key, {}) param_content = cmd_content.get(section, {}) if param_content: @@ -127,7 +128,7 @@ def put(self, cmd_names, section, key, value, env=DEFAULT_ENV): elif env_content: env_content.update({cmd_name_key: {section: {key: value}}}) else: - self.document.update({env: {cmd_name_key: {section: {key: value}}}}) + self.data.update({env: {cmd_name_key: {section: {key: value}}}}) # If the value we want to add to samconfig already exist in global section, we don't put it again in # the special command section self._deduplicate_global_parameters(cmd_name_key, section, key, env) @@ -142,7 +143,7 @@ def put_comment(self, comment): A comment to write to the samconfg file """ - self.document.update({"__comment__": comment}) + self.data.update({"__comment__": comment}) def flush(self): """ @@ -183,28 +184,32 @@ def config_dir(template_file_path=None): return os.getcwd() def _read(self): - if not self.document: + if not self.data: try: - self.document = self.file_manager.read(self.filepath) + self.data, self.file_document = self.file_manager.read(self.filepath) except FileParseException as e: raise SamConfigFileReadException(e) from e - if self.document: + if self.data: self._version_sanity_check(self._version()) - return self.document + return self.data def _write(self): - if not self.document: + if not self.data: return self._ensure_exists() current_version = self._version() if self._version() else SAM_CONFIG_VERSION - self.document.update({VERSION_KEY: current_version}) + self.data.update({VERSION_KEY: current_version}) - self.file_manager.write(self.document, self.filepath) + if self.file_document: + self.file_document.update({VERSION_KEY: current_version}) + self.file_manager.write_document(self.file_document, self.filepath) + else: + self.file_manager.write(self.data, self.filepath) def _version(self): - return self.document.get(VERSION_KEY, None) + return self.data.get(VERSION_KEY, None) def _deduplicate_global_parameters(self, cmd_name_key, section, key, env=DEFAULT_ENV): """ @@ -225,8 +230,8 @@ def _deduplicate_global_parameters(self, cmd_name_key, section, key, env=DEFAULT env : str Optional, Name of the environment """ - global_params = self.document.get(env, {}).get(DEFAULT_GLOBAL_CMDNAME, {}).get(section, {}) - command_params = self.document.get(env, {}).get(cmd_name_key, {}).get(section, {}) + global_params = self.data.get(env, {}).get(DEFAULT_GLOBAL_CMDNAME, {}).get(section, {}) + command_params = self.data.get(env, {}).get(cmd_name_key, {}).get(section, {}) if ( cmd_name_key != DEFAULT_GLOBAL_CMDNAME and global_params @@ -242,7 +247,7 @@ def _deduplicate_global_parameters(self, cmd_name_key, section, key, env=DEFAULT ) LOG.info(save_global_message) # Only keep the global parameter - del self.document[env][cmd_name_key][section][key] + del self.data[env][cmd_name_key][section][key] @staticmethod def _version_sanity_check(version: Any) -> None: diff --git a/tests/unit/lib/samconfig/test_file_manager.py b/tests/unit/lib/samconfig/test_file_manager.py index bb48c42a22..9d6d617a78 100644 --- a/tests/unit/lib/samconfig/test_file_manager.py +++ b/tests/unit/lib/samconfig/test_file_manager.py @@ -1,6 +1,8 @@ from pathlib import Path import tempfile from unittest import TestCase + +import tomlkit from samcli.lib.config.exceptions import FileParseException from samcli.lib.config.file_manager import TomlFileManager @@ -11,8 +13,9 @@ def test_read_toml(self): config_dir = tempfile.gettempdir() config_path = Path(config_dir, "samconfig.toml") config_path.write_text("version=0.1\n[config_env.topic1.parameters]\nword='clarity'\n") + config_dict, _ = TomlFileManager.read(config_path) self.assertEqual( - TomlFileManager.read(config_path), + config_dict, {"version": 0.1, "config_env": {"topic1": {"parameters": {"word": "clarity"}}}}, ) @@ -26,7 +29,8 @@ def test_read_toml_invalid_toml(self): def test_read_toml_file_path_not_valid(self): config_dir = "path/that/doesnt/exist" config_path = Path(config_dir, "samconfig.toml") - self.assertEqual(TomlFileManager.read(config_path), {}) + config_dict, _ = TomlFileManager.read(config_path) + self.assertEqual(config_dict, {}) def test_write_toml(self): config_dir = tempfile.gettempdir() @@ -58,4 +62,32 @@ def test_dont_write_toml_if_empty(self): def test_write_toml_bad_path(self): config_path = Path("path/to/some", "file_that_doesnt_exist.toml") with self.assertRaises(FileNotFoundError): - TomlFileManager.write({"some key": "some value"}, config_path) + TomlFileManager.write({"key": "some value"}, config_path) + + def test_write_toml_file(self): + config_dir = tempfile.gettempdir() + config_path = Path(config_dir, "samconfig.toml") + toml = tomlkit.parse('# This is a comment\nversion = 0.1\n[config_env.topic2.parameters]\nword = "clarity"\n') + + TomlFileManager.write_document(toml, config_path) + + txt = config_path.read_text() + self.assertIn("version = 0.1", txt) + self.assertIn("[config_env.topic2.parameters]", txt) + self.assertIn('word = "clarity"', txt) + self.assertIn("# This is a comment", txt) + + def test_dont_write_toml_file_if_empty(self): + config_dir = tempfile.gettempdir() + config_path = Path(config_dir, "samconfig.toml") + config_path.write_text("nothing to see here\n") + toml = tomlkit.document() + + TomlFileManager.write(toml, config_path) + + self.assertEqual(config_path.read_text(), "nothing to see here\n") + + def test_write_toml_file_bad_path(self): + config_path = Path("path/to/some", "file_that_doesnt_exist.toml") + with self.assertRaises(FileNotFoundError): + TomlFileManager.write_document(tomlkit.parse('key = "some value"'), config_path) diff --git a/tests/unit/lib/samconfig/test_samconfig.py b/tests/unit/lib/samconfig/test_samconfig.py index 346353501b..e5457864a6 100644 --- a/tests/unit/lib/samconfig/test_samconfig.py +++ b/tests/unit/lib/samconfig/test_samconfig.py @@ -25,7 +25,7 @@ def _setup_config(self): def _check_config_file(self): self.assertTrue(self.samconfig.exists()) self.assertTrue(self.samconfig.sanity_check()) - self.assertEqual(SAM_CONFIG_VERSION, self.samconfig.document.get(VERSION_KEY)) + self.assertEqual(SAM_CONFIG_VERSION, self.samconfig.data.get(VERSION_KEY)) def _update_samconfig(self, cmd_names, section, key, value, env=None): if env: @@ -162,9 +162,9 @@ def test_dedup_global_param(self): {"testKey": "ValueFromGlobal"}, self.samconfig.get_all(cmd_names=["myCommand"], section="mySection", env="myEnv"), ) - self.assertEqual(self.samconfig.document["myEnv"]["myCommand"]["mySection"], {}) + self.assertEqual(self.samconfig.data["myEnv"]["myCommand"]["mySection"], {}) self.assertEqual( - self.samconfig.document["myEnv"][DEFAULT_GLOBAL_CMDNAME]["mySection"], {"testKey": "ValueFromGlobal"} + self.samconfig.data["myEnv"][DEFAULT_GLOBAL_CMDNAME]["mySection"], {"testKey": "ValueFromGlobal"} ) def test_check_config_get(self): @@ -182,30 +182,30 @@ def test_check_sanity(self): def test_check_version_non_supported_type(self): self._setup_config() - self.samconfig.document.pop(VERSION_KEY) - self.samconfig.document.update({VERSION_KEY: "aadeff"}) + self.samconfig.data.pop(VERSION_KEY) + self.samconfig.data.update({VERSION_KEY: "aadeff"}) with self.assertRaises(SamConfigVersionException): self.samconfig.sanity_check() def test_check_version_no_version_exists(self): self._setup_config() - self.samconfig.document.pop(VERSION_KEY) + self.samconfig.data.pop(VERSION_KEY) with self.assertRaises(SamConfigVersionException): self.samconfig.sanity_check() def test_check_version_float(self): self._setup_config() - self.samconfig.document.pop(VERSION_KEY) - self.samconfig.document.update({VERSION_KEY: 0.2}) + self.samconfig.data.pop(VERSION_KEY) + self.samconfig.data.update({VERSION_KEY: 0.2}) self.samconfig.sanity_check() def test_write_config_file_non_standard_version(self): self._setup_config() - self.samconfig.document.pop(VERSION_KEY) - self.samconfig.document.update({VERSION_KEY: 0.2}) + self.samconfig.data.pop(VERSION_KEY) + self.samconfig.data.update({VERSION_KEY: 0.2}) self.samconfig.put(cmd_names=["local", "start", "api"], section="parameters", key="skip_pull_image", value=True) self.samconfig.sanity_check() - self.assertEqual(self.samconfig.document.get(VERSION_KEY), 0.2) + self.assertEqual(self.samconfig.data.get(VERSION_KEY), 0.2) def test_write_config_file_will_create_the_file_if_not_exist(self): with osutils.mkdir_temp(ignore_errors=True) as tempdir: From 1b4e58392376b141eb51db9bd701f483daaf3bc0 Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Tue, 30 May 2023 16:27:52 -0700 Subject: [PATCH 11/16] Allow file document to update properly --- samcli/lib/config/samconfig.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index c5d87656a1..206d96a215 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -129,6 +129,8 @@ def put(self, cmd_names, section, key, value, env=DEFAULT_ENV): env_content.update({cmd_name_key: {section: {key: value}}}) else: self.data.update({env: {cmd_name_key: {section: {key: value}}}}) + if self.file_document: + self.file_document.update({env: {cmd_name_key: {section: {key: value}}}}) # If the value we want to add to samconfig already exist in global section, we don't put it again in # the special command section self._deduplicate_global_parameters(cmd_name_key, section, key, env) From 1da7c89c3fac0a45c777a5d275e627fff022d6f9 Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Wed, 31 May 2023 11:15:10 -0700 Subject: [PATCH 12/16] Remove duplicate data Since TOMLDocument wraps a Python dictionary anyway, we don't need the separate information --- samcli/lib/config/file_manager.py | 23 +++++----- samcli/lib/config/samconfig.py | 45 ++++++++----------- tests/unit/lib/samconfig/test_file_manager.py | 8 ++-- tests/unit/lib/samconfig/test_samconfig.py | 22 ++++----- 4 files changed, 44 insertions(+), 54 deletions(-) diff --git a/samcli/lib/config/file_manager.py b/samcli/lib/config/file_manager.py index 7326745519..a7f5a7b66b 100644 --- a/samcli/lib/config/file_manager.py +++ b/samcli/lib/config/file_manager.py @@ -6,7 +6,7 @@ import logging from abc import ABC, abstractmethod from pathlib import Path -from typing import Any, Optional, Tuple +from typing import Any import tomlkit @@ -22,7 +22,7 @@ class FileManager(ABC): @staticmethod @abstractmethod - def read(filepath: Path) -> Tuple[dict, Optional[Any]]: + def read(filepath: Path) -> Any: """ Read a file at a given path. @@ -33,8 +33,8 @@ def read(filepath: Path) -> Tuple[dict, Optional[Any]]: Returns ------- - Tuple[dict, Optional[Any]] - The dictionary representation of the contents at the filepath location, along with a specialized + Any + A dictionary-like representation of the contents at the filepath location, along with a specialized representation of the file that was read, if there is a specialization of it. """ raise NotImplementedError("Read method not implemented.") @@ -76,7 +76,7 @@ class TomlFileManager(FileManager): """ @staticmethod - def read(filepath: Path) -> Tuple[dict, Optional[Any]]: + def read(filepath: Path) -> Any: """ Read a TOML file at the given path. @@ -87,23 +87,20 @@ def read(filepath: Path) -> Tuple[dict, Optional[Any]]: Returns ------- - Tuple[dict, Optional[Any]] - A Python dictionary representation of the contents of the TOML file at the provided location, as well as a - tomlkit document object of the TOML contents. + Any + A dictionary-like tomlkit.TOMLDocument object, which represents the contents of the TOML file at the + provided location. """ - document: dict = {} - toml_doc = None + toml_doc = tomlkit.document() try: txt = filepath.read_text() toml_doc = tomlkit.loads(txt) - document = dict(toml_doc) except OSError as e: LOG.debug(f"OSError occurred while reading TOML file: {str(e)}") - document = {} except tomlkit.exceptions.TOMLKitError as e: raise FileParseException(e) from e - return document, toml_doc + return toml_doc @staticmethod def write(document: dict, filepath: Path): diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index 206d96a215..e998381558 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -40,8 +40,7 @@ def __init__(self, config_dir, filename=None): Optional. Name of the configuration file. It is recommended to stick with default so in the future we could automatically support auto-resolving multiple config files within same directory. """ - self.data = {} - self.file_document = None + self.document = {} self.filepath = Path(config_dir, filename or DEFAULT_CONFIG_FILE_NAME) self.file_manager = self.FILE_MANAGER_MAPPER.get(self.filepath.suffix[1:], None) if not self.file_manager: @@ -55,8 +54,8 @@ def __init__(self, config_dir, filename=None): self._read() def get_stage_configuration_names(self): - if self.data: - return [stage for stage, value in self.data.items() if isinstance(value, dict)] + if self.document: + return [stage for stage, value in self.document.items() if isinstance(value, dict)] return [] def get_all(self, cmd_names, section, env=DEFAULT_ENV): @@ -85,9 +84,9 @@ def get_all(self, cmd_names, section, env=DEFAULT_ENV): env = env or DEFAULT_ENV - self.data = self._read() + self.document = self._read() - config_content = self.data.get(env, {}) + config_content = self.document.get(env, {}) params = config_content.get(self._to_key(cmd_names), {}).get(section, {}) if DEFAULT_GLOBAL_CMDNAME in config_content: global_params = config_content.get(DEFAULT_GLOBAL_CMDNAME, {}).get(section, {}) @@ -118,7 +117,7 @@ def put(self, cmd_names, section, key, value, env=DEFAULT_ENV): # self.document is a nested dict, we need to check each layer and add new tables, otherwise duplicated key # in parent layer will override the whole child layer cmd_name_key = self._to_key(cmd_names) - env_content = self.data.get(env, {}) + env_content = self.document.get(env, {}) cmd_content = env_content.get(cmd_name_key, {}) param_content = cmd_content.get(section, {}) if param_content: @@ -128,9 +127,7 @@ def put(self, cmd_names, section, key, value, env=DEFAULT_ENV): elif env_content: env_content.update({cmd_name_key: {section: {key: value}}}) else: - self.data.update({env: {cmd_name_key: {section: {key: value}}}}) - if self.file_document: - self.file_document.update({env: {cmd_name_key: {section: {key: value}}}}) + self.document.update({env: {cmd_name_key: {section: {key: value}}}}) # If the value we want to add to samconfig already exist in global section, we don't put it again in # the special command section self._deduplicate_global_parameters(cmd_name_key, section, key, env) @@ -145,7 +142,7 @@ def put_comment(self, comment): A comment to write to the samconfg file """ - self.data.update({"__comment__": comment}) + self.document.update({"__comment__": comment}) def flush(self): """ @@ -186,32 +183,28 @@ def config_dir(template_file_path=None): return os.getcwd() def _read(self): - if not self.data: + if not self.document: try: - self.data, self.file_document = self.file_manager.read(self.filepath) + self.document = self.file_manager.read(self.filepath) except FileParseException as e: raise SamConfigFileReadException(e) from e - if self.data: + if self.document: self._version_sanity_check(self._version()) - return self.data + return self.document def _write(self): - if not self.data: + if not self.document: return self._ensure_exists() current_version = self._version() if self._version() else SAM_CONFIG_VERSION - self.data.update({VERSION_KEY: current_version}) + self.document.update({VERSION_KEY: current_version}) - if self.file_document: - self.file_document.update({VERSION_KEY: current_version}) - self.file_manager.write_document(self.file_document, self.filepath) - else: - self.file_manager.write(self.data, self.filepath) + self.file_manager.write_document(self.document, self.filepath) def _version(self): - return self.data.get(VERSION_KEY, None) + return self.document.get(VERSION_KEY, None) def _deduplicate_global_parameters(self, cmd_name_key, section, key, env=DEFAULT_ENV): """ @@ -232,8 +225,8 @@ def _deduplicate_global_parameters(self, cmd_name_key, section, key, env=DEFAULT env : str Optional, Name of the environment """ - global_params = self.data.get(env, {}).get(DEFAULT_GLOBAL_CMDNAME, {}).get(section, {}) - command_params = self.data.get(env, {}).get(cmd_name_key, {}).get(section, {}) + global_params = self.document.get(env, {}).get(DEFAULT_GLOBAL_CMDNAME, {}).get(section, {}) + command_params = self.document.get(env, {}).get(cmd_name_key, {}).get(section, {}) if ( cmd_name_key != DEFAULT_GLOBAL_CMDNAME and global_params @@ -249,7 +242,7 @@ def _deduplicate_global_parameters(self, cmd_name_key, section, key, env=DEFAULT ) LOG.info(save_global_message) # Only keep the global parameter - del self.data[env][cmd_name_key][section][key] + del self.document[env][cmd_name_key][section][key] @staticmethod def _version_sanity_check(version: Any) -> None: diff --git a/tests/unit/lib/samconfig/test_file_manager.py b/tests/unit/lib/samconfig/test_file_manager.py index 9d6d617a78..ad24522c38 100644 --- a/tests/unit/lib/samconfig/test_file_manager.py +++ b/tests/unit/lib/samconfig/test_file_manager.py @@ -13,9 +13,9 @@ def test_read_toml(self): config_dir = tempfile.gettempdir() config_path = Path(config_dir, "samconfig.toml") config_path.write_text("version=0.1\n[config_env.topic1.parameters]\nword='clarity'\n") - config_dict, _ = TomlFileManager.read(config_path) + config_doc = TomlFileManager.read(config_path) self.assertEqual( - config_dict, + config_doc, {"version": 0.1, "config_env": {"topic1": {"parameters": {"word": "clarity"}}}}, ) @@ -29,8 +29,8 @@ def test_read_toml_invalid_toml(self): def test_read_toml_file_path_not_valid(self): config_dir = "path/that/doesnt/exist" config_path = Path(config_dir, "samconfig.toml") - config_dict, _ = TomlFileManager.read(config_path) - self.assertEqual(config_dict, {}) + config_doc = TomlFileManager.read(config_path) + self.assertEqual(config_doc, tomlkit.document()) def test_write_toml(self): config_dir = tempfile.gettempdir() diff --git a/tests/unit/lib/samconfig/test_samconfig.py b/tests/unit/lib/samconfig/test_samconfig.py index e5457864a6..346353501b 100644 --- a/tests/unit/lib/samconfig/test_samconfig.py +++ b/tests/unit/lib/samconfig/test_samconfig.py @@ -25,7 +25,7 @@ def _setup_config(self): def _check_config_file(self): self.assertTrue(self.samconfig.exists()) self.assertTrue(self.samconfig.sanity_check()) - self.assertEqual(SAM_CONFIG_VERSION, self.samconfig.data.get(VERSION_KEY)) + self.assertEqual(SAM_CONFIG_VERSION, self.samconfig.document.get(VERSION_KEY)) def _update_samconfig(self, cmd_names, section, key, value, env=None): if env: @@ -162,9 +162,9 @@ def test_dedup_global_param(self): {"testKey": "ValueFromGlobal"}, self.samconfig.get_all(cmd_names=["myCommand"], section="mySection", env="myEnv"), ) - self.assertEqual(self.samconfig.data["myEnv"]["myCommand"]["mySection"], {}) + self.assertEqual(self.samconfig.document["myEnv"]["myCommand"]["mySection"], {}) self.assertEqual( - self.samconfig.data["myEnv"][DEFAULT_GLOBAL_CMDNAME]["mySection"], {"testKey": "ValueFromGlobal"} + self.samconfig.document["myEnv"][DEFAULT_GLOBAL_CMDNAME]["mySection"], {"testKey": "ValueFromGlobal"} ) def test_check_config_get(self): @@ -182,30 +182,30 @@ def test_check_sanity(self): def test_check_version_non_supported_type(self): self._setup_config() - self.samconfig.data.pop(VERSION_KEY) - self.samconfig.data.update({VERSION_KEY: "aadeff"}) + self.samconfig.document.pop(VERSION_KEY) + self.samconfig.document.update({VERSION_KEY: "aadeff"}) with self.assertRaises(SamConfigVersionException): self.samconfig.sanity_check() def test_check_version_no_version_exists(self): self._setup_config() - self.samconfig.data.pop(VERSION_KEY) + self.samconfig.document.pop(VERSION_KEY) with self.assertRaises(SamConfigVersionException): self.samconfig.sanity_check() def test_check_version_float(self): self._setup_config() - self.samconfig.data.pop(VERSION_KEY) - self.samconfig.data.update({VERSION_KEY: 0.2}) + self.samconfig.document.pop(VERSION_KEY) + self.samconfig.document.update({VERSION_KEY: 0.2}) self.samconfig.sanity_check() def test_write_config_file_non_standard_version(self): self._setup_config() - self.samconfig.data.pop(VERSION_KEY) - self.samconfig.data.update({VERSION_KEY: 0.2}) + self.samconfig.document.pop(VERSION_KEY) + self.samconfig.document.update({VERSION_KEY: 0.2}) self.samconfig.put(cmd_names=["local", "start", "api"], section="parameters", key="skip_pull_image", value=True) self.samconfig.sanity_check() - self.assertEqual(self.samconfig.data.get(VERSION_KEY), 0.2) + self.assertEqual(self.samconfig.document.get(VERSION_KEY), 0.2) def test_write_config_file_will_create_the_file_if_not_exist(self): with osutils.mkdir_temp(ignore_errors=True) as tempdir: From 3c41bc0b3a181abd79623640e2c73e01617e3872 Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Wed, 31 May 2023 11:22:12 -0700 Subject: [PATCH 13/16] Add put comment for FileManager --- samcli/lib/config/file_manager.py | 40 +++++++++++++++++++++++++++++++ samcli/lib/config/samconfig.py | 2 +- 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/samcli/lib/config/file_manager.py b/samcli/lib/config/file_manager.py index a7f5a7b66b..2a14e5dcb8 100644 --- a/samcli/lib/config/file_manager.py +++ b/samcli/lib/config/file_manager.py @@ -69,6 +69,26 @@ def write_document(document: Any, filepath: Path): """ raise NotImplementedError("Write document method not implemented.") + @staticmethod + @abstractmethod + def put_comment(document: Any, comment: str) -> Any: + """ + Put a comment in a document object. + + Parameters + ---------- + document: Any + The object to write + comment: str + The comment to include in the document. + + Returns + ------- + Any + The new document, with the comment added to it. + """ + raise NotImplementedError("Put comment method not implemented.") + class TomlFileManager(FileManager): """ @@ -145,3 +165,23 @@ def write_document(document: Any, filepath: Path): return filepath.write_text(tomlkit.dumps(document)) + + @staticmethod + def put_comment(document: Any, comment: str) -> Any: + """ + Put a comment in a document object. + + Parameters + ---------- + document: Any + The tomlkit.TOMLDocument object to write + comment: str + The comment to include in the document. + + Returns + ------- + Any + The new TOMLDocument, with the comment added to it. + """ + document.add(tomlkit.comment(comment)) + return document diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index e998381558..3e30ba1bef 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -142,7 +142,7 @@ def put_comment(self, comment): A comment to write to the samconfg file """ - self.document.update({"__comment__": comment}) + self.document = self.file_manager.put_comment(self.document, comment) def flush(self): """ From 24d1564cf3b781ebfa739f5fa535c17b124bcdbc Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Thu, 1 Jun 2023 11:19:37 -0700 Subject: [PATCH 14/16] Implement requested changes --- samcli/lib/config/file_manager.py | 59 ++++--------------- samcli/lib/config/samconfig.py | 16 ++--- tests/unit/lib/samconfig/test_file_manager.py | 16 +++-- 3 files changed, 33 insertions(+), 58 deletions(-) diff --git a/samcli/lib/config/file_manager.py b/samcli/lib/config/file_manager.py index 2a14e5dcb8..6e0e00df27 100644 --- a/samcli/lib/config/file_manager.py +++ b/samcli/lib/config/file_manager.py @@ -13,7 +13,7 @@ from samcli.lib.config.exceptions import FileParseException LOG = logging.getLogger(__name__) - +COMMENT_KEY = "__comment__" class FileManager(ABC): """ @@ -43,7 +43,7 @@ def read(filepath: Path) -> Any: @abstractmethod def write(document: dict, filepath: Path): """ - Write a dictionary to a given file. + Write a dictionary or dictionary-like object to a given file. Parameters ---------- @@ -54,21 +54,6 @@ def write(document: dict, filepath: Path): """ raise NotImplementedError("Write method not implemented.") - @staticmethod - @abstractmethod - def write_document(document: Any, filepath: Path): - """ - Write the contents of a document object to a file at the provided location. - - Parameters - ---------- - document: Any - The object to write. - filepath: Path - The final location for the file to be written. - """ - raise NotImplementedError("Write document method not implemented.") - @staticmethod @abstractmethod def put_comment(document: Any, comment: str) -> Any: @@ -95,6 +80,8 @@ class TomlFileManager(FileManager): Static class to read and write toml files. """ + file_format = "TOML" + @staticmethod def read(filepath: Path) -> Any: """ @@ -116,7 +103,7 @@ def read(filepath: Path) -> Any: txt = filepath.read_text() toml_doc = tomlkit.loads(txt) except OSError as e: - LOG.debug(f"OSError occurred while reading TOML file: {str(e)}") + LOG.debug(f"OSError occurred while reading {TomlFileManager.file_format} file: {str(e)}") except tomlkit.exceptions.TOMLKitError as e: raise FileParseException(e) from e @@ -125,7 +112,7 @@ def read(filepath: Path) -> Any: @staticmethod def write(document: dict, filepath: Path): """ - Write the contents of a dictionary to a TOML file at the provided location. + Write the contents of a dictionary or tomlkit.TOMLDocument to a TOML file at the provided location. Parameters ---------- @@ -135,39 +122,19 @@ def write(document: dict, filepath: Path): The final location for the TOML file to be written. """ if not document: - LOG.debug("No document given for TomlFileManager to write.") + LOG.debug("Nothing for TomlFileManager to write.") return + + document = tomlkit.parse(tomlkit.dumps(document)) # cast from dict-like -> TOMLDocument - doc_no_comments = {k: v for k, v in document.items() if k != "__comment__"} - toml_doc = tomlkit.document() - - if document.get("__comment__", None): # Comment appears at the top of doc - toml_doc.add(tomlkit.comment(document["__comment__"])) - for k, v in doc_no_comments.items(): - toml_doc.add(k, v) - - filepath.write_text(tomlkit.dumps(toml_doc)) - - @staticmethod - def write_document(document: Any, filepath: Path): - """ - Write the contents of a tomlkit.TOMLDocument object to a TOML file at the provided location. - - Parameters - ---------- - document: Any - The object to write. - filepath: Path - The final location for the TOML file to be written. - """ - if not document: - LOG.debug("No TOMLDocument given for TomlFileManager to write.") - return + if document.get(COMMENT_KEY, None): # Remove dunder comments that may be residue from other formats + document.add(tomlkit.comment(document[COMMENT_KEY])) + document.pop(COMMENT_KEY) filepath.write_text(tomlkit.dumps(document)) @staticmethod - def put_comment(document: Any, comment: str) -> Any: + def put_comment(document: dict, comment: str) -> Any: """ Put a comment in a document object. diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index 3e30ba1bef..b8c7ec5d22 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -13,8 +13,8 @@ LOG = logging.getLogger(__name__) -DEFAULT_CONFIG_FILE_EXTENSION = "toml" -DEFAULT_CONFIG_FILE_NAME = f"samconfig.{DEFAULT_CONFIG_FILE_EXTENSION}" +DEFAULT_CONFIG_FILE_EXTENSION = ".toml" +DEFAULT_CONFIG_FILE_NAME = f"samconfig{DEFAULT_CONFIG_FILE_EXTENSION}" DEFAULT_ENV = "default" DEFAULT_GLOBAL_CMDNAME = "global" @@ -25,7 +25,7 @@ class SamConfig: """ FILE_MANAGER_MAPPER: Dict[str, Type[FileManager]] = { - "toml": TomlFileManager, + ".toml": TomlFileManager, } def __init__(self, config_dir, filename=None): @@ -42,11 +42,11 @@ def __init__(self, config_dir, filename=None): """ self.document = {} self.filepath = Path(config_dir, filename or DEFAULT_CONFIG_FILE_NAME) - self.file_manager = self.FILE_MANAGER_MAPPER.get(self.filepath.suffix[1:], None) + self.file_manager = self.FILE_MANAGER_MAPPER.get(self.filepath.suffix, None) if not self.file_manager: LOG.warning( - f"The config file extension '{self.filepath.suffix[1:]}' is not supported. " - f"Supported formats are: [.{'|.'.join(self.FILE_MANAGER_MAPPER.keys())}]" + f"The config file extension '{self.filepath.suffix}' is not supported. " + f"Supported formats are: [{'|'.join(self.FILE_MANAGER_MAPPER.keys())}]" ) raise SamConfigFileReadException( f"The config file {self.filepath} uses an unsupported extension, and cannot be read." @@ -142,7 +142,7 @@ def put_comment(self, comment): A comment to write to the samconfg file """ - self.document = self.file_manager.put_comment(self.document, comment) + self.file_manager.put_comment(self.document, comment) def flush(self): """ @@ -201,7 +201,7 @@ def _write(self): current_version = self._version() if self._version() else SAM_CONFIG_VERSION self.document.update({VERSION_KEY: current_version}) - self.file_manager.write_document(self.document, self.filepath) + self.file_manager.write(self.document, self.filepath) def _version(self): return self.document.get(VERSION_KEY, None) diff --git a/tests/unit/lib/samconfig/test_file_manager.py b/tests/unit/lib/samconfig/test_file_manager.py index ad24522c38..4c9a0ad4b5 100644 --- a/tests/unit/lib/samconfig/test_file_manager.py +++ b/tests/unit/lib/samconfig/test_file_manager.py @@ -5,7 +5,7 @@ import tomlkit from samcli.lib.config.exceptions import FileParseException -from samcli.lib.config.file_manager import TomlFileManager +from samcli.lib.config.file_manager import COMMENT_KEY, TomlFileManager class TestTomlFileManager(TestCase): @@ -38,7 +38,7 @@ def test_write_toml(self): toml = { "version": 0.1, "config_env": {"topic2": {"parameters": {"word": "clarity"}}}, - "__comment__": "This is a comment", + COMMENT_KEY: "This is a comment", } TomlFileManager.write(toml, config_path) @@ -69,7 +69,7 @@ def test_write_toml_file(self): config_path = Path(config_dir, "samconfig.toml") toml = tomlkit.parse('# This is a comment\nversion = 0.1\n[config_env.topic2.parameters]\nword = "clarity"\n') - TomlFileManager.write_document(toml, config_path) + TomlFileManager.write(toml, config_path) txt = config_path.read_text() self.assertIn("version = 0.1", txt) @@ -90,4 +90,12 @@ def test_dont_write_toml_file_if_empty(self): def test_write_toml_file_bad_path(self): config_path = Path("path/to/some", "file_that_doesnt_exist.toml") with self.assertRaises(FileNotFoundError): - TomlFileManager.write_document(tomlkit.parse('key = "some value"'), config_path) + TomlFileManager.write(tomlkit.parse('key = "some value"'), config_path) + + def test_toml_put_comment(self): + toml_doc = tomlkit.loads('version = 0.1\n[config_env.topic2.parameters]\nword = "clarity"\n') + + TomlFileManager.put_comment(toml_doc, "This is a comment") + + txt = tomlkit.dumps(toml_doc) + self.assertIn("# This is a comment", txt) From f2222844dc979f22ed8c483d5dcc4f6b3018fe87 Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Thu, 1 Jun 2023 11:39:32 -0700 Subject: [PATCH 15/16] Format files according to standard --- samcli/lib/config/file_manager.py | 14 ++++++++------ samcli/lib/config/samconfig.py | 2 +- tests/unit/lib/samconfig/test_file_manager.py | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/samcli/lib/config/file_manager.py b/samcli/lib/config/file_manager.py index 6e0e00df27..b5e56022ef 100644 --- a/samcli/lib/config/file_manager.py +++ b/samcli/lib/config/file_manager.py @@ -15,6 +15,7 @@ LOG = logging.getLogger(__name__) COMMENT_KEY = "__comment__" + class FileManager(ABC): """ Abstract class to be overridden by file managers for specific file extensions. @@ -124,14 +125,14 @@ def write(document: dict, filepath: Path): if not document: LOG.debug("Nothing for TomlFileManager to write.") return - - document = tomlkit.parse(tomlkit.dumps(document)) # cast from dict-like -> TOMLDocument - if document.get(COMMENT_KEY, None): # Remove dunder comments that may be residue from other formats - document.add(tomlkit.comment(document[COMMENT_KEY])) - document.pop(COMMENT_KEY) + toml_document = tomlkit.parse(tomlkit.dumps(document)) # cast from dict-like -> TOMLDocument - filepath.write_text(tomlkit.dumps(document)) + if toml_document.get(COMMENT_KEY, None): # Remove dunder comments that may be residue from other formats + toml_document.add(tomlkit.comment(toml_document[COMMENT_KEY])) + toml_document.pop(COMMENT_KEY) + + filepath.write_text(tomlkit.dumps(toml_document)) @staticmethod def put_comment(document: dict, comment: str) -> Any: @@ -150,5 +151,6 @@ def put_comment(document: dict, comment: str) -> Any: Any The new TOMLDocument, with the comment added to it. """ + document = tomlkit.parse(tomlkit.dumps(document)) # cast from dict-like -> TOMLDocument document.add(tomlkit.comment(comment)) return document diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index b8c7ec5d22..d70b8fe1bb 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -142,7 +142,7 @@ def put_comment(self, comment): A comment to write to the samconfg file """ - self.file_manager.put_comment(self.document, comment) + self.document = self.file_manager.put_comment(self.document, comment) def flush(self): """ diff --git a/tests/unit/lib/samconfig/test_file_manager.py b/tests/unit/lib/samconfig/test_file_manager.py index 4c9a0ad4b5..0973637023 100644 --- a/tests/unit/lib/samconfig/test_file_manager.py +++ b/tests/unit/lib/samconfig/test_file_manager.py @@ -95,7 +95,7 @@ def test_write_toml_file_bad_path(self): def test_toml_put_comment(self): toml_doc = tomlkit.loads('version = 0.1\n[config_env.topic2.parameters]\nword = "clarity"\n') - TomlFileManager.put_comment(toml_doc, "This is a comment") + toml_doc = TomlFileManager.put_comment(toml_doc, "This is a comment") txt = tomlkit.dumps(toml_doc) self.assertIn("# This is a comment", txt) From 33bb999dad1b97971548a4d45dbe9b18fce59a18 Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Thu, 1 Jun 2023 14:37:25 -0700 Subject: [PATCH 16/16] Implement helper method for dict-like to TOMLDocument --- samcli/lib/config/file_manager.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/samcli/lib/config/file_manager.py b/samcli/lib/config/file_manager.py index b5e56022ef..146946056f 100644 --- a/samcli/lib/config/file_manager.py +++ b/samcli/lib/config/file_manager.py @@ -126,7 +126,7 @@ def write(document: dict, filepath: Path): LOG.debug("Nothing for TomlFileManager to write.") return - toml_document = tomlkit.parse(tomlkit.dumps(document)) # cast from dict-like -> TOMLDocument + toml_document = TomlFileManager._to_toml(document) if toml_document.get(COMMENT_KEY, None): # Remove dunder comments that may be residue from other formats toml_document.add(tomlkit.comment(toml_document[COMMENT_KEY])) @@ -151,6 +151,11 @@ def put_comment(document: dict, comment: str) -> Any: Any The new TOMLDocument, with the comment added to it. """ - document = tomlkit.parse(tomlkit.dumps(document)) # cast from dict-like -> TOMLDocument + document = TomlFileManager._to_toml(document) document.add(tomlkit.comment(comment)) return document + + @staticmethod + def _to_toml(document: dict) -> tomlkit.TOMLDocument: + """Ensure that a dictionary-like object is a TOMLDocument.""" + return tomlkit.parse(tomlkit.dumps(document))