Skip to content

Commit 2f69127

Browse files
michaelnebelmichaelnebel
committed
C#: Address review comments.
1 parent c6d573d commit 2f69127

4 files changed

Lines changed: 122 additions & 15 deletions

File tree

‎csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependabotProxy.cs‎

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,12 @@ public class RegistryConfig
2020
/// <summary>
2121
/// The type of the package registry.
2222
/// </summary>
23-
public string Type { get; init; } = "";
23+
public string? Type { get; init; }
2424

2525
/// <summary>
2626
/// The URL of the package registry.
2727
/// </summary>
28-
public string URL { get; init; } = "";
28+
public string? Url { get; init; }
2929

3030
/// <summary>
3131
/// A boolean indicating whether this registry replaces the base registry.
@@ -42,10 +42,22 @@ public class RegistryConfig
4242
private readonly Dictionary<string, bool> registryMapping = [];
4343

4444
private ImmutableHashSet<string>? registryURLs;
45+
/// <summary>
46+
/// Gets the set of registry URLs that have been configured as part of the organization-level
47+
/// private registry configuration. This includes all registries, regardless of whether they replace
48+
/// the default feeds.
49+
/// </summary>
4550
public ImmutableHashSet<string> RegistryURLs =>
4651
registryURLs ??= registryMapping.Keys.ToImmutableHashSet();
4752

4853
private ImmutableHashSet<string>? registryBaseURLs;
54+
/// <summary>
55+
/// Gets the set of registry URLs that have been configured as part of the organization-level
56+
/// private registry configuration and that replace the default registry. This is a subset of
57+
/// <see cref="RegistryURLs"/>.
58+
/// If non-empty, the set should be used as a replacement for the default registry during
59+
/// package resolution.
60+
/// </summary>
4961
public ImmutableHashSet<string> RegistryBaseURLs =>
5062
registryBaseURLs ??= registryMapping.Where(kvp => kvp.Value).Select(kvp => kvp.Key).ToImmutableHashSet();
5163

@@ -83,16 +95,22 @@ private DependabotProxy(IDependabotProxyConfiguration config, ILogger logger, Te
8395
{
8496
foreach (RegistryConfig registry in array)
8597
{
98+
if (string.IsNullOrWhiteSpace(registry.Url))
99+
{
100+
logger.LogDebug("Ignoring registry with empty URL.");
101+
continue;
102+
}
103+
86104
// The array contains all configured private registries, not just ones for C#.
87105
// We ignore the non-C# ones here.
88-
if (!registry.Type.Equals("nuget_feed"))
106+
if (registry.Type is null || !registry.Type.Equals("nuget_feed"))
89107
{
90-
logger.LogDebug($"Ignoring registry at '{registry.URL}' since it is not of type 'nuget_feed'.");
108+
logger.LogDebug($"Ignoring registry at '{registry.Url}' since it is not of type 'nuget_feed'.");
91109
continue;
92110
}
93111

94-
logger.LogInfo($"Found private registry at '{registry.URL}'");
95-
registryMapping.AddOrUpdateToLatest(registry.URL, registry.ReplacesBase);
112+
logger.LogInfo($"Found private registry at '{registry.Url}'");
113+
registryMapping.AddOrUpdateToLatest(registry.Url, registry.ReplacesBase);
96114
}
97115
}
98116
}

‎csharp/extractor/Semmle.Extraction.Tests/DependabotProxy.cs‎

Lines changed: 41 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,12 @@ private static TemporaryDirectory MakeTemporaryDirectory()
2828
return new TemporaryDirectory(tmp, "testing", new LoggerStub());
2929
}
3030

31+
/// <summary>
32+
/// The purpose of this test is to verify that the registry proxy correctly handles the case where the port is not specified.
33+
/// In this case, the registry proxy should not be created.
34+
/// </summary>
3135
[Fact]
32-
public void TestDependabotProxyCreation1()
36+
public void TestDependabotProxyNoPort()
3337
{
3438
// Setup
3539
var config = new DependabotConfigurationStub
@@ -46,8 +50,12 @@ public void TestDependabotProxyCreation1()
4650
Assert.Null(proxy);
4751
}
4852

53+
/// <summary>
54+
/// The purpose of this test is to verify that the registry proxy correctly handles the case where the host is not specified.
55+
/// In this case, the registry proxy should not be created.
56+
/// </summary>
4957
[Fact]
50-
public void TestDependabotProxyCreation2()
58+
public void TestDependabotProxyNoHost()
5159
{
5260
// Setup
5361
var config = new DependabotConfigurationStub
@@ -96,6 +104,10 @@ public void TestDependabotProxyCreation2()
96104
-----END CERTIFICATE-----
97105
""";
98106

107+
/// <summary>
108+
/// The purpose of this test is to verify that the registry proxy correctly handles the case
109+
/// where the port, host, and certificate are specified.
110+
/// </summary>
99111
[Fact]
100112
public void TestDependabotProxyCertificate()
101113
{
@@ -118,8 +130,13 @@ public void TestDependabotProxyCertificate()
118130
Assert.NotNull(proxy.CertificatePath);
119131
}
120132

133+
/// <summary>
134+
/// The purpose of this test is to verify that the registry proxy correctly handles the case
135+
/// where the RegistryURLs environment variable is not a valid JSON list.
136+
/// In this case, the registry proxy should be created, but the list of private registries should be empty.
137+
/// </summary>
121138
[Fact]
122-
public void TestDependabotRegistryUrls1()
139+
public void TestDependabotRegistryUrlsParseError()
123140
{
124141
// Setup
125142
var config = new DependabotConfigurationStub
@@ -139,8 +156,13 @@ public void TestDependabotRegistryUrls1()
139156
Assert.Empty(proxy.RegistryBaseURLs);
140157
}
141158

159+
/// <summary>
160+
/// The purpose of this test is to verify that the registry proxy correctly handles the case
161+
/// where the RegistryURLs environment variable is a valid JSON list with a single entry.
162+
/// In this case, the registry proxy should be created, and the list of private registries should contain the single entry.
163+
/// </summary>
142164
[Fact]
143-
public void TestDependabotRegistryUrls2()
165+
public void TestDependabotRegistryUrlsSingle()
144166
{
145167
// Setup
146168
var config = new DependabotConfigurationStub
@@ -162,6 +184,13 @@ public void TestDependabotRegistryUrls2()
162184
Assert.Empty(proxy.RegistryBaseURLs);
163185
}
164186

187+
/// <summary>
188+
/// The purpose of this test is to verify that the registry proxy correctly handles the case
189+
/// where the RegistryURLs environment variable is a valid JSON list with multiple entries, but only one of them
190+
/// is of type "nuget_feed", which is relevant for C#.
191+
/// In this case, the registry proxy should be created, and the list of private registries should
192+
/// contain only the entry of type "nuget_feed".
193+
/// </summary>
165194
[Fact]
166195
public void TestDependabotRegistryUrls3()
167196
{
@@ -185,8 +214,15 @@ public void TestDependabotRegistryUrls3()
185214
Assert.Empty(proxy.RegistryBaseURLs);
186215
}
187216

217+
/// <summary>
218+
/// The purpose of this test is to verify that the registry proxy correctly handles the case
219+
/// where the RegistryURLs environment variable is a valid JSON list with multiple entries and one of them
220+
/// is configured to replace the base feeds.
221+
/// In this case, the registry proxy should be created, and the list of private registries should contain all
222+
/// entries, while the list of base registries should contain only the entry that replaces the base feeds.
223+
/// </summary>
188224
[Fact]
189-
public void TestDependabotReplacesBase1()
225+
public void TestDependabotRegistryUrlsReplacesBase()
190226
{
191227
// Setup
192228
var config = new DependabotConfigurationStub

‎csharp/extractor/Semmle.Extraction.Tests/FeedManager.cs‎

Lines changed: 56 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,11 @@ public class FileProviderStub : IFileProvider
6868
public ICollection<string> Resources { get; } = new List<string>();
6969
}
7070

71+
/// <summary>
72+
/// The purpose of this test class is to verify the behavior of the FeedManager class.
73+
/// The tests use stub implementations of the FeedManager's dependencies to control the behavior of the FeedManager
74+
/// and verify its behavior.
75+
/// </summary>
7176
public class FeedManagerTests
7277
{
7378
private static FeedManager MakeFeedManager()
@@ -80,6 +85,10 @@ private static FeedManager MakeFeedManager()
8085
return new FeedManager(logger, dotnet, dependabotProxy, fileProvider, feedManagerIo);
8186
}
8287

88+
/// <summary>
89+
/// The purpose of this test is to verify that the FeedManager correctly computes the set of
90+
/// explicit feeds.
91+
/// </summary>
8392
[Fact]
8493
public void TestExplicitFeeds()
8594
{
@@ -97,6 +106,10 @@ public void TestExplicitFeeds()
97106
], actualFeeds);
98107
}
99108

109+
/// <summary>
110+
/// The purpose of this test is to verify that the FeedManager correctly computes the set of
111+
/// inherited feeds.
112+
/// </summary>
100113
[Fact]
101114
public void TestInheritedFeeds()
102115
{
@@ -113,6 +126,10 @@ public void TestInheritedFeeds()
113126
], inherited);
114127
}
115128

129+
/// <summary>
130+
/// The purpose of this test is to verify that the FeedManager correctly computes the set of
131+
/// all feeds.
132+
/// </summary>
116133
[Fact]
117134
public void TestAllFeeds()
118135
{
@@ -132,6 +149,10 @@ public void TestAllFeeds()
132149
], all);
133150
}
134151

152+
/// <summary>
153+
/// The purpose of this test is to verify that the FeedManager correctly computes the set of
154+
/// reachable feeds.
155+
/// </summary>
135156
[Fact]
136157
public void TestReachableFeeds()
137158
{
@@ -149,6 +170,10 @@ public void TestReachableFeeds()
149170
], reachableFeeds);
150171
}
151172

173+
/// <summary>
174+
/// The purpose of this test is to verify that the FeedManager correctly computes the set of
175+
/// reachable explicit feeds.
176+
/// </summary>
152177
[Fact]
153178
public void TestReachableExplicitFeeds()
154179
{
@@ -165,6 +190,10 @@ public void TestReachableExplicitFeeds()
165190
], reachableFeeds);
166191
}
167192

193+
/// <summary>
194+
/// The purpose of this test is to verify that the FeedManager correctly computes the set of
195+
/// reachable fallback feeds.
196+
/// </summary>
168197
[Fact]
169198
public void TestReachableFallbackFeeds()
170199
{
@@ -182,6 +211,10 @@ public void TestReachableFallbackFeeds()
182211
], reachableFallback);
183212
}
184213

214+
/// <summary>
215+
/// The purpose of this test is to verify that the FeedManager correctly computes the set of
216+
/// feeds to use for a given file.
217+
/// </summary>
185218
[Fact]
186219
public void TestFeedsToUse()
187220
{
@@ -198,8 +231,12 @@ public void TestFeedsToUse()
198231
], feedsToUse);
199232
}
200233

234+
/// <summary>
235+
/// The purpose of this test is to verify that the FeedManager correctly computes the set of
236+
/// default feeds and reachable default feeds when no private registries are configured.
237+
/// </summary>
201238
[Fact]
202-
public void TestDefaultFeeds1()
239+
public void TestDefaultFeedsNugetOrg()
203240
{
204241
// Setup
205242
var feedManager = MakeFeedManager();
@@ -217,8 +254,13 @@ public void TestDefaultFeeds1()
217254
], reachableDefault);
218255
}
219256

257+
/// <summary>
258+
/// The purpose of this test is to verify that the FeedManager correctly computes the set of
259+
/// default feeds, reachable default feeds, and fallback feeds when private registries
260+
/// are configured and some of them replace the default feeds.
261+
/// </summary>
220262
[Fact]
221-
public void TestDefaultFeeds2()
263+
public void TestDefaultFeedsPrivateRegistries()
222264
{
223265
// Setup
224266
var logger = new LoggerStub();
@@ -247,8 +289,13 @@ public void TestDefaultFeeds2()
247289
], reachableFallback);
248290
}
249291

292+
/// <summary>
293+
/// The purpose of this test is to verify that the FeedManager correctly computes the set of
294+
/// all feeds when https://api.nuget.org/v3/index.json is not replaced by any private registries because
295+
/// none of them are configured to replace the base feeds.
296+
/// </summary>
250297
[Fact]
251-
public void TestNugetOrg()
298+
public void TestNugetOrgNotReplaced()
252299
{
253300
// Setup
254301
var logger = new LoggerStub();
@@ -274,6 +321,12 @@ public void TestNugetOrg()
274321
], allFeeds);
275322

276323
}
324+
325+
/// <summary>
326+
/// The purpose of this test is to verify that the FeedManager correctly computes the set of
327+
/// all feeds when https://api.nuget.org/v3/index.json and related NuGet.org URLs are replaced by private
328+
/// registries configured to replace the base feeds.
329+
/// </summary>
277330
[Fact]
278331
public void TestNugetOrgReplacement()
279332
{

‎csharp/ql/lib/change-notes/2026-09-03-replaces-base.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
---
22
category: minorAnalysis
33
---
4-
* In `build-mode: none`, private NuGet registries configured with `replaces-base: true` in the organization-level private registry configuration now replace `nuget.org` sources whenever dependencies are downloaded, including sources discovered from NuGet configuration.
4+
* Private NuGet registries for which the "Replaces base" option is enabled in the organization-level private registry configuration now replace `nuget.org` sources whenever dependencies are downloaded, including sources discovered from NuGet configuration.

0 commit comments

Comments
 (0)