feat: improve the error message when app_desc.yaml contains invalid m…

…odules data(both v2 and v3) (#1443)

Co-authored-by: schnee <[email protected]>

apiserver/paasng/paasng/platform/declarative/handlers.py

@@ -17,7 +17,7 @@
 
 import copy
 import logging
-from typing import Dict, Optional, TextIO
+from typing import Dict, Literal, Optional, TextIO
 
 import yaml
 from django.utils.translation import gettext as _
@@ -63,11 +63,9 @@ def detect_spec_version(json_data: Dict) -> AppSpecVersion:
 
 class DescriptionHandler(Protocol):
     @property
-    def app_desc(self) -> ApplicationDesc:
-        ...
+    def app_desc(self) -> ApplicationDesc: ...
 
-    def get_deploy_desc(self, module_name: Optional[str]) -> DeploymentDesc:
-        ...
+    def get_deploy_desc(self, module_name: Optional[str]) -> DeploymentDesc: ...
 
     def handle_app(self, user: User, source_origin: Optional[SourceOrigin] = None) -> Application:
         """Handle a YAML config for application initialization
@@ -117,16 +115,8 @@ def app_desc(self) -> ApplicationDesc:
         return app_desc
 
     def get_deploy_desc(self, module_name: Optional[str]) -> DeploymentDesc:
-        module_desc = self.json_data.get("module")
-        if "modules" in self.json_data:
-            found = next(filter(lambda x: x["name"] == module_name, self.json_data["modules"]), None)
-            if module_desc is not None:
-                logger.warning("Duplicate definition of module information !")
-            module_desc = found
-        if not module_desc:
-            logger.info("Skip running deployment controller because not content was provided")
-            raise DescriptionValidationError({"module": _("内容不能为空")})
-        desc = validate_desc(deploy_spec_v3.DeploymentDescSLZ, module_desc)
+        desc_data = _find_module_desc_data(self.json_data, module_name, "list")
+        desc = validate_desc(deploy_spec_v3.DeploymentDescSLZ, desc_data)
         return desc
 
     def handle_app(self, user: User, source_origin: Optional[SourceOrigin] = None) -> Application:
@@ -181,15 +171,13 @@ def app_desc(self) -> ApplicationDesc:
         return app_desc
 
     def get_deploy_desc(self, module_name: Optional[str] = None) -> DeploymentDesc:
-        module_desc = self.json_data.get("module")
-        if "modules" in self.json_data and module_name in self.json_data["modules"]:
-            if module_desc is not None:
-                logger.warning("Duplicate definition of module information !")
-            module_desc = self.json_data["modules"][module_name]
-        if not module_desc:
-            logger.info("Skip running deployment controller because not content was provided")
-            raise DescriptionValidationError({"module": _("内容不能为空")})
-        desc = validate_desc(deploy_spec_v2.DeploymentDescSLZ, module_desc)
+        """Get the deployment description object by module name.
+
+        :param module_name: The name of module.
+        :raise DescriptionValidationError: If no info can be found using the given module.
+        """
+        desc_data = _find_module_desc_data(self.json_data, module_name, "dict")
+        desc = validate_desc(deploy_spec_v2.DeploymentDescSLZ, desc_data)
         return desc
 
     def handle_app(self, user: User, source_origin: Optional[SourceOrigin] = None) -> Application:
@@ -259,3 +247,54 @@ def handle_deployment(self, deployment: Deployment) -> PerformResult:
         """
         controller = DeploymentDeclarativeController(deployment)
         return controller.perform_action(self.get_deploy_desc(None))
+
+
+def _find_module_desc_data(
+    json_data: Dict,
+    module_name: Optional[str],
+    modules_data_type: Literal["list", "dict"],
+) -> Dict:
+    """Find a module's desc data in the json data. This function can be used in both v2 and v3
+    because them have similar(but slightly different) structure.
+
+    In the `json_data` 2 fields are used to store the module data:
+
+    - "module": contains desc data of the default module.
+    - "modules": contains desc data of multiple modules, use a list(v3) or dict(v2) format.
+
+    :param modules_data_type: The data type that holds the modules data, v2 using dict, v3 using list.
+    """
+    if not module_name:
+        desc_data = json_data.get("module")
+        if not desc_data:
+            raise DescriptionValidationError({"module": _("模块配置内容不能为空")})
+        return desc_data
+
+    # "module_name" is not None, find the desc data in the json data.
+    desc_data = None
+    # When "modules" is provided, find the desc in it's content by module name
+    if modules_data := json_data.get("modules"):
+        # Use different approach for different module data type
+        if modules_data_type == "dict":
+            desc_data = modules_data.get(module_name)
+            existed_modules = ", ".join(modules_data.keys())
+        elif modules_data_type == "list":
+            desc_data = next((m for m in modules_data if m["name"] == module_name), None)
+            existed_modules = ", ".join(m["name"] for m in modules_data)
+        else:
+            raise ValueError("Wrong modules data type")
+
+        # Use the value in the "module" field as a fallback
+        desc_data = desc_data or json_data.get("module")
+        if not desc_data:
+            raise DescriptionValidationError(
+                {"modules": _("未找到 {} 模块的配置，当前已配置模块为 {}").format(module_name, existed_modules)}
+            )
+
+    # The "modules" field is not provided, use the value in the "module" field
+    if not desc_data:
+        desc_data = json_data.get("module")
+
+    if not desc_data:
+        raise DescriptionValidationError({"module": _("模块配置内容不能为空")})
+    return desc_data

apiserver/paasng/tests/paasng/platform/declarative/handlers/v2/test_deployment.py

@@ -23,97 +23,179 @@
 import cattr
 import pytest
 import yaml
-from blue_krill.contextlib import nullcontext as does_not_raise
 
 from paasng.platform.bkapp_model.models import get_svc_disc_as_env_variables
-from paasng.platform.declarative.handlers import AppDescriptionHandler, DescriptionHandler
-from paasng.platform.declarative.handlers import get_desc_handler as _get_desc_handler
+from paasng.platform.declarative.exceptions import DescriptionValidationError
+from paasng.platform.declarative.handlers import (
+    AppDescriptionHandler,
+    CNativeAppDescriptionHandler,
+    SMartDescriptionHandler,
+    get_desc_handler,
+)
 from paasng.platform.engine.configurations.config_var import get_preset_env_variables
 
 pytestmark = pytest.mark.django_db(databases=["default", "workloads"])
 
 
-def get_desc_handler(yaml_content: str) -> DescriptionHandler:
-    handler = _get_desc_handler(yaml.safe_load(yaml_content))
-    assert isinstance(handler, AppDescriptionHandler), "handler is not AppDescriptionHandler"
-    return handler
+@pytest.fixture()
+def yaml_v1_normal() -> str:
+    """A sample YAML content using v1 spec version."""
+    return dedent(
+        """
+        version: 1
+        module:
+            language: python
+            env_variables:
+                - key: BKPAAS_RESERVED_KEY
+                  value: raise error
+    """
+    )
 
 
-class TestAppDescriptionHandler:
+@pytest.fixture()
+def yaml_v2_normal() -> str:
+    """A sample YAML content using v2 spec version."""
+    return dedent(
+        """
+        spec_version: 2
+        module:
+            env_variables:
+                - key: FOO
+                  value: 1
+            language: python
+            svc_discovery:
+                bk_saas:
+                - foo-app
+                - bk_app_code: bar-app
+                  module_name: api
+            bkmonitor:
+                port: 80
+    """
+    )
+
+
+@pytest.fixture()
+def yaml_v3_normal() -> str:
+    """A sample YAML content using v3 spec version."""
+    return dedent(
+        """
+        specVersion: 3
+        module:
+          name: default
+          language: python
+          spec:
+            processes:
+              - name: web
+                replicas: 1
+                procCommand: python manage.py runserver
+    """
+    )
+
+
+class Test__get_desc_handler:
+    """Test cases for `get_desc_handler()` function"""
+
     @pytest.mark.parametrize(
-        ("yaml_content", "ctx"),
+        ("yaml_fixture_name", "expected_type"),
         [
-            (
-                dedent(
-                    """
-                spec_version: 2
-                module:
-                    env_variables:
-                        - key: FOO
-                          value: 1
-                    language: python
-                    svc_discovery:
-                        bk_saas:
-                        - foo-app
-                        - bk_app_code: bar-app
-                          module_name: api
-                    bkmonitor:
-                        port: 80
-                """
-                ),
-                does_not_raise(
-                    {
-                        "env_variables": {"FOO": "1"},
-                        "svc_discovery": {
-                            "BKPAAS_SERVICE_ADDRESSES_BKSAAS": base64.b64encode(
-                                json.dumps(
-                                    [
-                                        {
-                                            "key": {"bk_app_code": "foo-app", "module_name": None},
-                                            "value": {
-                                                "stag": "http://stag-dot-foo-app.bkapps.example.com",
-                                                "prod": "http://foo-app.bkapps.example.com",
-                                            },
-                                        },
-                                        {
-                                            "key": {"bk_app_code": "bar-app", "module_name": "api"},
-                                            "value": {
-                                                "stag": "http://stag-dot-api-dot-bar-app.bkapps.example.com",
-                                                "prod": "http://prod-dot-api-dot-bar-app.bkapps.example.com",
-                                            },
-                                        },
-                                    ]
-                                ).encode()
-                            ).decode()
-                        },
-                        "bk_monitor": {
-                            "port": 80,
-                            "target_port": 80,
-                        },
-                    }
-                ),
-            ),
-            (
-                dedent(
-                    """
-                    version: 1
-                    module:
-                        language: python
-                        env_variables:
-                            - key: BKPAAS_RESERVED_KEY
-                              value: raise error
-                    """
-                ),
-                pytest.raises(AssertionError, match="handler is not AppDescriptionHandler"),
-            ),
+            ("yaml_v1_normal", SMartDescriptionHandler),
+            ("yaml_v2_normal", AppDescriptionHandler),
+            ("yaml_v3_normal", CNativeAppDescriptionHandler),
         ],
     )
-    def test_deployment_normal(self, bk_deployment, yaml_content, ctx):
-        with ctx as expected, mock.patch(
+    def test_v1(self, yaml_fixture_name, expected_type, request):
+        _yaml_content = request.getfixturevalue(yaml_fixture_name)
+        handler = get_desc_handler(yaml.safe_load(_yaml_content))
+        assert isinstance(handler, expected_type)
+
+
+class TestAppDescriptionHandler:
+    def test_normal(self, bk_deployment, yaml_v2_normal):
+        """Handle a normal YAML content."""
+        with mock.patch(
             "paasng.platform.declarative.handlers.DeploymentDeclarativeController.update_bkmonitor"
         ) as update_bkmonitor:
-            get_desc_handler(yaml_content).handle_deployment(bk_deployment)
-            assert get_preset_env_variables(bk_deployment.app_environment) == expected["env_variables"]
-            assert get_svc_disc_as_env_variables(bk_deployment.app_environment) == expected["svc_discovery"]
+            get_desc_handler(yaml.safe_load(yaml_v2_normal)).handle_deployment(bk_deployment)
+
+            assert get_preset_env_variables(bk_deployment.app_environment) == {"FOO": "1"}
+            assert get_svc_disc_as_env_variables(bk_deployment.app_environment) == {
+                "BKPAAS_SERVICE_ADDRESSES_BKSAAS": base64.b64encode(
+                    json.dumps(
+                        [
+                            {
+                                "key": {"bk_app_code": "foo-app", "module_name": None},
+                                "value": {
+                                    "stag": "http://stag-dot-foo-app.bkapps.example.com",
+                                    "prod": "http://foo-app.bkapps.example.com",
+                                },
+                            },
+                            {
+                                "key": {"bk_app_code": "bar-app", "module_name": "api"},
+                                "value": {
+                                    "stag": "http://stag-dot-api-dot-bar-app.bkapps.example.com",
+                                    "prod": "http://prod-dot-api-dot-bar-app.bkapps.example.com",
+                                },
+                            },
+                        ]
+                    ).encode()
+                ).decode()
+            }
+
             assert update_bkmonitor.called
-            assert cattr.unstructure(update_bkmonitor.call_args[0][0]) == expected["bk_monitor"]
+            assert cattr.unstructure(update_bkmonitor.call_args[0][0]) == {"port": 80, "target_port": 80}
+
+    def test_with_modules_found(self, bk_deployment, bk_module):
+        _yaml_content = dedent(
+            f"""\
+            spec_version: 2
+            modules:
+                {bk_module.name}:
+                    language: python
+        """
+        )
+        get_desc_handler(yaml.safe_load(_yaml_content)).handle_deployment(bk_deployment)
+
+    def test_with_modules_not_found(self, bk_deployment):
+        _yaml_content = dedent(
+            """\
+            spec_version: 2
+            modules:
+                wrong-module-name:
+                    language: python
+        """
+        )
+        with pytest.raises(DescriptionValidationError, match="未找到.*当前已配置"):
+            get_desc_handler(yaml.safe_load(_yaml_content)).handle_deployment(bk_deployment)
+
+    def test_with_modules_not_found_fallback_to_module(self, bk_deployment):
+        _yaml_content = dedent(
+            """\
+            spec_version: 2
+            modules:
+                wrong-module-name:
+                    language: python
+            module:
+                language: python
+        """
+        )
+        get_desc_handler(yaml.safe_load(_yaml_content)).handle_deployment(bk_deployment)
+
+    def test_with_module(self, bk_deployment):
+        _yaml_content = dedent(
+            """\
+            spec_version: 2
+            module:
+                language: python
+        """
+        )
+        get_desc_handler(yaml.safe_load(_yaml_content)).handle_deployment(bk_deployment)
+
+    def test_with_module_and_modules_missing(self, bk_deployment):
+        _yaml_content = dedent(
+            """\
+            spec_version: 2
+            nothing: True
+        """
+        )
+        with pytest.raises(DescriptionValidationError, match="配置内容不能为空"):
+            get_desc_handler(yaml.safe_load(_yaml_content)).handle_deployment(bk_deployment)