-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: raise on ambiguous resource ID collision across sibling nested stacks #9186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 1 commit
5ed83e7
1ac9443
4dce905
383d8f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,9 +11,10 @@ | |
| from dataclasses import dataclass | ||
| from enum import Enum | ||
| from pathlib import Path | ||
| from typing import Any, Dict, Iterator, List, NamedTuple, Optional, Set, Union, cast | ||
| from typing import Any, Dict, Iterator, List, NamedTuple, Optional, Set, Tuple, Union, cast | ||
|
|
||
| from samcli.commands.local.cli_common.user_exceptions import ( | ||
| AmbiguousResourceIdentifier, | ||
| InvalidFunctionPropertyType, | ||
| InvalidLayerVersionArn, | ||
| UnsupportedIntrinsic, | ||
|
|
@@ -883,8 +884,19 @@ def get_resource_by_id( | |
| ------- | ||
| Dict | ||
| Resource dict | ||
|
|
||
| Raises | ||
| ------ | ||
| AmbiguousResourceIdentifier | ||
| If a bare logical ID (no stack path given) matches resources in more than one *nested* | ||
| stack, with no match in the root stack to prefer. The root stack, if it has a match, always | ||
| takes priority (this is relied upon by existing behavior); but silently picking the first | ||
| nested-stack match in stack-list order when there is no root match - and more than one | ||
| nested stack collides - could operate on the wrong physical resource (e.g. | ||
| `sam sync --resource-id`), so that specific case is now a clear, actionable error instead. | ||
| """ | ||
| search_all_stacks = not identifier.stack_path and not explicit_nested | ||
| matches: List[Tuple[Stack, Dict[str, Any]]] = [] | ||
| for stack in stacks: | ||
| if stack.stack_path == identifier.stack_path or search_all_stacks: | ||
| found_resource = None | ||
|
|
@@ -897,8 +909,22 @@ def get_resource_by_id( | |
| break | ||
|
|
||
| if found_resource: | ||
| return cast(Dict[str, Any], found_resource) | ||
| return None | ||
| if not stack.stack_path: | ||
| # The root stack always takes priority over nested stacks. | ||
| return cast(Dict[str, Any], found_resource) | ||
| matches.append((stack, found_resource)) | ||
| if not search_all_stacks: | ||
| break | ||
|
|
||
| if len(matches) > 1: | ||
| colliding_paths = [get_full_path(stack.stack_path, identifier.resource_iac_id) for stack, _ in matches] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [BUG] The remediation path in the ambiguity error is built from resource_id = ResourceMetadataNormalizer.get_resource_id(resource, logical_id)
if resource_id == identifier.resource_iac_id or (
not identifier.stack_path and logical_id == identifier.resource_iac_id
):When the match comes from the second branch (CDK / Concrete effect: a user with two sibling nested stacks each holding a CDK resource with logical ID Carry the matched ID alongside the resource so the message names an identifier that actually resolves: matches: List[Tuple[Stack, str, Dict[str, Any]]] = []
for stack in stacks:
if stack.stack_path == identifier.stack_path or search_all_stacks:
found_resource = None
found_resource_id = None
for logical_id, resource in stack.resources.items():
resource_id = ResourceMetadataNormalizer.get_resource_id(resource, logical_id)
if resource_id == identifier.resource_iac_id or (
not identifier.stack_path and logical_id == identifier.resource_iac_id
):
found_resource = resource
found_resource_id = resource_id
break
...
matches.append((stack, cast(str, found_resource_id), found_resource))
if len(matches) > 1:
colliding_paths = [get_full_path(stack.stack_path, resource_id) for stack, resource_id, in matches]A regression test covering a resource carrying |
||
| raise AmbiguousResourceIdentifier( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [GENERAL] The same "bare ID matches resources in more than one stack" condition is already handled elsewhere in the codebase with the opposite strategy — warn and pick deterministically ( # If multiple functions are found, only return one of them
if len(found_fs) > 1:
found_fs.sort(key=lambda f0: f0.full_path.lower())
message = (
f"Multiple functions found with keyword {name}! Function {found_fs[0].full_path} will be "
f"invoked! If it's not the function you are going to invoke, please choose one of them from below:"
)
LOG.warning(Colored().yellow(message))
resolved_function = found_fs[0]After this change the two resolvers disagree on the same user input: Either align the two (reuse the warn-and-pick behavior, or extend the ambiguity error to |
||
| f"Resource ID '{identifier.resource_iac_id}' is ambiguous: it matches resources in more than one " | ||
| f"nested stack ({', '.join(colliding_paths)}). Qualify it with the full stack path, " | ||
| f"e.g. '{colliding_paths[0]}'." | ||
| ) | ||
|
|
||
| return cast(Dict[str, Any], matches[0][1]) if matches else None | ||
|
|
||
|
|
||
| def get_resource_full_path_by_id(stacks: List[Stack], identifier: ResourceIdentifier) -> Optional[str]: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[BUG] The remediation path in the error message is built from
identifier.resource_iac_id, but a match can be found via either the normalized resource ID or the raw logical ID:The second branch is only reachable when
identifier.stack_pathis empty — which is exactly the ambiguous case this code raises on. So if the user passed a raw logical ID that differs from the normalized ID (CDK apps, or any template withSamResourceId/aws:cdk:pathmetadata — seeResourceMetadataNormalizer.get_resource_id), the message tells them to retry with e.g.NestedStackA/Function1ABC123. That qualified form can never resolve, because oncestack_pathis non-empty onlyresource_id == identifier.resource_iac_idis compared — the retry returnsNoneand surfaces as "resource not found". The user is sent to a dead end.Capture the matched resource ID and build the paths from it, the way
get_resource_full_path_by_idalready does (it returnsget_full_path(stack.stack_path, resource_id)). This also makes the listed paths match the canonical full paths used byget_all_resource_ids: