chore: Add warning about implicit logical ID usage in sam build

samcli/commands/build/build_context.py

@@ -9,8 +9,11 @@
 from typing import Dict, Optional, List, cast
 
 import click
+
+from samcli.lib.providers.sam_api_provider import SamApiProvider
 from samcli.lib.utils.packagetype import IMAGE
 
+from samcli.commands._utils.template import get_template_data
 from samcli.commands.build.exceptions import InvalidBuildDirException, MissingBuildMethodException
 from samcli.lib.bootstrap.nested_stack.nested_stack_manager import NestedStackManager
 from samcli.lib.build.build_graph import DEFAULT_DEPENDENCIES_DIR
@@ -154,6 +157,12 @@ def get_resources_to_build(self):
 
     def run(self):
         """Runs the building process by creating an ApplicationBuilder."""
+        template_dict = get_template_data(self._template_file)
+        template_transform = template_dict.get("Transform", "")
+        is_sam_template = isinstance(template_transform, str) and template_transform.startswith("AWS::Serverless")
+        if is_sam_template:
+            SamApiProvider.check_implicit_api_resource_ids(self.stacks)
+
         try:
             builder = ApplicationBuilder(
                 self.get_resources_to_build(),

samcli/lib/providers/sam_api_provider.py

@@ -7,6 +7,7 @@
 from samcli.lib.providers.cfn_base_api_provider import CfnBaseApiProvider
 from samcli.commands.validate.lib.exceptions import InvalidSamDocumentException
 from samcli.lib.providers.provider import Stack
+from samcli.lib.utils.colors import Colored
 from samcli.local.apigw.local_apigw_service import Route
 from samcli.lib.utils.resources import AWS_SERVERLESS_FUNCTION, AWS_SERVERLESS_API, AWS_SERVERLESS_HTTPAPI
 
@@ -327,3 +328,20 @@ def _get_route_stack_depth(route: Route) -> int:
         if not route.stack_path:
             return 0
         return route.stack_path.count("/") + 1
+
+    @staticmethod
+    def check_implicit_api_resource_ids(stacks: List[Stack]) -> None:
+        for stack in stacks:
+            for logical_id in stack.resources:
+                if logical_id in (
+                    SamApiProvider.IMPLICIT_API_RESOURCE_ID,
+                    SamApiProvider.IMPLICIT_HTTP_API_RESOURCE_ID,
+                ):
+                    LOG.warning(
+                        Colored().yellow(
+                            'Your template contains a resource with logical ID "%s", '
+                            "which is a reserved logical ID in AWS SAM. "
+                            "It could result in unexpected behaviors and is not recommended."
+                        ),
+                        logical_id,
+                    )

tests/unit/commands/buildcmd/test_build_context.py

@@ -27,6 +27,7 @@ class DeepWrap(Exception):
 
 
 class TestBuildContext__enter__(TestCase):
+    @patch("samcli.commands.build.build_context.get_template_data")
     @patch("samcli.commands.build.build_context.SamLocalStackProvider.get_stacks")
     @patch("samcli.commands.build.build_context.SamFunctionProvider")
     @patch("samcli.commands.build.build_context.SamLayerProvider")
@@ -39,6 +40,7 @@ def test_must_setup_context(
         SamLayerProviderMock,
         SamFunctionProviderMock,
         get_buildable_stacks_mock,
+        get_template_data_mock,
     ):
         template_dict = "template dict"
         stack = Mock()
@@ -108,6 +110,7 @@ def test_must_setup_context(
         ContainerManagerMock.assert_called_once_with(docker_network_id="network", skip_pull_image=True)
         func_provider_mock.get.assert_called_once_with("function_identifier")
 
+    @patch("samcli.commands.build.build_context.get_template_data")
     @patch("samcli.commands.build.build_context.SamLocalStackProvider.get_stacks")
     @patch("samcli.commands.build.build_context.SamFunctionProvider")
     @patch("samcli.commands.build.build_context.SamLayerProvider")
@@ -120,6 +123,7 @@ def test_must_fail_with_illegal_identifier(
         SamLayerProviderMock,
         SamFunctionProviderMock,
         get_buildable_stacks_mock,
+        get_template_data_mock,
     ):
         template_dict = "template dict"
         stack = Mock()
@@ -163,6 +167,7 @@ def test_must_fail_with_illegal_identifier(
         with self.assertRaises(ResourceNotFound):
             context.resources_to_build
 
+    @patch("samcli.commands.build.build_context.get_template_data")
     @patch("samcli.commands.build.build_context.SamLocalStackProvider.get_stacks")
     @patch("samcli.commands.build.build_context.SamFunctionProvider")
     @patch("samcli.commands.build.build_context.SamLayerProvider")
@@ -175,6 +180,7 @@ def test_must_return_only_layer_when_layer_is_build(
         SamLayerProviderMock,
         SamFunctionProviderMock,
         get_buildable_stacks_mock,
+        get_template_data_mock,
     ):
         template_dict = "template dict"
         stack = Mock()
@@ -216,6 +222,7 @@ def test_must_return_only_layer_when_layer_is_build(
         context.__enter__()
         self.assertTrue(layer1 in context.resources_to_build.layers)
 
+    @patch("samcli.commands.build.build_context.get_template_data")
     @patch("samcli.commands.build.build_context.SamLocalStackProvider.get_stacks")
     @patch("samcli.commands.build.build_context.SamFunctionProvider")
     @patch("samcli.commands.build.build_context.SamLayerProvider")
@@ -228,6 +235,7 @@ def test_must_return_buildable_dependent_layer_when_function_is_build(
         SamLayerProviderMock,
         SamFunctionProviderMock,
         get_buildable_stacks_mock,
+        get_template_data_mock,
     ):
         template_dict = "template dict"
         stack = Mock()
@@ -275,6 +283,7 @@ def test_must_return_buildable_dependent_layer_when_function_is_build(
         self.assertTrue(layer2 not in context.resources_to_build.layers)
         self.assertTrue(context.is_building_specific_resource)
 
+    @patch("samcli.commands.build.build_context.get_template_data")
     @patch("samcli.commands.build.build_context.SamLocalStackProvider.get_stacks")
     @patch("samcli.commands.build.build_context.SamFunctionProvider")
     @patch("samcli.commands.build.build_context.SamLayerProvider")
@@ -287,6 +296,7 @@ def test_must_fail_when_layer_is_build_without_buildmethod(
         SamLayerProviderMock,
         SamFunctionProviderMock,
         get_buildable_stacks_mock,
+        get_template_data_mock,
     ):
         template_dict = "template dict"
         stack = Mock()
@@ -329,6 +339,7 @@ def test_must_fail_when_layer_is_build_without_buildmethod(
         with self.assertRaises(MissingBuildMethodException):
             context.resources_to_build
 
+    @patch("samcli.commands.build.build_context.get_template_data")
     @patch("samcli.commands.build.build_context.SamLocalStackProvider.get_stacks")
     @patch("samcli.commands.build.build_context.SamFunctionProvider")
     @patch("samcli.commands.build.build_context.SamLayerProvider")
@@ -341,6 +352,7 @@ def test_must_return_many_functions_to_build(
         SamLayerProviderMock,
         SamFunctionProviderMock,
         get_buildable_stacks_mock,
+        get_template_data_mock,
     ):
         """
         In this unit test, we also verify
@@ -435,6 +447,7 @@ def test_must_return_many_functions_to_build(
 
     @parameterized.expand([(["remote_stack_1", "stack.remote_stack_2"], "print_warning"), ([], False)])
     @patch("samcli.commands.build.build_context.LOG")
+    @patch("samcli.commands.build.build_context.get_template_data")
     @patch("samcli.commands.build.build_context.SamLocalStackProvider.get_stacks")
     @patch("samcli.commands.build.build_context.SamFunctionProvider")
     @patch("samcli.commands.build.build_context.SamLayerProvider")
@@ -449,6 +462,7 @@ def test_must_print_remote_url_warning(
         SamLayerProviderMock,
         SamFunctionProviderMock,
         get_buildable_stacks_mock,
+        get_template_data_mock,
         log_mock,
     ):
         get_buildable_stacks_mock.return_value = ([], remote_stack_full_paths)
@@ -649,6 +663,7 @@ def test_cached_dir_and_deps_dir_creation(
 
 class TestBuildContext_run(TestCase):
     @patch("samcli.commands.build.build_context.SamLocalStackProvider.get_stacks")
+    @patch("samcli.commands.build.build_context.SamApiProvider")
     @patch("samcli.commands.build.build_context.SamFunctionProvider")
     @patch("samcli.commands.build.build_context.SamLayerProvider")
     @patch("samcli.commands.build.build_context.pathlib")
@@ -657,10 +672,12 @@ class TestBuildContext_run(TestCase):
     @patch("samcli.commands.build.build_context.ApplicationBuilder")
     @patch("samcli.commands.build.build_context.BuildContext.get_resources_to_build")
     @patch("samcli.commands.build.build_context.move_template")
+    @patch("samcli.commands.build.build_context.get_template_data")
     @patch("samcli.commands.build.build_context.os")
     def test_run_build_context(
         self,
         os_mock,
+        get_template_data_mock,
         move_template_mock,
         resources_mock,
         ApplicationBuilderMock,
@@ -669,6 +686,7 @@ def test_run_build_context(
         pathlib_mock,
         SamLayerProviderMock,
         SamFunctionProviderMock,
+        SamApiProviderMock,
         get_buildable_stacks_mock,
     ):
 
@@ -787,6 +805,7 @@ def test_run_build_context(
         ]
     )
     @patch("samcli.commands.build.build_context.SamLocalStackProvider.get_stacks")
+    @patch("samcli.commands.build.build_context.SamApiProvider")
     @patch("samcli.commands.build.build_context.SamFunctionProvider")
     @patch("samcli.commands.build.build_context.SamLayerProvider")
     @patch("samcli.commands.build.build_context.pathlib")
@@ -795,12 +814,14 @@ def test_run_build_context(
     @patch("samcli.commands.build.build_context.ApplicationBuilder")
     @patch("samcli.commands.build.build_context.BuildContext.get_resources_to_build")
     @patch("samcli.commands.build.build_context.move_template")
+    @patch("samcli.commands.build.build_context.get_template_data")
     @patch("samcli.commands.build.build_context.os")
     def test_must_catch_known_exceptions(
         self,
         exception,
         wrapped_exception,
         os_mock,
+        get_template_data_mock,
         move_template_mock,
         resources_mock,
         ApplicationBuilderMock,
@@ -809,6 +830,7 @@ def test_must_catch_known_exceptions(
         pathlib_mock,
         SamLayerProviderMock,
         SamFunctionProviderMock,
+        SamApiProviderMock,
         get_buildable_stacks_mock,
     ):
 
@@ -862,6 +884,7 @@ def test_must_catch_known_exceptions(
         self.assertEqual(wrapped_exception, ctx.exception.wrapped_from)
 
     @patch("samcli.commands.build.build_context.SamLocalStackProvider.get_stacks")
+    @patch("samcli.commands.build.build_context.SamApiProvider")
     @patch("samcli.commands.build.build_context.SamFunctionProvider")
     @patch("samcli.commands.build.build_context.SamLayerProvider")
     @patch("samcli.commands.build.build_context.pathlib")
@@ -870,10 +893,12 @@ def test_must_catch_known_exceptions(
     @patch("samcli.commands.build.build_context.ApplicationBuilder")
     @patch("samcli.commands.build.build_context.BuildContext.get_resources_to_build")
     @patch("samcli.commands.build.build_context.move_template")
+    @patch("samcli.commands.build.build_context.get_template_data")
     @patch("samcli.commands.build.build_context.os")
     def test_must_catch_function_not_found_exception(
         self,
         os_mock,
+        get_template_data_mock,
         move_template_mock,
         resources_mock,
         ApplicationBuilderMock,
@@ -882,6 +907,7 @@ def test_must_catch_function_not_found_exception(
         pathlib_mock,
         SamLayerProviderMock,
         SamFunctionProviderMock,
+        SamApiProviderMock,
         get_buildable_stacks_mock,
     ):
         stack = Mock()

tests/unit/commands/local/lib/test_api_provider.py

@@ -239,3 +239,24 @@ def test_apis_in_child_stack_overridden_by_apis_in_parents_within_implicit_or_ex
             (logicalId, [route2]),
         ]
         self.assertEqual(SamApiProvider.merge_routes(collector), [route1])
+
+
+class TestApiProvider_check_implicit_api_resource_ids(TestCase):
+    @patch("samcli.lib.providers.sam_api_provider.LOG.warning")
+    def test_check_implicit_api_resource_ids_false(self, warning_mock):
+        SamApiProvider.check_implicit_api_resource_ids([Mock(resources={"Api1": {"Properties": Mock()}})])
+        warning_mock.assert_not_called()
+
+    @patch("samcli.lib.providers.sam_api_provider.LOG.warning")
+    def test_check_implicit_api_resource_ids_rest_api(self, warning_mock):
+        SamApiProvider.check_implicit_api_resource_ids(
+            [Mock(resources={"Api1": {"Properties": Mock()}, "ServerlessRestApi": {"Properties": Mock()}})]
+        )
+        warning_mock.assert_called_once()
+
+    @patch("samcli.lib.providers.sam_api_provider.LOG.warning")
+    def test_check_implicit_api_resource_ids_http_api(self, warning_mock):
+        SamApiProvider.check_implicit_api_resource_ids(
+            [Mock(resources={"Api1": {"Properties": Mock()}, "ServerlessHttpApi": {"Properties": Mock()}})]
+        )
+        warning_mock.assert_called_once()