fix(event-uri): resolve uri with any existing context path #212
Conversation
| Assert.assertEquals( | ||
| "https://www.example.com/diagnostic", | ||
| uri.toString() | ||
| ); |
There was a problem hiding this comment.
It'd be preferable to test this in more of an end-to-end way, as the other HTTP-related tests in this file do. That is, instead of verifying that this low-level method appendPathToBaseURI does what it should, we would want to verify that DefaultEventSender, when configured with such-and-such a base URL, actually sends a request with the expected URL path to the target server. Look for instance at analyticsDataIsDelivered().
The reason we strongly prefer this approach is not just to avoid having to expose low-level helper methods to the tests. It's because even if you can prove that the helper method behaves properly when called with certain parameters, that does not prove that the higher-level code is using the helper method properly (or at all). Ultimately we don't care whether it is using that helper method or doing some equivalent computation of its own, what we care about is that the component works. And we definitely don't want to have a situation where we could accidentally introduce a regression in the higher-level component logic and have it not show up as a test failure because what we were testing was an implementation detail we assume it is using.
|
I'm assuming the intention here is to support custom base URIs that are going through some kind of proxy with URL rewriting, or a custom build of the Relay Proxy with different routing, since neither the actual LaunchDarkly service endpoints nor the standard Relay Proxy have a base context path. If so, I think a more general solution would be appropriate. The proposed fix is only for the events endpoint, but it's also possible to customize the base URI for the streaming and polling data sources and I think there are likely to be similar issues there. Whether you want to rework the PR to address this (and the unit testing issue I commented on) is up to you. I'd be happy to take care of those details and treat this as a bug report with a suggested fix, and we would credit you with a thank-you in the release note regardless of whether we accept this PR. |
Thanks for the quick response. You are absolutely correct. We use our own proxy with different routing. The context paths of the custom routes are getting completely dropped here and causing 404s. |
|
We've released version 5.1.1, which includes an extended version of this fix (there were similar issues with the other base URL settings). |
Requirements
Related issues
The way the URL is currently being resolved, it'd drop existing context paths if the uri didn't have a trailing
/Here's a sample unit tests that proves this bug
Describe the solution you've provided
if the URI doesn't have a trailing slash, still respect any existing context paths.
Describe alternatives you've considered
I tried providing a baseUri with trailing slash. But then another endpoint: /all, was attempting to add its own slash and failed since the baseuri already had one.
So it seemed reasonable to me that this would just respect the context path provided to it regardless of it having trailing slash or not.
Additional context
I've added additional unit tests to cover the following scenarios: