Skip to content

Handle duplicate Identity in SWA dictionary construction #53328

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.

Sign up for GitHub

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

Closed
lewing wants to merge 1 commit into from
Closed

Handle duplicate Identity in SWA dictionary construction #53328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/StaticWebAssetsSdk/Tasks/Compression/DiscoverPrecompressedAssets.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,16 @@ public override bool Execute()
var candidates = StaticWebAsset.FromTaskItemGroup(CandidateAssets);
var assetsToUpdate = new List<ITaskItem>();

var candidatesByIdentity = candidates.ToDictionary(asset => asset.Identity, OSPath.PathComparer);
// Build lookup dictionary, handling duplicate Identities from multiple projects
// referencing the same physical file (e.g. NuGet cache assets shared across WASM clients).
var candidatesByIdentity = new Dictionary<string, StaticWebAsset>(candidates.Length, OSPath.PathComparer);
foreach (var candidate in candidates)
{
if (!candidatesByIdentity.ContainsKey(candidate.Identity))
{
candidatesByIdentity.Add(candidate.Identity, candidate);
}
}
Comment on lines +37 to +44

Copilot AI Mar 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The duplicate-Identity guard uses ContainsKey(...) + Add(...), which performs two lookups per candidate. Prefer a single lookup with TryGetValue to keep this linear scan as cheap as possible (especially since this runs per build).

Copilot uses AI. Check for mistakes.

foreach (var candidate in candidates)
{
Expand Down
8 changes: 7 additions & 1 deletion src/StaticWebAssetsSdk/Tasks/Data/StaticWebAsset.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1234,7 +1234,13 @@ internal static Dictionary<string, StaticWebAsset> ToAssetDictionary(ITaskItem[]
for (var i = 0; i < candidateAssets.Length; i++)
{
var candidateAsset = FromTaskItem(candidateAssets[i], validate);
dictionary.Add(candidateAsset.Identity, candidateAsset);
// Multiple projects may reference the same physical file (e.g. NuGet cache assets
// shared across WASM clients). Since Identity is the FullPath, same Identity means
// same file on disk — keeping the first occurrence is correct.
if (!dictionary.ContainsKey(candidateAsset.Identity))
{
dictionary.Add(candidateAsset.Identity, candidateAsset);
}
Comment on lines +1237 to +1243

Copilot AI Mar 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change alters behavior in multiple tasks to tolerate duplicate Identity values, but there are existing unit tests for these tasks (e.g., GenerateStaticWebAssetEndpointsManifestTest, DiscoverPrecompressedAssetsTest, GenerateStaticWebAssetsManifestTest) that don't currently exercise the duplicate-identity scenario. Adding targeted tests that include two assets with the same Identity (and differing BasePath/TargetPath where applicable) would help prevent regressions and ensure the intended behavior is correct.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +1237 to +1243

Copilot AI Mar 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ToAssetDictionary now silently drops duplicate Identity entries. Even if the physical file is the same, different projects can legitimately attach different static-web-asset metadata (e.g., BasePath/RelativePath/AssetKind/CopyToPublishDirectory), and keeping the first one can produce incorrect downstream behavior. Consider validating that duplicates are equivalent (and error/warn if not), or changing the data structure to preserve all items (e.g., Identity -> list) for callers that need per-asset metadata.

Copilot uses AI. Check for mistakes.
}

return dictionary;
Expand Down
13 changes: 11 additions & 2 deletions src/StaticWebAssetsSdk/Tasks/GenerateStaticWebAssetEndpointsManifest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,17 @@ public override bool Execute()

// Get the list of the asset that need to be part of the manifest (this is similar to GenerateStaticWebAssetsDevelopmentManifest)
var assets = StaticWebAsset.FromTaskItemGroup(Assets);
var manifestAssets = ComputeManifestAssets(assets, ManifestType)
.ToDictionary(a => a.ResolvedAsset.Identity, a => a, OSPath.PathComparer);
// Build dictionary handling duplicate Identities from multiple projects
// referencing the same physical file (e.g. NuGet cache assets shared across WASM clients).
var manifestAssetsList = ComputeManifestAssets(assets, ManifestType);
var manifestAssets = new Dictionary<string, TargetPathAssetPair>(OSPath.PathComparer);
foreach (var a in manifestAssetsList)
{
if (!manifestAssets.ContainsKey(a.ResolvedAsset.Identity))
{
manifestAssets.Add(a.ResolvedAsset.Identity, a);
}
}
Comment on lines +69 to +77

Copilot AI Mar 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Building the lookup keyed only by ResolvedAsset.Identity and then skipping duplicates can select an arbitrary TargetPathAssetPair when the same physical file is included multiple times with different TargetPath/BasePath (e.g., multiple WASM clients). That can cause endpoints for one base path to be rewritten to a different target path. Consider keying by TargetPath (or Identity -> list) and selecting the matching entry per endpoint (e.g., by matching endpoint.Route/target path), or emitting an error when the same Identity maps to multiple distinct TargetPaths.

Copilot uses AI. Check for mistakes.

// Build exclusion matcher if patterns are provided
StaticWebAssetGlobMatcher exclusionMatcher = null;
Expand Down
11 changes: 10 additions & 1 deletion src/StaticWebAssetsSdk/Tasks/GenerateStaticWebAssetsManifest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,16 @@ private StaticWebAssetEndpoint[] FilterPublishEndpointsIfNeeded(StaticWebAsset[]
// inside the manifest because its cumbersome to do it in MSBuild directly.
if (StaticWebAssetsManifest.ManifestTypes.IsPublish(ManifestType))
{
var assetsByIdentity = assets.ToDictionary(a => a.Identity, a => a, OSPath.PathComparer);
// Build dictionary handling duplicate Identities from multiple projects
// referencing the same physical file (e.g. NuGet cache assets shared across WASM clients).
var assetsByIdentity = new Dictionary<string, StaticWebAsset>(assets.Length, OSPath.PathComparer);
foreach (var a in assets)
{
if (!assetsByIdentity.ContainsKey(a.Identity))
{
assetsByIdentity.Add(a.Identity, a);
}
}
Comment on lines +104 to +111

Copilot AI Mar 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new duplicate-Identity handling uses ContainsKey(...) followed by Add(...), which does two dictionary lookups per item. Since TryAdd isn't available on all TFMs, prefer a single lookup via TryGetValue (or attempt Add in a try/catch if you want to keep the fast path) to avoid unnecessary overhead on large asset lists.

Copilot uses AI. Check for mistakes.
var filteredEndpoints = new List<StaticWebAssetEndpoint>();

foreach (var endpoint in Endpoints.Select(StaticWebAssetEndpoint.FromTaskItem))
Expand Down
Loading