[ECR-3223] LC: port is optional; added prefix #997
Conversation
bullet-tooth
requested review from
diminio1,
MakarovS,
DENMROOT and
dmitry-timofeev
| package com.exonum.client; | ||
|
|
||
| import static com.google.common.base.Preconditions.checkNotNull; | ||
| import static java.util.Objects.requireNonNull; |
There was a problem hiding this comment.
Why don't we use the already used locally checkNotNull?
| } | ||
|
|
||
| /** | ||
| * Optionally, sets prefix url to be applied to all requests made by the client. |
There was a problem hiding this comment.
Sets an optional URL prefix [= not optionally sets]...
+ There is no prefix by default.
| final class HttpUrlHelper { | ||
|
|
||
| static HttpUrl getFullUrl(URL host, String prefix, String relativeUrl, | ||
| Map<String, String> query) { |
There was a problem hiding this comment.
(encoded)QueryParams/Parameters?
| private static List<Arguments> source() { | ||
| Map<String, String> noQuery = emptyMap(); | ||
| return ImmutableList.of( | ||
| of("http://localhost/path/to/source", |
There was a problem hiding this comment.
Lone of is off 🙃 , please replace with arguments.
|
|
||
| /** | ||
| * Removes heading slash from the path. | ||
| * Useful because underlying OkHttp applies slashes when constructing paths. |
There was a problem hiding this comment.
Doesn't okhttp do that itself? Also, double slashes must not cause any problems, mustn't they?
There was a problem hiding this comment.
No, okhttp doesn't do that :(
Urls with double stashes are valid. But such urls semantically identical but not equal. It may cause issues when storing them in collections and potentially cause some issues on a server side, for example caching (where the cache key is a path to the resource).
I see this question is more complex: such as we allow users to specify host and prefix in a string form may be do we need to have a full normalization urls support, for example as described here ?
There was a problem hiding this comment.
I see, do I understand it correctly that we do the normalization to aid caching on a (possibly, intermediate) server in cases where the user has optional leading and/or trailing slashes? Isn't the naive approach used here enough?
There was a problem hiding this comment.
Yes, right.
But, I believe, we can simplify this and remove normalization and rely fully on OkHttp.
There was a problem hiding this comment.
I must admit that the URLs look better normalized, but agree it is not strictly needed (if a user cares, they can omit the slashes) :-)
| * - port is optional | ||
| * - prefix is optional | ||
| * - paths can start either with or without heading slash | ||
| * - query params is optional. |
| return path; | ||
| } | ||
| private static HttpUrl normalize(HttpUrl url) { | ||
| URI normalized = url.uri().normalize(); |
There was a problem hiding this comment.
It is no longer a naïve approach 🙃 Shall we add more tests specific to this normalization algorithm to exercise that?
|
Would you please also add a short changelog entry? The build currently fails due unused imports. |
fixed build
# Conflicts: # exonum-light-client/CHANGELOG.md # exonum-light-client/src/main/java/com/exonum/client/ExonumHttpClient.java
| } | ||
|
|
||
| static void assertExactPath(RecordedRequest request, String url) { | ||
| assertThat(request.getPath(), is("/" + url)); |
There was a problem hiding this comment.
I'd try with matchers, as these are unclear.
| } | ||
|
|
||
| static void assertExactPath(RecordedRequest request, String url) { | ||
| assertThat(request.getPath(), is("/" + url)); |
There was a problem hiding this comment.
I'd try with matchers, as these are unclear.
Overview
See: https://jira.bf.local/browse/ECR-3223
Definition of Done