From 8f6f2ba8d66213ec9d0c9850db483e7e1bef9bb9 Mon Sep 17 00:00:00 2001 From: cesnimda Date: Sun, 19 Jul 2026 18:59:26 +0200 Subject: [PATCH] fix(health): report configured application version /health read the APP_VERSION environment variable directly, but docker-compose passes App__Version, which binds to the App:Version configuration key. The variable under that name never existed in the container, so the endpoint always reported "unknown". Read App:Version through IConfiguration, the approach AdminSystemController already used for the same value. The resolution rule (configured version, else assembly version) moves to a shared BuildMetadata helper rather than being written twice; AdminSystemController now calls it, so the admin page and /health cannot drift apart. Local development is unaffected: nothing sets App:Version there, and the assembly-version fallback still applies. Tests pin the configuration KEY, not just the behaviour, including that an App__Version environment variable binds to App:Version. The original bug failed silently, so a behavioural test alone would not have caught it. Verified against a running backend: App__Version=9.9.9-test reports 9.9.9-test; unset reports the assembly version rather than "unknown". Co-Authored-By: Claude Opus 4.8 --- JobTrackerApi.Tests/BuildMetadataTests.cs | 91 +++++++++++++++++++ .../Controllers/AdminSystemController.cs | 23 +---- JobTrackerApi/Program.cs | 7 +- JobTrackerApi/Services/BuildMetadata.cs | 39 ++++++++ deploy/first-production-deployment.md | 7 +- 5 files changed, 144 insertions(+), 23 deletions(-) create mode 100644 JobTrackerApi.Tests/BuildMetadataTests.cs create mode 100644 JobTrackerApi/Services/BuildMetadata.cs diff --git a/JobTrackerApi.Tests/BuildMetadataTests.cs b/JobTrackerApi.Tests/BuildMetadataTests.cs new file mode 100644 index 0000000..441788f --- /dev/null +++ b/JobTrackerApi.Tests/BuildMetadataTests.cs @@ -0,0 +1,91 @@ +using JobTrackerApi.Services; +using Microsoft.Extensions.Configuration; +using Xunit; + +namespace JobTrackerApi.Tests; + +// Pins the configuration KEY, not just the behaviour. /health previously read the +// APP_VERSION environment variable while docker-compose passed App__Version, so the +// endpoint silently reported "unknown" in every container. Nothing failed; the value +// was just wrong. These tests fail if the key drifts again. +public sealed class BuildMetadataTests +{ + private static IConfiguration Config(params (string Key, string Value)[] values) => + new ConfigurationBuilder() + .AddInMemoryCollection(values.Select(v => new KeyValuePair(v.Key, v.Value))) + .Build(); + + [Fact] + public void Reads_the_App_Version_configuration_key() + { + var version = BuildMetadata.ResolveVersion(Config(("App:Version", "1.2.3"))); + + Assert.Equal("1.2.3", version); + } + + [Fact] + public void App__Version_environment_variable_binds_to_the_same_key() + { + // This is the exact path docker-compose uses: App__Version in the container + // environment must reach App:Version in configuration. + var cfg = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary()) + .Add(new FakeEnvironmentSource(("App__Version", "42"))) + .Build(); + + Assert.Equal("42", BuildMetadata.ResolveVersion(cfg)); + } + + [Fact] + public void Falls_back_to_the_assembly_version_when_unconfigured() + { + // Local development sets nothing. "unknown" would be a regression, not a default. + var version = BuildMetadata.ResolveVersion(Config()); + + Assert.False(string.IsNullOrWhiteSpace(version)); + Assert.NotEqual("unknown", version); + } + + [Theory] + [InlineData("$(git rev-parse --short HEAD)")] + [InlineData("${APP_COMMIT_SHA}")] + [InlineData(" ")] + [InlineData(null)] + public void Unresolved_placeholders_and_blanks_are_rejected(string? raw) + { + Assert.Null(BuildMetadata.Normalize(raw)); + } + + [Fact] + public void A_real_value_survives_normalization_trimmed() + { + Assert.Equal("abc1234", BuildMetadata.Normalize(" abc1234 ")); + } + + // Minimal stand-in for the environment-variable provider, so the test does not + // mutate the real process environment and race other tests. + private sealed class FakeEnvironmentSource : IConfigurationSource + { + private readonly (string Key, string Value)[] _values; + + public FakeEnvironmentSource(params (string Key, string Value)[] values) => _values = values; + + public IConfigurationProvider Build(IConfigurationBuilder builder) => new Provider(_values); + + private sealed class Provider : ConfigurationProvider + { + private readonly (string Key, string Value)[] _values; + + public Provider((string Key, string Value)[] values) => _values = values; + + public override void Load() + { + foreach (var (key, value) in _values) + { + // "__" is the environment-variable spelling of the ":" section separator. + Data[key.Replace("__", ":")] = value; + } + } + } + } +} diff --git a/JobTrackerApi/Controllers/AdminSystemController.cs b/JobTrackerApi/Controllers/AdminSystemController.cs index 29cde58..0037350 100644 --- a/JobTrackerApi/Controllers/AdminSystemController.cs +++ b/JobTrackerApi/Controllers/AdminSystemController.cs @@ -1,4 +1,3 @@ -using System.Reflection; using System.Runtime.InteropServices; using JobTrackerApi.Data; using JobTrackerApi.Services; @@ -50,20 +49,8 @@ public sealed class AdminSystemController : ControllerBase AiServiceMetrics Ai ); - private static string? NormalizeBuildMetadata(string? value) - { - var trimmed = (value ?? string.Empty).Trim(); - if (string.IsNullOrWhiteSpace(trimmed)) return null; - - // Ignore unresolved shell/compose placeholders that would otherwise leak - // directly into the admin UI, e.g. $(git rev-parse --short HEAD) or ${APP_COMMIT_SHA}. - if ((trimmed.StartsWith("$(") && trimmed.EndsWith(")")) || (trimmed.StartsWith("${") && trimmed.EndsWith("}"))) - { - return null; - } - - return trimmed; - } + // Shared with the /health endpoint so both report the same version from the same key. + private static string? NormalizeBuildMetadata(string? value) => BuildMetadata.Normalize(value); private EmailSettingsSnapshot BuildFallbackEmailSettingsSnapshot() { @@ -221,11 +208,7 @@ public sealed class AdminSystemController : ControllerBase LastError: ex.Message); } - var version = NormalizeBuildMetadata(_cfg["App:Version"]); - if (string.IsNullOrWhiteSpace(version)) - { - version = Assembly.GetExecutingAssembly().GetName().Version?.ToString() ?? "unknown"; - } + var version = BuildMetadata.ResolveVersion(_cfg); var commitSha = NormalizeBuildMetadata(_cfg["App:CommitSha"]); var buildStamp = NormalizeBuildMetadata(_cfg["App:BuildStamp"]); diff --git a/JobTrackerApi/Program.cs b/JobTrackerApi/Program.cs index dc82a65..a9eaf58 100644 --- a/JobTrackerApi/Program.cs +++ b/JobTrackerApi/Program.cs @@ -525,10 +525,13 @@ app.MapControllers(); // that queried MariaDB would restart a perfectly healthy backend whenever the database blipped, and // would also be a free unauthenticated way to probe database availability. // docs/production-readiness-review.md. -app.MapGet("/health", () => Results.Ok(new +// Version comes from configuration (App:Version), which is how docker-compose supplies it as +// App__Version. Reading the APP_VERSION environment variable directly meant /health always +// reported "unknown" in a container. Falls back to the assembly version for local development. +app.MapGet("/health", (IConfiguration cfg) => Results.Ok(new { status = "ok", - version = Environment.GetEnvironmentVariable("APP_VERSION") ?? "unknown", + version = BuildMetadata.ResolveVersion(cfg), })).AllowAnonymous(); // API schema for tooling/docs. Dev-only: not exposed in production deployments. diff --git a/JobTrackerApi/Services/BuildMetadata.cs b/JobTrackerApi/Services/BuildMetadata.cs new file mode 100644 index 0000000..a2f5a24 --- /dev/null +++ b/JobTrackerApi/Services/BuildMetadata.cs @@ -0,0 +1,39 @@ +using System.Reflection; + +namespace JobTrackerApi.Services; + +// Build metadata (version, commit, stamp) comes from configuration, populated by +// docker-compose as App__Version / App__CommitSha / App__BuildStamp. +// +// This lives here because two callers need the same answer: the admin system page and +// the anonymous /health endpoint. /health used to read the APP_VERSION *environment +// variable* directly, which compose never sets under that name — it passes App__Version, +// binding to the App:Version configuration key. So /health always reported "unknown". +// See docs/release-candidate-review.md (N2). +public static class BuildMetadata +{ + // Ignore unresolved shell/compose placeholders that would otherwise leak into the + // admin UI or a health response, e.g. $(git rev-parse --short HEAD) or ${APP_COMMIT_SHA}. + public static string? Normalize(string? value) + { + var trimmed = (value ?? string.Empty).Trim(); + if (string.IsNullOrWhiteSpace(trimmed)) return null; + + if ((trimmed.StartsWith("$(") && trimmed.EndsWith(")")) || (trimmed.StartsWith("${") && trimmed.EndsWith("}"))) + { + return null; + } + + return trimmed; + } + + // Configured version, else the assembly version, else "unknown". The assembly fallback + // is what makes local development useful: nothing sets App:Version there. + public static string ResolveVersion(IConfiguration cfg) + { + var version = Normalize(cfg["App:Version"]); + if (!string.IsNullOrWhiteSpace(version)) return version; + + return Assembly.GetExecutingAssembly().GetName().Version?.ToString() ?? "unknown"; + } +} diff --git a/deploy/first-production-deployment.md b/deploy/first-production-deployment.md index e4f3e14..8d9494c 100644 --- a/deploy/first-production-deployment.md +++ b/deploy/first-production-deployment.md @@ -144,7 +144,12 @@ upstream cannot be resolved. ```bash # Health endpoint — anonymous, does not touch the database curl -fsS https:///health -# expect: {"status":"ok","version":"..."} +# expect: {"status":"ok","version":""} +# +# The version comes from App:Version, which compose passes as App__Version from APP_VERSION. +# CI sets it to the workflow run number. A version of "1.0.0.0" (the assembly fallback) means +# APP_VERSION did not reach the container — harmless in itself, but it tells you the build +# metadata is not flowing, so the admin system page will be vague about what is deployed. # Auth still enforced (this is the check that proves the API is not open) curl -s -o /dev/null -w '%{http_code}\n' https:///api/jobapplications