-
Notifications
You must be signed in to change notification settings - Fork 69
fix path concatenation for non-root base endpoints, added test to check #1603
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
fix path concatenation for non-root base endpoints, added test to check #1603
Conversation
|
|
||
| this.metrics = normalizeAndJoinPaths(location, overrides.metrics); | ||
| this.logs = normalizeAndJoinPaths(location, overrides.logs); | ||
| } | ||
|
|
||
| private URI normalizeAndJoinPaths(URI base, String path) { | ||
| String basePath = base.getPath(); | ||
| if (basePath == null) { | ||
| basePath = "/"; | ||
| } | ||
|
|
||
| String pathToAppend = path; | ||
| if (pathToAppend.startsWith("/")) { | ||
| pathToAppend = pathToAppend.substring(1); | ||
| } | ||
|
|
||
| if (!basePath.endsWith("/")) { | ||
| basePath += "/"; | ||
| } | ||
|
|
||
| try { | ||
| return new URI( | ||
| base.getScheme(), | ||
| base.getUserInfo(), | ||
| base.getHost(), | ||
| base.getPort(), | ||
| basePath + pathToAppend, | ||
| base.getQuery(), | ||
| base.getFragment() | ||
| ); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to join URI paths", e); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach loses the ability to fully override the endpoint with a different URI that includes scheme host and port as well as path.
| this.metrics = normalizeAndJoinPaths(location, overrides.metrics); | |
| this.logs = normalizeAndJoinPaths(location, overrides.logs); | |
| } | |
| private URI normalizeAndJoinPaths(URI base, String path) { | |
| String basePath = base.getPath(); | |
| if (basePath == null) { | |
| basePath = "/"; | |
| } | |
| String pathToAppend = path; | |
| if (pathToAppend.startsWith("/")) { | |
| pathToAppend = pathToAppend.substring(1); | |
| } | |
| if (!basePath.endsWith("/")) { | |
| basePath += "/"; | |
| } | |
| try { | |
| return new URI( | |
| base.getScheme(), | |
| base.getUserInfo(), | |
| base.getHost(), | |
| base.getPort(), | |
| basePath + pathToAppend, | |
| base.getQuery(), | |
| base.getFragment() | |
| ); | |
| } catch (Exception e) { | |
| throw new RuntimeException("Failed to join URI paths", e); | |
| } | |
| this.metrics = resolveOverride(location, overrides.metrics); | |
| this.logs = resolveOverride(location, overrides.logs); | |
| } | |
| private URI resolveOverride( | |
| URI location, | |
| String override) | |
| { | |
| URI normalized = Optional.ofNullable(location.getPath()) | |
| .map(p -> p.endsWith("/") ? p : p + "/") | |
| .map(location::resolve) | |
| .orElse(location.resolve("/")); | |
| URI overrideURI = Optional.of(URI.create(override)) | |
| .map(...) | |
| // TODO: check for missing authority to treat as path only | |
| // TODO: if path has leading / then remove | |
| .get(); | |
| // TODO: resolve overrideURI against normalized location to support absolute or relative overrides | |
| } |
| assertThat(metrics, equalTo(URI.create("http://example.com/v42/metrix"))); | ||
| assertThat(logs, equalTo(URI.create("http://example.com/v42/logz"))); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test for full URI override, including scheme host and port that differ from base location.
- Add resolveOverride() to handle path-only and full URI overrides - Normalize base path and strip leading slash from override paths - Add tests for non-root base path and full URI override scenarios
|
@jfallows I've updated the PR based on your feedback:
The implementation now supports both path appending for non-root bases and full URI overrides. |
|
@Kunals990 the PR build is broken seems due to checkstyle issues. Please run a full local build |
Description
When base endpoint has a non-root path (e.g. /telemetry), default overrides for logs and metrics are now properly appended instead of replacing the base path.
Changes:
Tests:
Fixes #1602