Skip to content

Commit c55348b

Browse files
authored
Fix all Copilot review suggestions (#1-10) (#387)
Fixes from PR #368 (Batch 4): 1. UpdateInfoEventArgs: make VersionRespDTO nullable to avoid null ref 2. ClientUpdateStrategy: build VersionInfo list from DownloadAssets instead of passing empty list to ProcessInfo constructor 3. ClientUpdateStrategy: use injected _orchestrator if available, fall back to creating one with HttpClient 5. DefaultDownloadOrchestrator: ensure destDir exists (Directory.CreateDirectory) and dispose SemaphoreSlim (using) 6. DownloadPlanBuilder: guard against invalid currentVersion string Fixes from PR #380 (Batch 10): 8. OSSUpdateStrategy: set zip filename to match Decompress() expectations 9. OSSUpdateStrategy: throw clear exception when URL is null/empty 10. OSSUpdateStrategy: set HttpClient.Timeout to 60s to match old behavior Closes #386
1 parent 7e145a9 commit c55348b

5 files changed

Lines changed: 51 additions & 19 deletions

File tree

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ public static class DownloadPlanBuilder
2222
public static DownloadPlan Build(IEnumerable<DownloadAsset> assets, string currentVersion)
2323
{
2424
if (assets == null) return DownloadPlan.Empty;
25+
if (ParseVersion(currentVersion) == null) return DownloadPlan.Empty;
2526

2627
// 1. Filter out frozen packages
2728
var active = assets

src/c#/GeneralUpdate.Core/Download/MultiEventArgs/UpdateInfoEventArgs.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
namespace GeneralUpdate.Core.Download;
55

6-
public class UpdateInfoEventArgs(VersionRespDTO info) : EventArgs
6+
public class UpdateInfoEventArgs(VersionRespDTO? info = null) : EventArgs
77
{
8-
public VersionRespDTO Info { get; private set; } = info;
8+
public VersionRespDTO? Info { get; private set; } = info;
99
}

src/c#/GeneralUpdate.Core/Download/Orchestrators/DefaultDownloadOrchestrator.cs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,11 @@ public async Task<DownloadReport> ExecuteAsync(
4040
if (plan == null || !plan.HasAssets)
4141
return new DownloadReport(Array.Empty<DownloadResult>(), 0, TimeSpan.Zero, 0, 0);
4242

43+
Directory.CreateDirectory(destDir);
44+
4345
var sw = Stopwatch.StartNew();
4446
var results = new List<DownloadResult>();
45-
var sem = new SemaphoreSlim(maxConcurrency);
47+
using var sem = new SemaphoreSlim(maxConcurrency);
4648
long totalBytes = 0;
4749

4850
var tasks = plan.Assets.Select(async asset =>

src/c#/GeneralUpdate.Core/Strategy/ClientUpdateStrategy.cs

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -172,9 +172,19 @@ private async Task ExecuteStandardWorkflowAsync(Encoding encoding, int timeout)
172172
}
173173

174174
// Build process info for the upgrade process
175+
// Convert DownloadAsset list to VersionInfo for ProcessInfo compatibility
176+
var downloadVersions = downloadPlan.Assets.Select(a => new VersionInfo
177+
{
178+
Name = a.Name,
179+
Hash = a.SHA256,
180+
Url = a.Url,
181+
Version = a.Version,
182+
Format = "ZIP"
183+
}).ToList();
184+
175185
_configInfo.ProcessInfo = JsonSerializer.Serialize(
176186
ConfigurationMapper.MapToProcessInfo(
177-
_configInfo, new List<VersionInfo>(),
187+
_configInfo, downloadVersions,
178188
BlackListManager.Instance.BlackFormats.ToList(),
179189
BlackListManager.Instance.BlackFiles.ToList(),
180190
BlackListManager.Instance.SkipDirectorys.ToList()),
@@ -185,17 +195,21 @@ private async Task ExecuteStandardWorkflowAsync(Encoding encoding, int timeout)
185195

186196
_osStrategy!.Create(_configInfo);
187197

188-
// Download via orchestrator (replaces old DownloadManager)
198+
// Download via orchestrator
189199
GeneralTracer.Info($"ClientUpdateStrategy: downloading {downloadPlan.Assets.Count} asset(s).");
190-
var httpClient = new System.Net.Http.HttpClient();
191-
try
200+
if (_orchestrator != null)
192201
{
193-
var orchestrator = new Download.Orchestrators.DefaultDownloadOrchestrator(httpClient);
194-
await orchestrator.ExecuteAsync(downloadPlan, _configInfo.TempPath).ConfigureAwait(false);
202+
await _orchestrator.ExecuteAsync(downloadPlan, _configInfo.TempPath).ConfigureAwait(false);
195203
}
196-
finally
204+
else
197205
{
198-
httpClient.Dispose();
206+
var httpClient = new System.Net.Http.HttpClient();
207+
try
208+
{
209+
var orchestrator = new Download.Orchestrators.DefaultDownloadOrchestrator(httpClient);
210+
await orchestrator.ExecuteAsync(downloadPlan, _configInfo.TempPath).ConfigureAwait(false);
211+
}
212+
finally { httpClient.Dispose(); }
199213
}
200214

201215
await SafeReportDownloadCompletedAsync(hooksCtx).ConfigureAwait(false);

src/c#/GeneralUpdate.Core/Strategy/OSSUpdateStrategy.cs

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -117,17 +117,32 @@ public void StartApp()
117117

118118
private async Task DownloadVersionsAsync(List<VersionOSS> versions)
119119
{
120-
var assets = versions.Select(v => new Download.Models.DownloadAsset(
121-
Name: v.PacketName ?? v.Version ?? "unknown",
122-
Url: v.Url ?? string.Empty,
123-
Size: 0,
124-
SHA256: v.Hash,
125-
Version: v.Version ?? "0.0.0"
126-
)).ToList();
120+
var assets = versions.Select(v =>
121+
{
122+
if (string.IsNullOrWhiteSpace(v.Url))
123+
throw new InvalidOperationException(
124+
$"OSS version '{v.PacketName ?? v.Version}' has no download URL.");
125+
126+
// Use PacketName.zip as the filename to match Decompress() expectations.
127+
// The orchestrator falls back to {Name}.{Version} when URL-based extraction fails,
128+
// so we set Version to "zip" to produce e.g. "myapp.zip.zip".
129+
// Better: set Name to the zip filename directly.
130+
var zipName = $"{v.PacketName ?? v.Version}zip";
131+
if (!zipName.EndsWith(".zip", StringComparison.OrdinalIgnoreCase))
132+
zipName += ".zip";
133+
134+
return new Download.Models.DownloadAsset(
135+
Name: zipName,
136+
Url: v.Url,
137+
Size: 0,
138+
SHA256: v.Hash,
139+
Version: v.Version ?? "0.0.0"
140+
);
141+
}).ToList();
127142

128143
var plan = new Download.Models.DownloadPlan(assets, false);
129144

130-
var httpClient = new System.Net.Http.HttpClient();
145+
var httpClient = new System.Net.Http.HttpClient { Timeout = TimeSpan.FromSeconds(TimeOut) };
131146
try
132147
{
133148
var orchestrator = new Download.Orchestrators.DefaultDownloadOrchestrator(httpClient);

0 commit comments

Comments
 (0)