fix: Improve config JSON array/object error message and parser (melta…

…no#7344)

src/meltano/core/setting_definition.py

@@ -2,8 +2,10 @@
 
 from __future__ import annotations
 
+import ast
 import json
 import typing as t
+from collections.abc import Mapping, Sequence
 from datetime import date, datetime
 from enum import Enum
 
@@ -59,9 +61,8 @@ def get(self, env) -> str:
         Returns:
             Env var value for given env var.
         """
-        if self.negated:
-            return str(not utils.truthy(env[self.key]))
-        return env[self.key]
+        value = env[self.key]
+        return str(not utils.truthy(value)) if self.negated else value
 
 
 class SettingMissingError(Error):
@@ -144,6 +145,9 @@ class SettingKind(YAMLEnum):
     HIDDEN = "hidden"
 
 
+ParseValueExpectedType = t.TypeVar("ParseValueExpectedType")
+
+
 class SettingDefinition(NameEq, Canonical):
     """Meltano SettingDefinition class."""
 
@@ -353,6 +357,55 @@ def env_vars(
 
         return [EnvVar(key) for key in utils.uniques_in(env_keys)]
 
+    @staticmethod
+    def _parse_value(
+        unparsed: str,
+        expected_type_name: str,
+        expected_type: type[ParseValueExpectedType],
+    ) -> ParseValueExpectedType:
+        """Parse a JSON string.
+
+        Parsing is attempted first with `json.loads`, and then with
+        `ast.literal_eval` as a fallback. It is used as a fallback because it
+        correctly parses most inputs that `json.loads` can parse, but is more
+        liberal about what it accepts. For example, `json.loads` requires
+        double quotes for strings, but `ast.literal_eval` can use either single
+        or double quotes.
+
+        Args:
+            unparsed: The JSON string.
+            expected_type_name: The name of the expected type, e.g. "array".
+                Used in the error message if parsing fails or the type is not
+                as expected.
+            expected_type: The Python type class of the expected type. Used to
+                ensure that the parsed value is of the expected type.
+
+        Raises:
+            parse_error: Parsing failed, or the parsed value had an unexpected type.
+
+        Returns:
+            The parsed value.
+        """
+        parse_error = ValueError(
+            f"Failed to parse JSON {expected_type_name} from string: {unparsed!r}"
+        )
+        try:
+            parsed = json.loads(unparsed)
+        except json.JSONDecodeError:
+            try:  # noqa: WPS505
+                parsed = ast.literal_eval(unparsed)
+            except (
+                ValueError,
+                TypeError,
+                SyntaxError,
+                MemoryError,
+                RecursionError,
+            ) as ex:
+                raise parse_error from ex
+        if not isinstance(parsed, expected_type):
+            raise parse_error
+        return parsed
+
     def cast_value(self, value: t.Any) -> t.Any:  # noqa: C901
         """Cast given value.
 
@@ -361,9 +414,6 @@ def cast_value(self, value: t.Any) -> t.Any:  # noqa: C901
 
         Returns:
             Value cast according to specified setting definition kind.
-
-        Raises:
-            ValueError: if value cannot be cast to setting kind.
         """
         value = value.isoformat() if isinstance(value, (date, datetime)) else value
 
@@ -373,13 +423,13 @@ def cast_value(self, value: t.Any) -> t.Any:  # noqa: C901
             elif self.kind == SettingKind.INTEGER:
                 return int(value)
             elif self.kind == SettingKind.OBJECT:
-                value = json.loads(value)
-                if not isinstance(value, dict):
-                    raise ValueError(f"JSON value '{value}' is not an object")
+                value = dict(
+                    self._parse_value(value, "object", Mapping),  # type: ignore
+                )
             elif self.kind == SettingKind.ARRAY:
-                value = json.loads(value)
-                if not isinstance(value, list):
-                    raise ValueError(f"JSON value '{value}' is not an array")
+                value = list(
+                    self._parse_value(value, "array", Sequence),  # type: ignore
+                )
 
         processor = self.value_processor
         if value is not None and processor:

tests/meltano/core/test_setting_definition.py

@@ -0,0 +1,57 @@
+from __future__ import annotations
+
+import pytest
+
+from meltano.core.setting_definition import SettingDefinition, SettingKind
+
+
+class TestSettingDefinition:
+    @pytest.mark.parametrize(
+        ("setting_definition", "uncast_expected_pairs"),
+        (
+            (
+                SettingDefinition("test_setting", kind=SettingKind.ARRAY),
+                (
+                    ('["abc", "xyz"]', ["abc", "xyz"]),
+                    ('["abc", "xyz",]', ["abc", "xyz"]),
+                    ("['abc', 'xyz']", ["abc", "xyz"]),
+                    ("[abc, xyz]", ValueError),
+                    ("'abc', 'xyz'", ["abc", "xyz"]),
+                    ("'abc', 'xyz',", ["abc", "xyz"]),
+                    ('[null, "xyz"]', [None, "xyz"]),
+                    ("{'abc', 'xyz'}", ValueError),
+                    ("1234", ValueError),
+                    ("[1234, 5678]", [1234, 5678]),
+                ),
+            ),
+            (
+                SettingDefinition("test_setting", kind=SettingKind.OBJECT),
+                (
+                    (
+                        '{"key_1": "value_1", "key_2": "value_2"}',
+                        {"key_1": "value_1", "key_2": "value_2"},
+                    ),
+                    ('{"key": "value",}', {"key": "value"}),
+                    ('{"key": null}', {"key": None}),
+                    ("{'key': null}", ValueError),
+                    ("key: value", ValueError),
+                    ("{'key': 'value'}", {"key": "value"}),
+                    ("{key: value}", ValueError),
+                    ("{'abc', 'xyz'}", ValueError),
+                    ("1234", ValueError),
+                ),
+            ),
+        ),
+        ids=("array", "object"),
+    )
+    def test_cast_value_array(
+        self,
+        setting_definition: SettingDefinition,
+        uncast_expected_pairs,
+    ):
+        for uncast, expected in uncast_expected_pairs:
+            if isinstance(expected, type) and issubclass(expected, Exception):
+                with pytest.raises(expected):
+                    setting_definition.cast_value(uncast)
+            else:
+                assert setting_definition.cast_value(uncast) == expected