Skip to content

Commit dd3941b

Browse files
Blind-StrikerBlind-Striker
committed
refactor: enhance GitHub badge handler with improved exception handling, request validation, and response caching
- Renamed `packageName` to `packageId` across methods and parameters for clarity. - Improved error response handling by introducing specific cases for forbidden and unauthorized access. - Added cache consistency improvements with proper ETag and Last-Modified handling. - Updated GitHubPackageService API integration with enhanced caching and request retries. - Streamlined response generation with cached payload fallback for 304 Not Modified scenarios. - Refactored GitHubOrgSecretsService with null-safe checks for secret mappings.
1 parent 3e81a2a commit dd3941b

11 files changed

Lines changed: 170 additions & 164 deletions

File tree

‎src/BadgeSmith.Api/Domain/Services/Contracts/IGitHubPackageService.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ internal interface IGitHubPackageService
1111
/// Retrieves the latest version information for a GitHub package.
1212
/// </summary>
1313
/// <param name="organization">The GitHub organization name.</param>
14-
/// <param name="packageName">The package name.</param>
14+
/// <param name="packageId">The package name.</param>
1515
/// <param name="token">The GitHub Personal Access Token.</param>
1616
/// <param name="versionRange">Optional version range constraint.</param>
1717
/// <param name="includePrerelease">Whether to include prerelease versions.</param>
@@ -21,7 +21,7 @@ internal interface IGitHubPackageService
2121
/// </returns>
2222
public Task<GitHubPackageResult> GetLatestVersionAsync(
2323
string organization,
24-
string packageName,
24+
string packageId,
2525
string token,
2626
string? versionRange = null,
2727
bool includePrerelease = false,

‎src/BadgeSmith.Api/Domain/Services/GitHub/GitHubOrgSecretsService.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ public async Task<GithubSecretResult> GetGitHubTokenAsync(string organizationNam
6565
return new Error($"Failed to retrieve GitHub token for organization '{orgLower}'");
6666
}
6767

68-
if (getItemResponse.Item.Count == 0)
68+
if (getItemResponse.Item == null || getItemResponse.Item.Count == 0)
6969
{
7070
_logger.LogWarning("No secret mapping found for organization {Organization}", orgLower);
7171
return new SecretNotFound($"No secret mapping found for organization {orgLower}");

‎src/BadgeSmith.Api/Domain/Services/GitHub/GitHubPackageService.cs‎

Lines changed: 66 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -1,140 +1,122 @@
1+
using System.Net;
12
using System.Net.Http.Headers;
23
using System.Text.Json;
4+
using System.Web;
35
using BadgeSmith.Api.Domain.Services.Contracts;
46
using BadgeSmith.Api.Infrastructure.Caching;
57
using BadgeSmith.Api.Json;
68
using Microsoft.Extensions.Logging;
9+
using ZLinq;
710

811
namespace BadgeSmith.Api.Domain.Services.GitHub;
912

1013
internal sealed class GitHubPackageService : IGitHubPackageService
1114
{
12-
private readonly HttpClient _httpClient;
15+
private readonly HttpClient _gitHubClient;
1316
private readonly INuGetVersionService _nuGetVersionService;
1417
private readonly IAppCache _cache;
1518
private readonly ILogger<GitHubPackageService> _logger;
1619

17-
private static readonly TimeSpan CacheTtl = TimeSpan.FromMinutes(5);
20+
private static readonly TimeSpan CacheTtl = TimeSpan.FromMinutes(15);
1821

1922
public GitHubPackageService(
20-
HttpClient httpClient,
23+
HttpClient gitHubClient,
2124
INuGetVersionService nuGetVersionService,
2225
IAppCache cache,
2326
ILogger<GitHubPackageService> logger)
2427
{
25-
_httpClient = httpClient ?? throw new ArgumentNullException(nameof(httpClient));
28+
_gitHubClient = gitHubClient ?? throw new ArgumentNullException(nameof(gitHubClient));
2629
_nuGetVersionService = nuGetVersionService ?? throw new ArgumentNullException(nameof(nuGetVersionService));
2730
_cache = cache ?? throw new ArgumentNullException(nameof(cache));
2831
_logger = logger ?? throw new ArgumentNullException(nameof(logger));
2932
}
3033

3134
public async Task<GitHubPackageResult> GetLatestVersionAsync(
3235
string organization,
33-
string packageName,
36+
string packageId,
3437
string token,
3538
string? versionRange = null,
3639
bool includePrerelease = false,
3740
CancellationToken ct = default)
3841
{
42+
using var activity = BadgeSmithApiActivitySource.ActivitySource.StartActivity($"{nameof(GitHubPackageService)}.{nameof(GetLatestVersionAsync)}");
3943
ArgumentException.ThrowIfNullOrWhiteSpace(organization);
40-
ArgumentException.ThrowIfNullOrWhiteSpace(packageName);
44+
ArgumentException.ThrowIfNullOrWhiteSpace(packageId);
4145
ArgumentException.ThrowIfNullOrEmpty(token);
4246

43-
var orgLower = organization.ToLowerInvariant();
44-
var packageLower = packageName.ToLowerInvariant();
47+
_logger.LogInformation("Fetching GitHub package versions for {PackageId}", packageId);
48+
var orgNormalized = organization.ToLowerInvariant();
49+
var packageIdNormalized = packageId.ToLowerInvariant();
50+
var url = new Uri($"orgs/{HttpUtility.UrlEncode(orgNormalized)}/packages/nuget/{HttpUtility.UrlEncode(packageIdNormalized)}/versions", UriKind.Relative);
51+
var cacheKey = $"github_package:index:{orgNormalized}:{packageIdNormalized}";
52+
var hasCache = _cache.TryGetValue<(string Payload, string? ETag, DateTimeOffset? LastModified)>(cacheKey, out var cached);
4553

46-
var cacheKey = $"github_package:{orgLower}:{packageLower}:{versionRange ?? "latest"}:{includePrerelease}";
47-
48-
// Try cache first
49-
if (_cache.TryGetValue<GitHubPackageInfo>(cacheKey, out var cachedPackage))
50-
{
51-
_logger.LogDebug("Retrieved cached GitHub package info for {Org}/{Package}", orgLower, packageLower);
52-
return cachedPackage;
53-
}
54+
using var request = new HttpRequestMessage(HttpMethod.Get, url);
55+
request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token);
56+
request.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/vnd.github+json"));
5457

55-
try
58+
if (hasCache)
5659
{
57-
// Fetch package versions from GitHub Packages API
58-
var packageVersions = await FetchPackageVersionsAsync(orgLower, packageLower, token, ct).ConfigureAwait(false);
59-
60-
if (packageVersions == null || packageVersions.Count == 0)
60+
if (!string.IsNullOrWhiteSpace(cached.ETag))
6161
{
62-
_logger.LogWarning("No versions found for GitHub package {Org}/{Package}", orgLower, packageLower);
63-
return new PackageNotFound($"Package '{packageLower}' not found in organization '{orgLower}'");
62+
request.Headers.IfNoneMatch.ParseAdd(cached.ETag);
6463
}
6564

66-
// Filter and select the appropriate version
67-
var selectedVersion = SelectVersion(packageVersions, versionRange, includePrerelease);
68-
if (selectedVersion == null)
65+
if (cached.LastModified.HasValue)
6966
{
70-
var criteria = string.IsNullOrWhiteSpace(versionRange) ? "latest" : versionRange;
71-
return new PackageNotFound($"No matching version found for package '{packageLower}' with criteria '{criteria}' (prerelease: {includePrerelease})");
67+
request.Headers.IfModifiedSince = cached.LastModified.Value;
7268
}
73-
74-
var packageInfo = new GitHubPackageInfo(
75-
PackageName: packageLower,
76-
Organization: orgLower,
77-
VersionString: selectedVersion.Name,
78-
IsPrerelease: selectedVersion.Prerelease,
79-
LastModifiedUtc: selectedVersion.UpdatedAt
80-
);
81-
82-
// Cache the result
83-
_cache.Set(cacheKey, packageInfo, CacheTtl);
84-
_logger.LogDebug("Cached GitHub package info for {Org}/{Package} version {Version}", orgLower, packageLower, selectedVersion.Name);
85-
86-
return packageInfo;
87-
}
88-
catch (HttpRequestException ex)
89-
{
90-
_logger.LogError(ex, "HTTP error while fetching GitHub package {Org}/{Package}", orgLower, packageLower);
91-
return new Error($"Failed to fetch package information: {ex.Message}");
92-
}
93-
catch (TaskCanceledException ex) when (ex.InnerException is TimeoutException)
94-
{
95-
_logger.LogError(ex, "Timeout while fetching GitHub package {Org}/{Package}", orgLower, packageLower);
96-
return new Error("Request timeout while fetching package information");
9769
}
98-
catch (Exception ex)
99-
{
100-
_logger.LogError(ex, "Unexpected error while fetching GitHub package {Org}/{Package}", orgLower, packageLower);
101-
return new Error($"An unexpected error occurred: {ex.Message}");
102-
}
103-
}
104-
105-
private async Task<IReadOnlyList<GithubPackageVersion>?> FetchPackageVersionsAsync(string org, string packageName, string token, CancellationToken ct)
106-
{
107-
// GitHub Packages API endpoint for package versions
108-
var url = new Uri($"orgs/{org}/packages/nuget/{packageName}/versions", UriKind.Relative);
109-
110-
using var request = new HttpRequestMessage(HttpMethod.Get, url);
111-
request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token);
112-
request.Headers.UserAgent.Add(new ProductInfoHeaderValue("BadgeSmith", "1.0"));
113-
request.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/vnd.github+json"));
11470

115-
_logger.LogDebug("Fetching GitHub package versions from {Url}", url);
71+
using var response = await _gitHubClient.SendAsync(request, ct).ConfigureAwait(false);
72+
string content;
73+
string? etag;
74+
DateTimeOffset? lastMod;
11675

117-
using var response = await _httpClient.SendAsync(request, ct).ConfigureAwait(false);
118-
119-
if (response.StatusCode == System.Net.HttpStatusCode.NotFound)
76+
switch (response.StatusCode)
12077
{
121-
_logger.LogWarning("GitHub package {Org}/{Package} not found (404)", org, packageName);
122-
return null;
78+
case HttpStatusCode.NotModified when !hasCache:
79+
return new Error("Received 304 Not Modified without a cached entry");
80+
case HttpStatusCode.NotModified when hasCache:
81+
content = cached.Payload;
82+
etag = response.Headers.ETag?.Tag ?? cached.ETag;
83+
lastMod = response.Content.Headers.LastModified ?? response.Headers.Date ?? cached.LastModified;
84+
break;
85+
case HttpStatusCode.NotFound:
86+
return new PackageNotFound($"Package '{packageId}' not found");
87+
case HttpStatusCode.Forbidden:
88+
return new ForbiddenPackageAccess($"GitHub package {orgNormalized}/{packageIdNormalized} access forbidden");
89+
case HttpStatusCode.Unauthorized:
90+
return new UnauthorizedPackageAccess($"GitHub package {orgNormalized}/{packageIdNormalized} not authorized");
91+
default:
92+
if (!response.IsSuccessStatusCode)
93+
{
94+
return new Error($"NuGet API error: {response.StatusCode}");
95+
}
96+
97+
content = await response.Content.ReadAsStringAsync(ct).ConfigureAwait(false);
98+
etag = response.Headers.ETag?.Tag;
99+
lastMod = response.Content.Headers.LastModified ?? response.Headers.Date;
100+
break;
123101
}
124102

125-
if (response.StatusCode == System.Net.HttpStatusCode.Forbidden)
103+
_cache.Set(cacheKey, (content, etag, lastMod), CacheTtl);
104+
var githubPackageVersions = JsonSerializer.Deserialize(content, LambdaFunctionJsonSerializerContext.Default.IReadOnlyListGithubPackageVersion);
105+
106+
if (githubPackageVersions == null || githubPackageVersions.Count == 0)
126107
{
127-
_logger.LogWarning("Access forbidden for GitHub package {Org}/{Package} (403)", org, packageName);
128-
return null;
108+
return new PackageNotFound($"No versions found for package '{packageId}'");
129109
}
130110

131-
response.EnsureSuccessStatusCode();
111+
var versions = githubPackageVersions.AsValueEnumerable().Select(version => version.Name).ToArray();
112+
var nuGetVersionResult = _nuGetVersionService.ParseAndFilterVersions(versions, versionRange, includePrerelease);
132113

133-
var content = await response.Content.ReadAsStringAsync(ct).ConfigureAwait(false);
134-
var versions = JsonSerializer.Deserialize(content, LambdaFunctionJsonSerializerContext.Default.IReadOnlyListGithubPackageVersion);
135-
136-
_logger.LogDebug("Retrieved {Count} versions for GitHub package {Org}/{Package}", versions?.Count ?? 0, org, packageName);
137-
138-
return versions;
114+
return nuGetVersionResult
115+
.Match<GitHubPackageResult>
116+
(
117+
version => new GitHubPackageInfo(packageIdNormalized, orgNormalized, version.ToString(), version.IsPrerelease, lastMod),
118+
range => range,
119+
notfound => new PackageNotFound(notfound.Reason)
120+
);
139121
}
140122
}

‎src/BadgeSmith.Api/Domain/Services/GitHub/GitHubPackageServiceFactory.cs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,10 @@ private static GitHubOrgSecretsService CreateGithubOrgSecretsService()
3636
private static GitHubPackageService CreateGitHubPackageService()
3737
{
3838
var githubClient = HttpClientFactory.CreateGithubClient();
39-
var githubOrgSecretsService = GithubOrgSecretsServiceLazy.Value;
4039
var nuGetVersionService = new NuGetVersionService();
4140
var memoryAppCache = new MemoryAppCache();
4241
var gitHubPackageLogger = LoggerFactory.CreateLogger<GitHubPackageService>();
4342

44-
return new GitHubPackageService(githubClient, githubOrgSecretsService, nuGetVersionService, memoryAppCache, gitHubPackageLogger);
43+
return new GitHubPackageService(githubClient, nuGetVersionService, memoryAppCache, gitHubPackageLogger);
4544
}
4645
}

‎src/BadgeSmith.Api/Domain/Services/GitHub/GitHubResults.cs‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System.Text.Json.Serialization;
2+
using BadgeSmith.Api.Domain.Services.Package;
23
using OneOf;
34

45
namespace BadgeSmith.Api.Domain.Services.GitHub;
@@ -7,14 +8,18 @@ internal record SecretNotFound(string Reason) : NotFoundFailure(Reason);
78

89
internal sealed record PackageNotFound(string Reason) : NotFoundFailure(Reason);
910

11+
internal sealed record UnauthorizedPackageAccess(string Reason) : Error(Reason);
12+
13+
internal sealed record ForbiddenPackageAccess(string Reason) : Error(Reason);
14+
1015
[GenerateOneOf]
1116
internal partial class GithubSecretResult : OneOfBase<string, SecretNotFound, Error>
1217
{
1318
public bool IsSuccess => IsT0 && AsT0 != null;
1419

1520
public bool IsFailure => !IsSuccess;
1621

17-
public string? GithubSecret => AsT0;
22+
public string? GithubSecret => IsT0 ? AsT0 : null;
1823

1924
public OneOf<SecretNotFound, Error> Failure
2025
{
@@ -36,21 +41,24 @@ public OneOf<SecretNotFound, Error> Failure
3641
}
3742

3843
[GenerateOneOf]
39-
internal sealed partial class GitHubPackageResult : OneOfBase<GitHubPackageInfo, PackageNotFound, Error>
44+
internal sealed partial class GitHubPackageResult : OneOfBase<GitHubPackageInfo, PackageNotFound, InvalidVersionRange, UnauthorizedPackageAccess, ForbiddenPackageAccess, Error>
4045
{
4146
public bool IsSuccess => IsT0 && AsT0 != null;
4247
public GitHubPackageInfo? GitHubPackageInfo => IsT0 ? AsT0 : null;
4348

44-
public OneOf<PackageNotFound, Error> Failure => IsT0
49+
public OneOf<PackageNotFound, InvalidVersionRange, UnauthorizedPackageAccess, ForbiddenPackageAccess, Error> Failure => IsT0
4550
? throw new InvalidOperationException("Result is successful")
46-
: Match<OneOf<PackageNotFound, Error>>(
51+
: Match<OneOf<PackageNotFound, InvalidVersionRange, UnauthorizedPackageAccess, ForbiddenPackageAccess, Error>>(
4752
_ => throw new InvalidOperationException("Result is successful"),
4853
notFound => notFound,
54+
range => range,
55+
unAuth => unAuth,
56+
forbidden => forbidden,
4957
error => error
5058
);
5159
}
5260

53-
internal sealed record GitHubPackageInfo(string PackageName, string Organization, string VersionString, bool IsPrerelease, DateTime? LastModifiedUtc);
61+
internal sealed record GitHubPackageInfo(string PackageName, string Organization, string VersionString, bool IsPrerelease, DateTimeOffset? LastModifiedUtc = null);
5462

5563
internal record GithubPackageVersion(
5664
[property: JsonPropertyName("id")] int Id,

‎src/BadgeSmith.Api/Domain/Services/Nuget/NuGetPackageService.cs‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ internal class NuGetPackageService : INuGetPackageService
1717
private readonly HttpClient _nugetClient;
1818
private readonly IAppCache _cache;
1919

20+
private static readonly TimeSpan CacheTtl = TimeSpan.FromMinutes(15);
21+
2022
public NuGetPackageService(INuGetVersionService nuGetVersionService, ILogger<NuGetPackageService> logger, HttpClient nugetClient, IAppCache cache)
2123
{
2224
_nuGetVersionService = nuGetVersionService;
@@ -35,10 +37,9 @@ public async Task<NuGetResult> GetLatestVersionAsync(
3537
ArgumentException.ThrowIfNullOrWhiteSpace(packageId);
3638

3739
_logger.LogInformation("Fetching NuGet package versions for {PackageId}", packageId);
38-
var normalizedPackageId = packageId.ToLowerInvariant();
39-
var url = new Uri($"v3-flatcontainer/{normalizedPackageId}/index.json", UriKind.Relative);
40-
41-
var cacheKey = $"nuget:index:{normalizedPackageId}";
40+
var packageIdNormalized = packageId.ToLowerInvariant();
41+
var url = new Uri($"v3-flatcontainer/{packageIdNormalized}/index.json", UriKind.Relative);
42+
var cacheKey = $"nuget:index:{packageIdNormalized}";
4243
var hasCache = _cache.TryGetValue<(string Payload, string? ETag, DateTimeOffset? LastModified)>(cacheKey, out var cached);
4344

4445
using var request = new HttpRequestMessage(HttpMethod.Get, url);
@@ -85,7 +86,7 @@ public async Task<NuGetResult> GetLatestVersionAsync(
8586
break;
8687
}
8788

88-
_cache.Set(cacheKey, (content, etag, lastMod), TimeSpan.FromMinutes(15));
89+
_cache.Set(cacheKey, (content, etag, lastMod), CacheTtl);
8990

9091
var indexResponse = JsonSerializer.Deserialize(content, LambdaFunctionJsonSerializerContext.Default.NuGetIndexResponse);
9192

‎src/BadgeSmith.Api/Domain/Services/Nuget/NuGetResult.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ internal partial class NuGetResult : OneOfBase<NuGetPackageInfo, PackageNotFound
1515

1616
public bool IsFailure => !IsSuccess;
1717

18-
public NuGetPackageInfo? NuGetPackageInfo => AsT0;
18+
public NuGetPackageInfo? NuGetPackageInfo => IsT0 ? AsT0 : null;
1919

2020
public bool TryGetFailure(out OneOf<NotFoundFailure, ValidationFailure, Error>? failure)
2121
{

‎src/BadgeSmith.Api/Domain/Services/Package/NuGetVersionResult.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ internal partial class NuGetVersionResult : OneOfBase<NuGetVersion, InvalidVersi
1414

1515
public bool IsFailure => !IsSuccess;
1616

17-
public NuGetVersion? NuGetVersion => AsT0;
17+
public NuGetVersion? NuGetVersion => IsT0 ? AsT0 : null;
1818

1919
public OneOf<InvalidVersionRange, LastVersionNotFound> Failure
2020
{

‎src/BadgeSmith.Api/Domain/Services/Package/NuGetVersionService.cs‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -9,19 +9,19 @@ public NuGetVersionResult ParseAndFilterVersions(ReadOnlySpan<string> versionStr
99
{
1010
using var activity = BadgeSmithApiActivitySource.ActivitySource.StartActivity($"{nameof(NuGetVersionService)}.{nameof(ParseAndFilterVersions)}");
1111

12-
if (string.IsNullOrWhiteSpace(versionRange) && versionStrings.Length > 0)
13-
{
14-
for (var i = versionStrings.Length - 1; i >= 0; i--)
15-
{
16-
if (NuGetVersion.TryParse(versionStrings[i], out var version) && (includePrerelease || !version.IsPrerelease))
17-
{
18-
return version;
19-
}
20-
}
21-
22-
return new LastVersionNotFound("The latest version of the package could not be found");
23-
}
24-
12+
// if (string.IsNullOrWhiteSpace(versionRange) && versionStrings.Length > 0)
13+
// {
14+
// for (var i = versionStrings.Length - 1; i >= 0; i--)
15+
// {
16+
// if (NuGetVersion.TryParse(versionStrings[i], out var version) && (includePrerelease || !version.IsPrerelease))
17+
// {
18+
// return version;
19+
// }
20+
// }
21+
//
22+
// return new LastVersionNotFound("The latest version of the package could not be found");
23+
// }
24+
//
2525
VersionRange? range = null;
2626
if (!string.IsNullOrWhiteSpace(versionRange) && !VersionRange.TryParse(versionRange, out range))
2727
{
@@ -54,7 +54,7 @@ public NuGetVersionResult ParseAndFilterVersions(ReadOnlySpan<string> versionStr
5454
}
5555
}
5656

57-
return maxVersion;
57+
return maxVersion == null ? new LastVersionNotFound("The latest version of the package could not be found") : new NuGetVersionResult(maxVersion);
5858
}
5959

6060
private static string BuildCriteriaDescription(string? versionRange, bool includePrerelease)

0 commit comments

Comments
 (0)