Make ClientRequestContext.authority() and host() return non-null #5969
Conversation
d6f7ae3 to
b2eae62
Compare
| } | ||
|
|
||
| span.tag(SpanTags.TAG_HTTP_HOST, firstNonNull(ctx.authority(), "UNKNOWN")) | ||
| span.tag(SpanTags.TAG_HTTP_HOST, firstNonNullException(ctx::authority, "UNKNOWN")) |
There was a problem hiding this comment.
Q) Did you change this because some were tests broken?
There was a problem hiding this comment.
Yes, a test case testEmptyEndpointTags() in BraveClientTest.class failed.
// BraveClientTest.class
void testEmptyEndpointTags() {
...
// expected: "UNKNOWN"
// but was: null
assertThat(span.tag("http.host")).isEqualTo("UNKNOWN"); <--- Assertion Error
}| /** | ||
| * The utility class for ClientRequestContext. | ||
| */ | ||
| public final class ClientRequestContextUtil { |
There was a problem hiding this comment.
The implementations of this class are not related to ClientRequestContext.
Would rename it to ObjectUtil?
| * or the first value is null, this function will return the second value. | ||
| * Otherwise, it returns the first value. | ||
| */ | ||
| public static <T> T firstNonNullException(@CheckForNull Supplier<T> firstSupplier, @CheckForNull T second) { |
There was a problem hiding this comment.
Armeria uses @NonNullByDefault annotation so CheckForNull is unnecessary.
| public static <T> T firstNonNullException(@CheckForNull Supplier<T> firstSupplier, @CheckForNull T second) { | |
| public static <T> T firstNonNullException(Supplier<T> firstSupplier, T second) { |
There was a problem hiding this comment.
Thanks for your comments!
I didn't know that at all 🙇♂️
| } | ||
| } | ||
| } catch (IllegalStateException e) { | ||
| // Just pass, because it's normal condition. |
There was a problem hiding this comment.
What do you think of creating a private method to a nullable value instead of catching the exception?
@Nullable
private String authorityOrNull();There was a problem hiding this comment.
Wow, It is better way!
@ikhoon nim, I have a question.
Should the methods authority() and host() throw a CheckedException so that the user is forced to handle the error?
IMHO, I think UncheckedException is better than CheckedException in case. because it's easy to read.
Motivation:
host(),authority()will become stableAPI.Modifications:
host(),authority()ofClientRequestContextwill throwIllegalStateExceptionifhost,authorityare null.@UnstableAPI,@Nullablefromhost(),authority().autoFillSchemeAuthorityAndOrigin(). (It can be called during initialization ofClientRequestContext)Result:
ClientRequestContext.authority()andhost()return non-null #5552