Removed jm.CustomHeader from TlsSessionInfo heirarchy #4427
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When `TlsSessionInfo` was added, the only way to mark it as a header that not be rendered was to implement `CustomHeader`, and then implement a method called `suppressRendering` to return `true`. Obviously, this semantically didn't make sense since it's not a custom header, but not long after, a much better, more robust and semantically reasonable mechanism to ensure it didn't get rendered was provided in: akka/akka@a93a73e This added `renderInRequests` and `renderInResponses` flags to headers, along with the traits `RequestHeader`, `ResponseHeader`, and `SyntheticHeader`. The Scala `Tls-Session-Info` class was modified to implement `SyntheticHeader` instead of the Scala `CustomHeader` trait, but the author forgot to modify the Java `TlsSessionInfo` class to no longer extend the Java `CustomHeader` class. This didn't cause any real issue at the time, because there was nowhere that the Java `CustomHeader` class was returned or accepted by any API calls, or treated specially in anyway. That was, until akka#4348, which caused the modelled header lookup method to throw an exception, instead of return `None`, when the header is not present, if it extends the Java `CustomHeader` class. So, this change removes `CustomHeader` from the superclass heirarchy of `TlsSessionInfo`, and instead makes it implement the Scala `HttpHeader` class directly, like all other headers. As far as I can tell, this should cause no issues, as `CustomHeader` is otherwise still not treated specially anywhere or exposed in any API. It would only cause an issue if some end user code tried to assign it to a `CustomHeader` typed variable, which I don't see any reason to ever do that.
johanandren
approved these changes
Oct 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #4426.
When
TlsSessionInfowas added, the only way to mark it as a header that should not be rendered was to implementCustomHeader, and then implement a method calledsuppressRenderingto returntrue.Obviously, this semantically didn't make sense since it's not a custom header, but not long after, a much better, more robust and semantically reasonable mechanism to ensure it didn't get rendered was provided in:
akka/akka@a93a73e
This added
renderInRequestsandrenderInResponsesflags to headers, along with the traitsRequestHeader,ResponseHeader, andSyntheticHeader. The ScalaTls-Session-Infoclass was modified to implementSyntheticHeaderinstead of the ScalaCustomHeadertrait, but the author forgot to modify the JavaTlsSessionInfoclass to no longer extend the JavaCustomHeaderclass.This didn't cause any real issue at the time, because there was nowhere that the Java
CustomHeaderclass was returned or accepted by any API calls, or treated specially in anyway. That was, until #4348, which caused the modelled header lookup method to throw an exception, instead of returnNone, when the header is not present, if it extends the JavaCustomHeaderclass.So, this change removes
CustomHeaderfrom the superclass heirarchy ofTlsSessionInfo, and instead makes it implement the ScalaHttpHeaderclass directly, like all other headers. As far as I can tell, this should cause no issues, asCustomHeaderis otherwise still not treated specially anywhere or exposed in any API. It would only cause an issue if some end user code tried to assign it to aCustomHeadertyped variable, which I don't see any reason to ever do that.