Skip to content

Commit d23c111

Browse files
authored
fix(sources): probe the health-check path under the base URL, not relative to it (#255)
HttpHealthProbe.CombineUrl resolved the configured health-check path as a relative URI. That replaces the base's last path segment whenever the base has no trailing slash (https://api.example.com/v1/orders + "ping" -> https://api.example.com/v1/ping) and discards the base path entirely when the path starts with "/" (-> /ping at the host root). The HTTP and OData health checks therefore probed a URL the author never configured: a reachable source can be reported unhealthy, or an unreachable one healthy if the wrong URL answers. The documented contract is "relative to the source's base URL", i.e. appended under it. Concatenate instead — the same move the Elasticsearch (D64), HubSpot, Airtable and Salesforce packages each made after hitting this independently, each with a comment naming it. This shared helper was the last place still resolving relatively, and the two remaining callers of it are the HTTP and OData health checks. An absolute http(s) health-check path is still honoured as given, and a query on the configured path stays a query rather than being escaped into the path. Covered by HttpHealthProbeUrlTests; 4 of its 8 cases fail against the old implementation.
1 parent 8f6a7ac commit d23c111

4 files changed

Lines changed: 88 additions & 3 deletions

File tree

CHANGELOG.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,13 @@ The `NeoReports.Abstractions` contract follows SemVer strictly.
5858
the host — D20 — this is a nudge, not a behaviour change).
5959

6060
### Fixed
61+
- **A configured health-check path is now probed under the source's base URL.** `HttpHealthProbe`
62+
resolved it as a relative URI, which replaces the base's last path segment when the base has no
63+
trailing slash (`.../v1/orders` + `ping``.../v1/ping`) and discards the base path entirely for a
64+
leading `/`. The HTTP and OData health checks therefore probed a URL the author never configured —
65+
reporting a reachable source unhealthy, or an unreachable one healthy when the wrong URL answered.
66+
It now concatenates, matching what the Elasticsearch, HubSpot, Airtable and Salesforce packages
67+
already did. An absolute `http(s)` health-check path is still used as given.
6168
- **Run-time parameters now work on every job backend.** The run request types its parameter values
6269
as `object?`, so `System.Text.Json` handed each one to the pipeline as a `JsonElement` — which no
6370
ADO provider can bind (*"No mapping exists from object type System.Text.Json.JsonElement"*). Every

docs/STATUS-AND-BACKLOG.md

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,10 @@ rather than decided:
221221
`new Uri(nextUrl)` (absolute-only) on a server-supplied link; RFC 8288 and OData both permit a
222222
relative one. Fails loudly (`UriFormatException`, opaque message) rather than silently. Salesforce
223223
is the only package that resolves this correctly.
224-
- **`HttpHealthProbe.CombineUrl` still has the relative-`Uri` bug — the 5th sighting of this class.**
224+
- ~~**`HttpHealthProbe.CombineUrl` still has the relative-`Uri` bug — the 5th sighting of this class.**~~
225+
**FIXED** — the shared helper now concatenates under the base path (absolute `http(s)` paths still
226+
used as given), matching the four leaf packages; covered by `HttpHealthProbeUrlTests`, verified to
227+
fail against the old implementation. Original description:
225228
`new Uri(baseUri, path)` drops the base's last path segment when it has no trailing slash, and a
226229
leading `/` resets to the host root. Elasticsearch (D64), HubSpot, Airtable and Salesforce each
227230
independently rewrote away from it — with comments naming it — but the shared helper still does it,

src/Sources/NeoReports.Sources.Http.Common/HttpHealthProbe.cs

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,14 +38,46 @@ public static async Task<HttpResponseMessage> SendAsync(HttpClient client, HttpM
3838
return await client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false);
3939
}
4040

41-
/// <summary>Resolves the URL to probe: <paramref name="healthCheckPath"/> relative to <paramref name="baseUrl"/>, or <paramref name="baseUrl"/> itself when unset.</summary>
41+
/// <summary>
42+
/// Resolves the URL to probe: <paramref name="healthCheckPath"/> appended <b>under</b>
43+
/// <paramref name="baseUrl"/>, or <paramref name="baseUrl"/> itself when unset. An absolute
44+
/// http(s) <paramref name="healthCheckPath"/> is used as given.
45+
/// </summary>
46+
/// <remarks>
47+
/// Deliberately concatenates instead of using <c>new Uri(baseUri, relative)</c>: relative-URI
48+
/// resolution replaces the base's last path segment whenever the base has no trailing slash
49+
/// (<c>.../v1/orders</c> + <c>ping</c> → <c>.../v1/ping</c>), and a leading <c>/</c> on the
50+
/// relative part discards the base path entirely (→ <c>/ping</c> at the host root). Either way the
51+
/// probe hits a URL the author never configured, so a reachable source can report unhealthy — or
52+
/// an unreachable one report healthy if the wrong URL happens to answer. The Elasticsearch (D64),
53+
/// HubSpot, Airtable and Salesforce packages each hit this and moved to concatenation; this shared
54+
/// helper is the last place that resolved relatively.
55+
/// </remarks>
4256
public static string CombineUrl(string baseUrl, string? healthCheckPath)
4357
{
4458
if (healthCheckPath is null)
4559
return baseUrl;
4660

4761
var baseUri = new Uri(baseUrl, UriKind.Absolute);
48-
return new Uri(baseUri, healthCheckPath).ToString();
62+
63+
// An absolute probe URL is a deliberate override, not something to append.
64+
if (Uri.TryCreate(healthCheckPath, UriKind.Absolute, out Uri? absolute)
65+
&& (absolute.Scheme == Uri.UriSchemeHttp || absolute.Scheme == Uri.UriSchemeHttps))
66+
{
67+
return absolute.ToString();
68+
}
69+
70+
// A query on the configured path belongs in the query component, not escaped into the path.
71+
var split = healthCheckPath.IndexOf('?', StringComparison.Ordinal);
72+
string relativePath = split < 0 ? healthCheckPath : healthCheckPath[..split];
73+
string relativeQuery = split < 0 ? string.Empty : healthCheckPath[(split + 1)..];
74+
75+
var builder = new UriBuilder(baseUri)
76+
{
77+
Path = baseUri.AbsolutePath.TrimEnd('/') + "/" + relativePath.TrimStart('/'),
78+
Query = relativeQuery,
79+
};
80+
return builder.Uri.ToString();
4981
}
5082

5183
/// <summary>
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
using NeoReports.Sources.Http.Common;
2+
using Shouldly;
3+
using Xunit;
4+
5+
namespace NeoReports.Sources.Http.UnitTests;
6+
7+
/// <summary>
8+
/// The health-check path is documented as being probed <b>relative to the source's base URL</b>, so it
9+
/// must land under the whole base path. Relative-<c>Uri</c> resolution does not do that — it replaces
10+
/// the base's last segment, and drops the base path entirely for a leading <c>/</c> — the bug the
11+
/// Elasticsearch (D64), HubSpot, Airtable and Salesforce packages each hit separately.
12+
/// </summary>
13+
public class HttpHealthProbeUrlTests
14+
{
15+
[Theory]
16+
// Base with a path and no trailing slash: the last segment must survive.
17+
[InlineData("https://api.example.com/v1/orders", "ping", "https://api.example.com/v1/orders/ping")]
18+
// A leading slash on the configured path must not reset to the host root.
19+
[InlineData("https://api.example.com/v1/orders", "/ping", "https://api.example.com/v1/orders/ping")]
20+
// Trailing slash on the base is equivalent.
21+
[InlineData("https://api.example.com/v1/orders/", "ping", "https://api.example.com/v1/orders/ping")]
22+
// Base at the host root still works.
23+
[InlineData("https://api.example.com", "ping", "https://api.example.com/ping")]
24+
// Multi-segment probe paths.
25+
[InlineData("https://api.example.com/v1", "health/live", "https://api.example.com/v1/health/live")]
26+
public void Health_path_is_appended_under_the_whole_base_path(string baseUrl, string path, string expected) =>
27+
HttpHealthProbe.CombineUrl(baseUrl, path).ShouldBe(expected);
28+
29+
[Fact]
30+
public void No_path_probes_the_base_url_itself() =>
31+
HttpHealthProbe.CombineUrl("https://api.example.com/v1/orders", null)
32+
.ShouldBe("https://api.example.com/v1/orders");
33+
34+
[Fact]
35+
public void A_query_on_the_configured_path_stays_a_query() =>
36+
HttpHealthProbe.CombineUrl("https://api.example.com/v1", "health?deep=1")
37+
.ShouldBe("https://api.example.com/v1/health?deep=1");
38+
39+
[Fact]
40+
public void An_absolute_probe_url_is_used_as_given() =>
41+
HttpHealthProbe.CombineUrl("https://api.example.com/v1", "https://status.example.com/health")
42+
.ShouldBe("https://status.example.com/health");
43+
}

0 commit comments

Comments
 (0)