Enhance ProtocolSwitchStrategy with Simplified Protocol Parsing #627
Conversation
|
@arturobernalg Is there a reason for not using |
arturobernalg
force-pushed
the
protocolSwitchStrategy
branch
from
efefe21 to
80d6155
Compare
@ok2c please do another pass |
httpclient5/src/main/java/org/apache/hc/client5/http/impl/ProtocolSwitchStrategy.java
Outdated
Show resolved
Hide resolved
arturobernalg
force-pushed
the
protocolSwitchStrategy
branch
from
a7a8993 to
d26c7eb
Compare
arturobernalg
force-pushed
the
protocolSwitchStrategy
branch
from
3485c89 to
17dafc8
Compare
httpclient5/src/main/java/org/apache/hc/client5/http/impl/ProtocolSwitchStrategy.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/impl/ProtocolSwitchStrategy.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/impl/ProtocolSwitchStrategy.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/impl/ProtocolSwitchStrategy.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/impl/ProtocolSwitchStrategy.java
Outdated
Show resolved
Hide resolved
arturobernalg
force-pushed
the
protocolSwitchStrategy
branch
2 times, most recently
from
8e729c9 to
e5660d0
Compare
|
Please @ok2c do another pass |
httpclient5/src/main/java/org/apache/hc/client5/http/impl/ProtocolSwitchStrategy.java
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/impl/ProtocolSwitchStrategy.java
Outdated
Show resolved
Hide resolved
arturobernalg
force-pushed
the
protocolSwitchStrategy
branch
from
37fdd6f to
429759f
Compare
ok2c
left a comment
ok2c
left a comment
There was a problem hiding this comment.
@arturobernalg So much better in my opinion. Now to make it better still, please consider copying #parseHeaders method from apache/httpcomponents-core@99630f4 and using it to parse the sequence of protocol tokens without making intermediate String objects
Unify HTTP and TLS token parsing in the Upgrade header by replacing custom version parsing with ProtocolVersionParser. This change removes redundant code and ensures that only supported protocols (HTTP/ and TLS tokens) are accepted, while all other upgrade protocols are rejected as unsupported.
arturobernalg
force-pushed
the
protocolSwitchStrategy
branch
from
c88410f to
42c6006
Compare
All right @ok2c I think now i have full in all the remarks |
| return null; | ||
| } | ||
|
|
||
| if (end - start == 3) { |
There was a problem hiding this comment.
@arturobernalg I think there is a way to make it nicer, but it is good enough for now
…627) Unify HTTP and TLS token parsing in the Upgrade header by replacing custom version parsing with ProtocolVersionParser. This change removes redundant code and ensures that only supported protocols (HTTP/ and TLS tokens) are accepted, while all other upgrade protocols are rejected as unsupported.
|
@arturobernalg I took a closer look at your code and had to refactor a few things. Please take a look at 0ba6102. I hope you agree implementation is shorter and cleaner. I also had to fix the Exception message asserts in the test case. |
HI @ok2c look good. Thank for the review. |
2 participants
refines the ProtocolSwitchStrategy class to improve the parsing of protocol tokens in the Upgrade header, making the implementation more elegant and future-proof.