-
Notifications
You must be signed in to change notification settings - Fork 926
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
b2eae62
4532087
6e31ca3
45dc82b
392c249
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,53 @@ | ||||||
/* | ||||||
* Copyright 2024 LINE Corporation | ||||||
* | ||||||
* LINE Corporation licenses this file to you under the Apache License, | ||||||
* version 2.0 (the "License"); you may not use this file except in compliance | ||||||
* with the License. You may obtain a copy of the License at: | ||||||
* | ||||||
* https://www.apache.org/licenses/LICENSE-2.0 | ||||||
* | ||||||
* Unless required by applicable law or agreed to in writing, software | ||||||
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||||||
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||||||
* License for the specific language governing permissions and limitations | ||||||
* under the License. | ||||||
*/ | ||||||
package com.linecorp.armeria.client; | ||||||
|
||||||
import java.util.function.Supplier; | ||||||
|
||||||
import javax.annotation.CheckForNull; | ||||||
|
||||||
/** | ||||||
* The utility class for ClientRequestContext. | ||||||
*/ | ||||||
public final class ClientRequestContextUtil { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The implementations of this class are not related to |
||||||
|
||||||
/** | ||||||
* Returns a non-null value. If an unexpected exception occurs while retrieving the first value, | ||||||
* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Armeria uses
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your comments! |
||||||
try { | ||||||
if (firstSupplier != null) { | ||||||
final T first = firstSupplier.get(); | ||||||
if (first != null) { | ||||||
return first; | ||||||
} | ||||||
} | ||||||
} catch (Exception e) { | ||||||
// Ignore | ||||||
} | ||||||
|
||||||
if (second != null) { | ||||||
return second; | ||||||
} | ||||||
|
||||||
throw new NullPointerException("Both parameters are null"); | ||||||
} | ||||||
|
||||||
private ClientRequestContextUtil() { | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -483,15 +483,20 @@ private void failEarly(Throwable cause) { | |
|
||
// TODO(ikhoon): Consider moving the logic for filling authority to `HttpClientDelegate.exceute()`. | ||
private void autoFillSchemeAuthorityAndOrigin() { | ||
final String authority = authority(); | ||
if (authority != null && endpoint != null && endpoint.isIpAddrOnly()) { | ||
// The connection will be established with the IP address but `host` set to the `Endpoint` | ||
// could be used for SNI. It would make users send HTTPS requests with CSLB or configure a reverse | ||
// proxy based on an authority. | ||
final String host = SchemeAndAuthority.of(null, authority).host(); | ||
if (!NetUtil.isValidIpV4Address(host) && !NetUtil.isValidIpV6Address(host)) { | ||
endpoint = endpoint.withHost(host); | ||
|
||
try { | ||
final String authority = authority(); | ||
if (endpoint != null && endpoint.isIpAddrOnly()) { | ||
// The connection will be established with the IP address but `host` set to the `Endpoint` | ||
// could be used for SNI. It would make users send HTTPS requests | ||
// with CSLB or configure a reverse proxy based on an authority. | ||
final String host = SchemeAndAuthority.of(null, authority).host(); | ||
if (!NetUtil.isValidIpV4Address(host) && !NetUtil.isValidIpV6Address(host)) { | ||
endpoint = endpoint.withHost(host); | ||
} | ||
} | ||
} catch (IllegalStateException e) { | ||
// Just pass, because it's normal condition. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow, It is better way! IMHO, I think |
||
} | ||
|
||
final HttpHeadersBuilder headersBuilder = internalRequestHeaders.toBuilder(); | ||
|
@@ -750,7 +755,6 @@ public String fragment() { | |
return requestTarget().fragment(); | ||
} | ||
|
||
@Nullable | ||
@Override | ||
public String authority() { | ||
final HttpHeaders additionalRequestHeaders = this.additionalRequestHeaders; | ||
|
@@ -774,6 +778,11 @@ public String authority() { | |
if (authority == null) { | ||
authority = internalRequestHeaders.get(HttpHeaderNames.HOST); | ||
} | ||
if (authority == null) { | ||
throw new IllegalStateException( | ||
"ClientRequestContext may be in the process of initialization." + | ||
"In this case, host() or authority() could be null"); | ||
} | ||
return authority; | ||
} | ||
|
||
|
@@ -794,29 +803,25 @@ private String origin() { | |
return origin; | ||
} | ||
|
||
@Nullable | ||
@Override | ||
public String host() { | ||
final String authority = authority(); | ||
if (authority == null) { | ||
return null; | ||
} | ||
return SchemeAndAuthority.of(null, authority).host(); | ||
} | ||
|
||
@Override | ||
public URI uri() { | ||
final String scheme = getScheme(sessionProtocol()); | ||
final String authority = authority(); | ||
final String path = path(); | ||
final String query = query(); | ||
final String fragment = fragment(); | ||
try (TemporaryThreadLocals tmp = TemporaryThreadLocals.acquire()) { | ||
final StringBuilder buf = tmp.stringBuilder(); | ||
buf.append(scheme); | ||
if (authority != null) { | ||
try { | ||
final String authority = authority(); | ||
buf.append("://").append(authority); | ||
} else { | ||
} catch (IllegalStateException e) { | ||
buf.append(':'); | ||
} | ||
buf.append(path); | ||
|
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.
Q) Did you change this because some were tests broken?
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.
Yes, a test case
testEmptyEndpointTags()
inBraveClientTest.class
failed.