Skip to content
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

Make ClientRequestContext.authority() and host() return non-null #5969

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

chickenchickenlove
Copy link
Contributor

Motivation:

  • If host(), authority() never return null, host(), authority() will become stableAPI.

Modifications:

  • host(), authority() of ClientRequestContext will throw IllegalStateException if host, authority are null.
  • Remove annotations @UnstableAPI, @Nullable from host(), authority().
  • Rearrange a little bit in autoFillSchemeAuthorityAndOrigin(). (It can be called during initialization of ClientRequestContext)
  • Add a test case.

Result:

@chickenchickenlove chickenchickenlove changed the title Make ClientRequestContext.authority() and host() return non-null [Not Yet ]Make ClientRequestContext.authority() and host() return non-null Nov 7, 2024
@jrhee17 jrhee17 added this to the 1.32.0 milestone Nov 7, 2024
@chickenchickenlove chickenchickenlove changed the title [Not Yet ]Make ClientRequestContext.authority() and host() return non-null Make ClientRequestContext.authority() and host() return non-null Nov 7, 2024
@@ -72,7 +72,7 @@ public void parse(brave.http.HttpRequest request, TraceContext context, SpanCust
return;
}

span.tag(SpanTags.TAG_HTTP_HOST, firstNonNull(ctx.authority(), "UNKNOWN"))
span.tag(SpanTags.TAG_HTTP_HOST, firstNonNullException(ctx::authority, "UNKNOWN"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q) Did you change this because some were tests broken?

Copy link
Contributor Author

@chickenchickenlove chickenchickenlove Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Armeria uses @NonNullByDefault annotation so CheckForNull is unnecessary.

Suggested change
public static <T> T firstNonNullException(@CheckForNull Supplier<T> firstSupplier, @CheckForNull T second) {
public static <T> T firstNonNullException(Supplier<T> firstSupplier, T second) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comments!
I didn't know that at all 🙇‍♂️

}
} catch (IllegalStateException e) {
// Just pass, because it's normal condition.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of creating a private method to a nullable value instead of catching the exception?

@Nullable
private String authorityOrNull();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make ClientRequestContext.authority() and host() return non-null
3 participants