-
Notifications
You must be signed in to change notification settings - Fork 174
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
0.5 wss support #42
0.5 wss support #42
Conversation
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 has changes to the exposed API. I suggest overloading the methods instead, to keep the API backwards compatible.
@@ -67,8 +67,9 @@ public JSONClient(ClientCoreProfile coreProfile, String identity) { | |||
featureRepository.addFeatureProfile(coreProfile); | |||
} | |||
|
|||
public void enableWSS(SSLContext sslContext) throws IOException { | |||
transmitter.enableWSS(sslContext); | |||
public JSONClient enableWSS(WssSocketBuilder wssSocketBuilder) throws IOException { |
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.
This is an exposed API, which means that changes will break peoples code. I suggest that we keep the original one and add this as an overload.
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.
Good point. Updated class to be backward compatible.
@@ -67,8 +67,9 @@ public JSONClient(ClientCoreProfile coreProfile, String identity) { | |||
featureRepository.addFeatureProfile(coreProfile); | |||
} | |||
|
|||
public void enableWSS(SSLContext sslContext) throws IOException { | |||
transmitter.enableWSS(sslContext); | |||
public JSONClient enableWSS(WssSocketBuilder wssSocketBuilder) throws IOException { |
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 acts like a builder. Not that I mind that much, but why?
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.
The reason behind is that we have creation of client and initialization of connection separated in the logic flow. A t the same time we need to ensure some parameters for a socket creation are present at the client creation time (SSLSocketFactory) and some of them we know only at the moment when we initialize connection (URI). Therefore I used builder to handle that distributed parameters gathering and encapsulate a creation logic at the same time.
@@ -53,8 +54,8 @@ public JSONServer(ServerCoreProfile coreProfile) { | |||
featureRepository.addFeatureProfile(coreProfile); | |||
} | |||
|
|||
public void enableWSS(SSLContext sslContext) { | |||
listener.enableWSS(sslContext); | |||
public void enableWSS(WssFactoryBuilder wssFactoryBuilder) { |
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.
This is an exposed API, which means that changes will break peoples code. I suggest that we keep the original one and add this as an overload.
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.
Good point. Updated class to be backward compatible.
@@ -53,8 +54,8 @@ public JSONServer(ServerCoreProfile coreProfile) { | |||
featureRepository.addFeatureProfile(coreProfile); | |||
} | |||
|
|||
public void enableWSS(SSLContext sslContext) { | |||
listener.enableWSS(sslContext); | |||
public void enableWSS(WssFactoryBuilder wssFactoryBuilder) { |
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.
The client returned an object of itself, this one doesn't.
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.
Fixed.
|
||
if(WSS_SCHEME.equals(resource.getScheme())) { | ||
try { | ||
client.setSocket(wssSocketBuilder |
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.
I haven't tested this theory, but this could end in a null pointer exception.
If the url starts with "wss:" and I didn't call enableWSS(...) before I called connect(...)
Then the wssSocketBuilder is null --> Null pointer exception.
Instantiating the wssSocketBuilder in the constructor would work around this, but I can't tell what would happen if I tried to connect without an sslContext.
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.
I've added verification to SSL... builders to fail fast if critical parameters we know at the moment of server/client creation are missing. Also created separate constructor for WSS scheme vs old constructor to be used for WS. I suggest a WS-initialized client must not (due to security reasons) try to communicate via WSS scheme and it probably won't be able to establish a connection in practice if a server is configured properly. So I've added explicit IllegalStateException in case we try to connect WS-initialized client to WSS endpoint.
So this PR will introduce two new interfaces to the external API. So I think this is a good time to discuss these interfaces. I like to argue that external API's (interfaces) should expose as little internal implementation logic as possible. I would also argue that the WssSocketBuilder and WssFactoryBuilder exposes too much. I have been wrapping my head around it since yesterday (it was late), but I keep coming back to one solution. We probably need to write a guide to use it, but as long as the overloaded enableWSS(sslContext..) is there to create the builder for them, I think it's okay. Any thoughts? |
I think a lib might be flexible, but the settings must be just one of the ways to build the object. Optional Builder is OK for it, that helps us to add more parameters without crashing of legacy code. For example, I really interested in authentication and would like to create a PR with it later.
Well, I guess a code of the project is pretty intelligible, users are able to understand what they might do just reading the code. |
- backward compatibility of exposed API preserved - symmetric implementation for server and client - fail-fast for server/client builders for WSS not initialized with SSLContext/SSLSocketFactory - More explanatory exception for scheme and configuration mismatch for client than just NPE
Codecov Report
@@ Coverage Diff @@
## master #42 +/- ##
============================================
- Coverage 51.09% 49.47% -1.63%
Complexity 673 673
============================================
Files 157 160 +3
Lines 2558 2642 +84
Branches 180 187 +7
============================================
Hits 1307 1307
- Misses 1183 1267 +84
Partials 68 68 |
server.start(); | ||
closed = false; | ||
} | ||
|
||
public void enableWSS(SSLContext sslContext) { | ||
server.setWebSocketFactory(new DefaultSSLWebSocketServerFactory(sslContext)); | ||
public void enableWSS(WssFactoryBuilder wssFactoryBuilder) { |
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.
if you throw IllegalStateException manually, you might say about it there.
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.
Right. Corresponding Javadoc added.
public void enableWSS(SSLContext sslContext) throws IOException { | ||
SSLSocketFactory factory = sslContext.getSocketFactory(); | ||
client.setSocket(factory.createSocket()); | ||
public void enableWSS(WssSocketBuilder wssSocketBuilder) { |
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.
if you throw IllegalStateException manually, you might say about it there.
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.
Right. Corresponding Javadoc added.
@TVolden very valid point regarding to the interfaces design. I propose I will change the one for the server. The few things to take into consideration for the client: we still need one to set URI to ensure contract with WebSocketTransmitter. Also it might make sense to preserve others to ensure in WebSocketTransmitter we are in sync with underlying socket but this is something I have to check. |
WssFactoryBuilder and WssSocketBuilder now expose only the minimum required API
@TVolden @sumlin after considering the question I think it's right to clean WSS-related interfaces from implementation details. At the same time I believe it's necessary to provide to users access to set network configuration (proxy, connection timeout, etc). Consequently I think one more optional constructor parameter with configuration for server/client might be an option. Then implementation will ensure that configuration is propagated to server/client and underlying sockets implementation. What do you think? (regarding to both WS and WSS). |
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.
Changes look good.
I think it's a good idea. |
No description provided.