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
[ELY-2167] Added javadoc for SNI realted classes. (SNIContextMatcher, SNISSLContext). #1712
Conversation
@@ -17,6 +17,9 @@ | |||
|
|||
import javax.net.ssl.SSLContext; | |||
|
|||
/** | |||
* A class which sets the SSLContext based on the given SNIContextMatcher using its default context and protocol. |
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.
@fazer1929 Since this class represents SSLContext , something similar to the following could be documented instead: SSLContext that uses provided SNIContextMatcher to resolve which SSLContext to use for the connection
* <ul> | ||
* <li>The exacts map defaults to being empty</li> | ||
* <li>The wildcards map defaults to being empty</li> | ||
* </ul> |
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.
@fazer1929 Just a minor. We don't have to be so specific when it comes to the listing of default values here. It is best to document only things that are not clear. If it was some specific hard coded values list then documenting it would make sense. But here the default is an empty list which can be assumed
@@ -26,6 +26,9 @@ | |||
import javax.net.ssl.SNIServerName; | |||
import javax.net.ssl.SSLContext; | |||
|
|||
/** | |||
* A class which returns a matching SSL context based on the SNI server name provided. |
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.
Since it is a list of servers that can be matched (see public SSLContext getContext(List<SNIServerName> servers
method below) I would reprhase a bit
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.
It'd be nice to document the getContext method and mention which entry from the servers
list will be used since it is not clear that the exacts are matched first and then the willdcards etc.
@fazer1929 Thank you for the PR! I added minor comments. |
@fazer1929 Thank you! |
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.
Thanks @fazer1929!
https://issues.redhat.com/browse/ELY-2167