Skip to content

Commit 0a8c4a2

Browse files
JusterZhuclaude
andcommitted
fix(core): deduplicate switch-to-full logic, add missing mapper, mixed-AppType tests
Code review fixes: - Extract duplicated 'switch to full' code block into local function SwitchToFull - Add missing MaxChainBeforeFallback mapping in ConfigurationMapper - Add 3 mixed-AppType test cases (Client+Upgrade split scenarios) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent a18123b commit 0a8c4a2

3 files changed

Lines changed: 72 additions & 14 deletions

File tree

src/c#/GeneralUpdate.Core/Configuration/ConfigurationMapper.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,7 @@ public static UpdateContext MapToUpdateContext(UpdateRequest source, UpdateConte
103103
target.UpgradeClientVersion = source.UpgradeClientVersion;
104104
target.ProductId = source.ProductId;
105105
target.CustomHeaders = source.CustomHeaders;
106+
target.MaxChainBeforeFallback = source.MaxChainBeforeFallback;
106107

107108
return target;
108109
}

src/c#/GeneralUpdate.Core/Download/DownloadPlanBuilder.cs

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -192,29 +192,26 @@ public static DownloadPlan Build(
192192
.Sum(a => a.Size);
193193
int chainCount = chainCandidates.Count(a => a.AppType == bestFull.AppType);
194194

195-
if (maxChainBeforeFallback > 0 && chainCount > maxChainBeforeFallback)
195+
// Local helper: build a "switch to full" plan that replaces same-AppType
196+
// chain packages with bestFull, while keeping chains for other AppTypes
197+
// (and any same-AppType chains whose version exceeds the full's version).
198+
DownloadPlan SwitchToFull(string reason)
196199
{
197-
GeneralTracer.Info($"DownloadPlanBuilder: chain count {chainCount} exceeds MaxChainBeforeFallback {maxChainBeforeFallback}, switching to full package {bestFull.Name} (chain total {chainTotal}, full size {bestFull.Size})");
198-
var bestFullSv = Lookup(bestFull);
200+
GeneralTracer.Info($"DownloadPlanBuilder: {reason}, switching to full package {bestFull.Name} (chain count {chainCount}, chain total {chainTotal}, full size {bestFull.Size})");
201+
var fullVersion = Lookup(bestFull);
199202
var planAssets = new List<DownloadAsset> { bestFull };
200203
planAssets.AddRange(chainCandidates
201204
.Where(a => a.AppType != bestFull.AppType
202-
|| (Lookup(a) is { } av && bestFullSv != null && av > bestFullSv))
205+
|| (Lookup(a) is { } av && fullVersion != null && av > fullVersion))
203206
.OrderBy(a => Lookup(a)));
204207
return new DownloadPlan(planAssets, isForcibly);
205208
}
206209

210+
if (maxChainBeforeFallback > 0 && chainCount > maxChainBeforeFallback)
211+
return SwitchToFull($"chain count {chainCount} exceeds MaxChainBeforeFallback {maxChainBeforeFallback}");
212+
207213
if (chainTotal >= bestFull.Size)
208-
{
209-
GeneralTracer.Info($"DownloadPlanBuilder: chain total {chainTotal} >= full size {bestFull.Size}, switching to full package {bestFull.Name}");
210-
var bestFullSv = Lookup(bestFull);
211-
var planAssets = new List<DownloadAsset> { bestFull };
212-
planAssets.AddRange(chainCandidates
213-
.Where(a => a.AppType != bestFull.AppType
214-
|| (Lookup(a) is { } av && bestFullSv != null && av > bestFullSv))
215-
.OrderBy(a => Lookup(a)));
216-
return new DownloadPlan(planAssets, isForcibly);
217-
}
214+
return SwitchToFull($"chain total {chainTotal} >= full size {bestFull.Size}");
218215
}
219216

220217
// ── Chain plan with fallback fulls ──

tests/CoreTest/Download/DownloadPlanBuilderTests.cs

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,66 @@ public void Build_ChainSelected_WhenMaxChainBeforeFallbackIsZero()
163163
Assert.All(result.Assets, a => Assert.Equal((int)PackageType.Chain, a.PackageType));
164164
}
165165

166+
[Fact]
167+
public void Build_MixedAppType_FullSwitchKeepsOtherAppTypeChains()
168+
{
169+
// Client full v2.0 + 12 Client chains (trigger count) + 3 Upgrade chains.
170+
// → Client chains replaced by Client full, Upgrade chains kept.
171+
var assets = new List<DownloadAsset>();
172+
for (int i = 1; i <= 12; i++)
173+
assets.Add(AssetWithType($"client-chain{i}", $"1.{i}.0", size: 10, appType: (int)AppType.Client));
174+
for (int i = 1; i <= 3; i++)
175+
assets.Add(AssetWithType($"upgrade-chain{i}", $"1.{i}.0", size: 10, appType: (int)AppType.Upgrade));
176+
assets.Add(AssetWithType("client-full", "2.0.0", size: 500, packageType: (int)PackageType.Full, appType: (int)AppType.Client));
177+
178+
var result = DownloadPlanBuilder.Build(assets, "1.0.0", "1.0.0", maxChainBeforeFallback: 8);
179+
Assert.True(result.HasAssets);
180+
// 1 Client full + 3 Upgrade chains = 4 assets
181+
Assert.Equal(4, result.Assets.Count);
182+
Assert.Contains(result.Assets, a => a.PackageType == (int)PackageType.Full && a.AppType == (int)AppType.Client);
183+
Assert.Equal(3, result.Assets.Count(a => a.AppType == (int)AppType.Upgrade));
184+
}
185+
186+
[Fact]
187+
public void Build_MixedAppType_OnlyFullForUpgrade_DoesNotAffectClientChains()
188+
{
189+
// Upgrade full v2.0 + 3 Upgrade chains (below threshold) + 5 Client chains.
190+
// No Client full → count/size check scoped to Upgrade chains only.
191+
// 3 Upgrade chains ≤ 8, total 30 < 500 → chain plan with Upgrade fallback.
192+
var assets = new List<DownloadAsset>();
193+
for (int i = 1; i <= 5; i++)
194+
assets.Add(AssetWithType($"client-chain{i}", $"1.{i}.0", size: 20, appType: (int)AppType.Client));
195+
for (int i = 1; i <= 3; i++)
196+
assets.Add(AssetWithType($"upgrade-chain{i}", $"1.{i}.0", size: 10, appType: (int)AppType.Upgrade));
197+
assets.Add(AssetWithType("upgrade-full", "2.0.0", size: 500, packageType: (int)PackageType.Full, appType: (int)AppType.Upgrade));
198+
199+
var result = DownloadPlanBuilder.Build(assets, "1.0.0", "1.0.0", maxChainBeforeFallback: 8);
200+
Assert.True(result.HasAssets);
201+
// All 8 chain packages selected, none replaced
202+
Assert.Equal(8, result.Assets.Count);
203+
Assert.All(result.Assets, a => Assert.Equal((int)PackageType.Chain, a.PackageType));
204+
}
205+
206+
[Fact]
207+
public void Build_MixedAppType_CountTriggersForClient_UpgradeChainsUnaffected()
208+
{
209+
// Client full + 12 Client chains (triggers count) + 12 Upgrade chains (no Upgrade full).
210+
// → Client chains replaced by Client full, all 12 Upgrade chains kept.
211+
var assets = new List<DownloadAsset>();
212+
for (int i = 1; i <= 12; i++)
213+
assets.Add(AssetWithType($"client-chain{i}", $"1.{i}.0", size: 5, appType: (int)AppType.Client));
214+
for (int i = 1; i <= 12; i++)
215+
assets.Add(AssetWithType($"upgrade-chain{i}", $"1.{i}.0", size: 5, appType: (int)AppType.Upgrade));
216+
assets.Add(AssetWithType("client-full", "2.0.0", size: 500, packageType: (int)PackageType.Full, appType: (int)AppType.Client));
217+
218+
var result = DownloadPlanBuilder.Build(assets, "1.0.0", "1.0.0", maxChainBeforeFallback: 8);
219+
Assert.True(result.HasAssets);
220+
// 1 Client full + 12 Upgrade chains (no Upgrade full → kept as-is)
221+
Assert.Equal(13, result.Assets.Count);
222+
Assert.Equal(12, result.Assets.Count(a => a.AppType == (int)AppType.Upgrade));
223+
Assert.Single(result.Assets, a => a.PackageType == (int)PackageType.Full);
224+
}
225+
166226
[Fact]
167227
public void Build_MinClientVersionTooHigh_FilteredOut()
168228
{

0 commit comments

Comments
 (0)