From 714fd9430b8e75f7ff1fcbb94c59f7a8acf26c82 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 16:26:08 +0000 Subject: [PATCH] fix: make --allow replace an upstream's configured allow-list [patch] The --allow flags were written as Repositories:{index} configuration keys, and configuration merges arrays by index. A flag therefore replaced only the first entries of a list from --config, appsettings.json or the environment: config ["studio/game.git", "studio/tools.git"] plus --allow github=studio/engine.git bound ["studio/engine.git", "studio/tools.git"], silently dropping studio/game.git. The flags are now grouped per upstream and assigned in a post-configure step, so an upstream named by --allow allows exactly the flag patterns. Upstreams no flag names keep their configured list. This matches ktsu.GitLfsCache, and the --allow help text and README now say so. Fixes ktsu-dev/GitBranchStateCache#46 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015LsGhhi4Vq9pNq2fr5T75T --- .../GitBranchStateCache.Tests.csproj | 1 + .../Tool/AllowFlagTests.cs | 107 ++++++++++++++++++ GitBranchStateCache.Tool/Program.cs | 72 +++++++++--- README.md | 2 +- 4 files changed, 166 insertions(+), 16 deletions(-) create mode 100644 GitBranchStateCache.Tests/Tool/AllowFlagTests.cs diff --git a/GitBranchStateCache.Tests/GitBranchStateCache.Tests.csproj b/GitBranchStateCache.Tests/GitBranchStateCache.Tests.csproj index b09d092..1d05fe0 100644 --- a/GitBranchStateCache.Tests/GitBranchStateCache.Tests.csproj +++ b/GitBranchStateCache.Tests/GitBranchStateCache.Tests.csproj @@ -30,5 +30,6 @@ + diff --git a/GitBranchStateCache.Tests/Tool/AllowFlagTests.cs b/GitBranchStateCache.Tests/Tool/AllowFlagTests.cs new file mode 100644 index 0000000..a9c49e0 --- /dev/null +++ b/GitBranchStateCache.Tests/Tool/AllowFlagTests.cs @@ -0,0 +1,107 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitBranchStateCache.Tests.Tool; + +using System.Text; +using ktsu.GitBranchStateCache.Configuration; +using ktsu.GitBranchStateCache.Tool; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; + +[TestClass] +public class AllowFlagTests +{ + private const string ConfigFile = """ + { + "GitBranchStateCache": { + "Upstreams": { + "github": { "BaseUrl": "https://github.com", "Repositories": ["studio/game.git", "studio/tools.git"] }, + "ado": { "BaseUrl": "https://dev.azure.com/org", "Repositories": ["project/_git/game", "other/_git/tools"] } + } + } + } + """; + + /// + /// Binds the options the way the tool does: a configuration file, then the flags over it. + /// + private static GitBranchStateCacheOptions Bind(params string[] allows) + { + Assert.IsTrue( + Program.TryParseAllows(allows, out Dictionary> allowLists, out string? invalid), + invalid); + + using MemoryStream file = new(Encoding.UTF8.GetBytes(ConfigFile)); + IConfiguration configuration = new ConfigurationBuilder().AddJsonStream(file).Build(); + + ServiceCollection services = new(); + services.AddOptions() + .Bind(configuration.GetSection(GitBranchStateCacheOptions.SectionName)); + services.PostConfigure(options => Program.ReplaceAllowLists(options, allowLists)); + + using ServiceProvider provider = services.BuildServiceProvider(); + return provider.GetRequiredService>().Value; + } + + private static void AssertRepositories(GitBranchStateCacheOptions options, string upstream, params string[] expected) => + CollectionAssert.AreEqual(expected, options.Upstreams[upstream].Repositories.ToArray()); + + [TestMethod] + public void Allow_FewerPatternsThanConfigured_ReplacesTheConfiguredList() + { + GitBranchStateCacheOptions options = Bind("github=studio/engine.git"); + + AssertRepositories(options, "github", "studio/engine.git"); + } + + [TestMethod] + public void Allow_AsManyPatternsAsConfigured_ReplacesTheConfiguredList() + { + GitBranchStateCacheOptions options = Bind("github=a/one.git", "github=b/two.git"); + + AssertRepositories(options, "github", "a/one.git", "b/two.git"); + } + + [TestMethod] + public void Allow_Repeated_KeepsEveryPatternInOrder() + { + GitBranchStateCacheOptions options = Bind("github=a/one.git", "GitHub=b/two.git", "github=c/three.git"); + + AssertRepositories(options, "github", "a/one.git", "b/two.git", "c/three.git"); + } + + [TestMethod] + public void Allow_ForOneUpstream_LeavesTheOthersConfiguredList() + { + GitBranchStateCacheOptions options = Bind("github=studio/engine.git"); + + AssertRepositories(options, "ado", "project/_git/game", "other/_git/tools"); + } + + [TestMethod] + public void Allow_ForAnUnconfiguredUpstream_AddsItWithThoseRepositories() + { + GitBranchStateCacheOptions options = Bind("gitlab=team/app.git"); + + AssertRepositories(options, "gitlab", "team/app.git"); + } + + [TestMethod] + public void NoAllow_KeepsTheConfiguredList() + { + GitBranchStateCacheOptions options = Bind(); + + AssertRepositories(options, "github", "studio/game.git", "studio/tools.git"); + } + + [TestMethod] + [DataRow("github")] + [DataRow("=studio/game.git")] + [DataRow("github=")] + public void TryParseAllows_Malformed_ReportsTheEntry(string entry) + { + Assert.IsFalse(Program.TryParseAllows([entry], out _, out string? invalid)); + Assert.AreEqual(entry, invalid); + } +} diff --git a/GitBranchStateCache.Tool/Program.cs b/GitBranchStateCache.Tool/Program.cs index a2c909b..bb5ef6f 100644 --- a/GitBranchStateCache.Tool/Program.cs +++ b/GitBranchStateCache.Tool/Program.cs @@ -4,6 +4,7 @@ namespace ktsu.GitBranchStateCache.Tool; using System.CommandLine; using ktsu.Essentials; +using ktsu.GitBranchStateCache.Configuration; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.HttpOverrides; using Microsoft.Extensions.Configuration; @@ -58,7 +59,7 @@ private static async Task Main(string[] args) Option allow = new("--allow", "-a") { Description = - "A repository this upstream may mirror, as name=pattern, for example github=studio/game.git. Repeatable. Required at least once per upstream, and every pattern must name a literal path segment.", + "A repository this upstream may mirror, as name=pattern, for example github=studio/game.git. Repeatable. Required at least once per upstream, and every pattern must name a literal path segment. Replaces, rather than adds to, the list configuration gives the upstream it names.", AllowMultipleArgumentsPerToken = false, }; @@ -108,14 +109,18 @@ private static async Task Main(string[] args) .ConfigureAwait(false); } - if (!TryApplyAllows(parseResult.GetValue(allow), overrides, out string? invalidAllow)) + if (!TryParseAllows( + parseResult.GetValue(allow), + out Dictionary> allowLists, + out string? invalidAllow)) { return await FailAsync( $"'{invalidAllow}' is not a valid allow entry. Use name=pattern, for example github=studio/game.git.") .ConfigureAwait(false); } - return await RunAsync(overrides, listenPort, configPath, cancellationToken).ConfigureAwait(false); + return await RunAsync(overrides, allowLists, listenPort, configPath, cancellationToken) + .ConfigureAwait(false); }); return await root.Parse(args) @@ -125,6 +130,7 @@ private static async Task Main(string[] args) private static async Task RunAsync( Dictionary overrides, + Dictionary> allowLists, int port, string? configPath, CancellationToken cancellationToken) @@ -150,6 +156,7 @@ private static async Task RunAsync( builder.Configuration["Kestrel:Endpoints:Http:Url"] = $"http://*:{port}"; builder.Services.AddGitBranchStateCache(builder.Configuration); + builder.Services.PostConfigure(options => ReplaceAllowLists(options, allowLists)); // Behind an ingress the request this service sees is not the one the client made. Nothing here // builds a URL from the request, so this exists for the client address in the logs rather than @@ -260,23 +267,19 @@ private static bool TryApplyUpstreams( } /// - /// Binds every --allow flag to configuration. + /// Groups every --allow flag by the upstream it names. /// - /// - /// Indexed per upstream so repeating the flag appends rather than overwrites, which is what a - /// repeatable option has to do to be useful. - /// /// The flag values, or null when the flag was not given. - /// Configuration to add to. + /// The patterns given for each upstream, in the order they were typed. /// The first entry that could not be read, when one could not. /// when every entry was well formed. - private static bool TryApplyAllows( + internal static bool TryParseAllows( string[]? entries, - Dictionary overrides, + out Dictionary> allowLists, out string? invalid) { invalid = null; - Dictionary counts = new(StringComparer.OrdinalIgnoreCase); + allowLists = new(StringComparer.OrdinalIgnoreCase); foreach (string entry in entries ?? []) { @@ -286,12 +289,51 @@ private static bool TryApplyAllows( return false; } - int index = counts.TryGetValue(name, out int used) ? used : 0; - counts[name] = index + 1; + if (!allowLists.TryGetValue(name, out List? patterns)) + { + patterns = []; + allowLists[name] = patterns; + } - overrides[$"GitBranchStateCache:Upstreams:{name}:Repositories:{index}"] = pattern; + patterns.Add(pattern); } return true; } + + /// + /// Makes each upstream named by --allow allow exactly the patterns given for it. + /// + /// + /// Not written as configuration keys like the other flags. Configuration merges arrays by index, + /// so Repositories:0 from the command line would replace only the first entry of a list from + /// a file or the environment and leave the rest in force: a file allowing studio/game.git and + /// studio/tools.git plus --allow github=studio/engine.git would bind + /// studio/engine.git and studio/tools.git, silently dropping studio/game.git. + /// Assigning the list after binding, as a post-configure step, is what makes the flag replace the + /// list rather than patch it. Upstreams no flag names keep the list configuration gave them. This + /// is the same behaviour as ktsu.GitLfsCache, so the two tools read their flags alike. + /// + /// The options as bound from configuration. + /// The patterns given for each upstream. + internal static void ReplaceAllowLists( + GitBranchStateCacheOptions options, + IReadOnlyDictionary> allowLists) + { + foreach ((string name, List patterns) in allowLists) + { + if (!options.Upstreams.TryGetValue(name, out UpstreamOptions? upstream)) + { + upstream = new UpstreamOptions(); + options.Upstreams[name] = upstream; + } + + upstream.Repositories.Clear(); + + foreach (string pattern in patterns) + { + upstream.Repositories.Add(pattern); + } + } + } } diff --git a/README.md b/README.md index ca3308a..715e6fb 100644 --- a/README.md +++ b/README.md @@ -70,7 +70,7 @@ gitbranchstatecache --upstream github=https://github.com --allow github=studio/g --upstream ado=https://dev.azure.com/myorg --allow ado=myproject/_git/game ``` -`--allow` is required at least once per upstream and is also repeatable. Unlike `ktsu.GitLfsCache`, there is no pattern meaning every repository: every pattern must name at least one literal path segment. One request for a repository not on the list would clone a permanent mirror of it onto a shared volume, sized by the repository rather than by the request, that nothing ever evicts. +`--allow` is required at least once per upstream and is also repeatable. For an upstream it names, the flags replace whatever list configuration gives that upstream, rather than adding to it; upstreams no flag names keep their configured list. Unlike `ktsu.GitLfsCache`, there is no pattern meaning every repository: every pattern must name at least one literal path segment. One request for a repository not on the list would clone a permanent mirror of it onto a shared volume, sized by the repository rather than by the request, that nothing ever evicts. Patterns match case insensitively, because forge repository names are, and a pattern that fails only because someone typed `Studio` is a support ticket rather than a control. An allowed repository path is then reduced to lower case before anything is derived from it, so clients that disagree about casing still share one mirror, one fetch and one cached diff, and the volume holds one directory per repository whatever casing was used to ask for it. What is sent to the forge keeps the caller's spelling.