fix(sources): probe the health-check path under the base URL, not relative to it - #255
Merged
Merged
Conversation
…ative to it 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.
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
HttpHealthProbe.CombineUrlresolved the configured health-check path as a relative URI. That:https://api.example.com/v1/orders+ping→https://api.example.com/v1/**ping**(ordersgone);/— →https://api.example.com/ping.So the HTTP and OData health checks probed a URL the author never configured: a reachable source can be reported unhealthy, or an unreachable one healthy if the wrong URL happens to answer.
The documented contract is "relative to the source's base URL", i.e. appended under it. This concatenates instead.
Why this one matters beyond its size
This is the 5th independent sighting of the same bug class in this repo. Elasticsearch (D64), HubSpot, Airtable and Salesforce each hit it separately and each rewrote their own URL builder away from
new Uri(base, relative)— every one of them leaving a comment naming the trap. The shared helper was the last place still resolving relatively, and the two remaining callers of it are exactly the two health checks that never got their own builder.Behaviour preserved
http(s)health-check path is still honoured as given.Verification
HttpHealthProbeUrlTests(8 cases: no trailing slash, leading/, trailing slash, host-root base, multi-segment, query, absolute override, unset).Found by a bug hunt over the nine HTTP-family source packages; the rest of that hunt's findings are in
docs/STATUS-AND-BACKLOG.md§6.