ok2c commented on code in PR #627:
URL:
https://github.com/apache/httpcomponents-client/pull/627#discussion_r2016149501
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/ProtocolSwitchStrategy.java:
##########
@@ -45,31 +48,53 @@
@Internal
public final class ProtocolSwitchStrategy {
- enum ProtocolSwitch { FAILURE, TLS }
+ private static final ProtocolVersionParser PARSER =
ProtocolVersionParser.INSTANCE;
public ProtocolVersion switchProtocol(final HttpMessage response) throws
ProtocolException {
final Iterator<String> it = MessageSupport.iterateTokens(response,
HttpHeaders.UPGRADE);
-
ProtocolVersion tlsUpgrade = null;
+ ProtocolVersion httpUpgrade = null;
+
while (it.hasNext()) {
- final String token = it.next();
- if (token.startsWith("TLS")) {
- // TODO: Improve handling of HTTP protocol token once
HttpVersion has a #parse method
- try {
- tlsUpgrade = token.length() == 3 ? TLS.V_1_2.getVersion()
: TLS.parse(token.replace("TLS/", "TLSv"));
- } catch (final ParseException ex) {
- throw new ProtocolException("Invalid protocol: " + token);
+ final String token = it.next().trim();
+ try {
+ if (token.startsWith("TLS") || token.startsWith("HTTP/")) {
Review Comment:
@arturobernalg This line looks completely unnecessary. Why? Parse first -
validate next.
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/ProtocolSwitchStrategy.java:
##########
@@ -45,31 +48,53 @@
@Internal
public final class ProtocolSwitchStrategy {
- enum ProtocolSwitch { FAILURE, TLS }
+ private static final ProtocolVersionParser PARSER =
ProtocolVersionParser.INSTANCE;
public ProtocolVersion switchProtocol(final HttpMessage response) throws
ProtocolException {
final Iterator<String> it = MessageSupport.iterateTokens(response,
HttpHeaders.UPGRADE);
-
ProtocolVersion tlsUpgrade = null;
+ ProtocolVersion httpUpgrade = null;
+
while (it.hasNext()) {
- final String token = it.next();
- if (token.startsWith("TLS")) {
- // TODO: Improve handling of HTTP protocol token once
HttpVersion has a #parse method
- try {
- tlsUpgrade = token.length() == 3 ? TLS.V_1_2.getVersion()
: TLS.parse(token.replace("TLS/", "TLSv"));
- } catch (final ParseException ex) {
- throw new ProtocolException("Invalid protocol: " + token);
+ final String token = it.next().trim();
+ try {
+ if (token.startsWith("TLS") || token.startsWith("HTTP/")) {
+ final ProtocolVersion version = parseToken(token);
+ if (token.startsWith("TLS")) {
Review Comment:
@arturobernalg Should not it be `#equalsIgnoreCase`?
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/ProtocolSwitchStrategy.java:
##########
@@ -45,31 +48,53 @@
@Internal
public final class ProtocolSwitchStrategy {
- enum ProtocolSwitch { FAILURE, TLS }
+ private static final ProtocolVersionParser PARSER =
ProtocolVersionParser.INSTANCE;
public ProtocolVersion switchProtocol(final HttpMessage response) throws
ProtocolException {
final Iterator<String> it = MessageSupport.iterateTokens(response,
HttpHeaders.UPGRADE);
-
ProtocolVersion tlsUpgrade = null;
+ ProtocolVersion httpUpgrade = null;
+
while (it.hasNext()) {
- final String token = it.next();
- if (token.startsWith("TLS")) {
- // TODO: Improve handling of HTTP protocol token once
HttpVersion has a #parse method
- try {
- tlsUpgrade = token.length() == 3 ? TLS.V_1_2.getVersion()
: TLS.parse(token.replace("TLS/", "TLSv"));
- } catch (final ParseException ex) {
- throw new ProtocolException("Invalid protocol: " + token);
+ final String token = it.next().trim();
+ try {
+ if (token.startsWith("TLS") || token.startsWith("HTTP/")) {
+ final ProtocolVersion version = parseToken(token);
+ if (token.startsWith("TLS")) {
+ tlsUpgrade = version;
+ } else {
+ httpUpgrade = version;
+ }
+ } else {
+ throw new ProtocolException("Unsupported protocol: " +
token);
+ }
+ } catch (final ParseException ex) {
+ if (token.startsWith("TLS")) {
Review Comment:
@arturobernalg Why? All this is unnecessary. Just throw `new
ProtocolException("Unsupported protocol: " + protocolVersion)`
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/ProtocolSwitchStrategy.java:
##########
@@ -45,31 +48,53 @@
@Internal
public final class ProtocolSwitchStrategy {
- enum ProtocolSwitch { FAILURE, TLS }
+ private static final ProtocolVersionParser PARSER =
ProtocolVersionParser.INSTANCE;
public ProtocolVersion switchProtocol(final HttpMessage response) throws
ProtocolException {
final Iterator<String> it = MessageSupport.iterateTokens(response,
HttpHeaders.UPGRADE);
-
ProtocolVersion tlsUpgrade = null;
+ ProtocolVersion httpUpgrade = null;
+
while (it.hasNext()) {
- final String token = it.next();
- if (token.startsWith("TLS")) {
- // TODO: Improve handling of HTTP protocol token once
HttpVersion has a #parse method
- try {
- tlsUpgrade = token.length() == 3 ? TLS.V_1_2.getVersion()
: TLS.parse(token.replace("TLS/", "TLSv"));
- } catch (final ParseException ex) {
- throw new ProtocolException("Invalid protocol: " + token);
+ final String token = it.next().trim();
+ try {
+ if (token.startsWith("TLS") || token.startsWith("HTTP/")) {
+ final ProtocolVersion version = parseToken(token);
+ if (token.startsWith("TLS")) {
+ tlsUpgrade = version;
+ } else {
+ httpUpgrade = version;
Review Comment:
@arturobernalg Why? We should only tolerate (do nothing) if it is
`HTTP/1.1`. All other versions should be rejected
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/ProtocolSwitchStrategy.java:
##########
@@ -45,31 +48,53 @@
@Internal
public final class ProtocolSwitchStrategy {
- enum ProtocolSwitch { FAILURE, TLS }
+ private static final ProtocolVersionParser PARSER =
ProtocolVersionParser.INSTANCE;
public ProtocolVersion switchProtocol(final HttpMessage response) throws
ProtocolException {
final Iterator<String> it = MessageSupport.iterateTokens(response,
HttpHeaders.UPGRADE);
-
ProtocolVersion tlsUpgrade = null;
+ ProtocolVersion httpUpgrade = null;
+
while (it.hasNext()) {
- final String token = it.next();
- if (token.startsWith("TLS")) {
- // TODO: Improve handling of HTTP protocol token once
HttpVersion has a #parse method
- try {
- tlsUpgrade = token.length() == 3 ? TLS.V_1_2.getVersion()
: TLS.parse(token.replace("TLS/", "TLSv"));
- } catch (final ParseException ex) {
- throw new ProtocolException("Invalid protocol: " + token);
+ final String token = it.next().trim();
+ try {
+ if (token.startsWith("TLS") || token.startsWith("HTTP/")) {
+ final ProtocolVersion version = parseToken(token);
+ if (token.startsWith("TLS")) {
+ tlsUpgrade = version;
+ } else {
+ httpUpgrade = version;
+ }
+ } else {
+ throw new ProtocolException("Unsupported protocol: " +
token);
+ }
+ } catch (final ParseException ex) {
+ if (token.startsWith("TLS")) {
+ throw new ProtocolException("Invalid TLS protocol: " +
token, ex);
+ } else if (token.startsWith("HTTP/")) {
+ throw new ProtocolException("Invalid HTTP protocol: " +
token, ex);
+ } else {
+ throw new ProtocolException("Unsupported protocol: " +
token, ex);
}
- } else if (token.equals("HTTP/1.1")) {
- // TODO: Improve handling of HTTP protocol token once
HttpVersion has a #parse method
- } else {
- throw new ProtocolException("Unsupported protocol: " + token);
}
}
- if (tlsUpgrade == null) {
+
+ if (tlsUpgrade != null) {
+ return tlsUpgrade;
+ } else if (httpUpgrade != null) {
+ return httpUpgrade;
+ } else {
throw new ProtocolException("Invalid protocol switch response");
}
- return tlsUpgrade;
}
+ private ProtocolVersion parseToken(final String token) throws
ParseException {
+ if (token.equals("TLS")) {
+ return TLS.V_1_2.getVersion();
Review Comment:
@arturobernalg Why? I do not understand why you want to treat all `TLS`
versions as `TLS/1.2`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org
For additional commands, e-mail: dev-h...@hc.apache.org
