0.5 wss support #42

eupakhomov · 2018-04-16T00:52:20Z

No description provided.

coveralls · 2018-04-16T00:55:53Z

Coverage decreased (-1.7%) to 52.044% when pulling 00f8bb9 on eupakhomov:0.5_wss_support into 4821388 on ChargeTimeEU:master.

TVolden

It has changes to the exposed API. I suggest overloading the methods instead, to keep the API backwards compatible.

TVolden · 2018-04-17T06:46:59Z

ocpp-v1_6/src/main/java/eu/chargetime/ocpp/JSONClient.java

@@ -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 {


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.

Good point. Updated class to be backward compatible.

TVolden · 2018-04-17T06:47:55Z

ocpp-v1_6/src/main/java/eu/chargetime/ocpp/JSONClient.java

@@ -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 {


It acts like a builder. Not that I mind that much, but why?

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.

TVolden · 2018-04-17T06:48:11Z

ocpp-v1_6/src/main/java/eu/chargetime/ocpp/JSONServer.java

@@ -53,8 +54,8 @@ public JSONServer(ServerCoreProfile coreProfile) {
        featureRepository.addFeatureProfile(coreProfile);
    }

-    public void enableWSS(SSLContext sslContext) {
-        listener.enableWSS(sslContext);
+    public void enableWSS(WssFactoryBuilder wssFactoryBuilder) {


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.

Good point. Updated class to be backward compatible.

TVolden · 2018-04-17T06:48:54Z

ocpp-v1_6/src/main/java/eu/chargetime/ocpp/JSONServer.java

@@ -53,8 +54,8 @@ public JSONServer(ServerCoreProfile coreProfile) {
        featureRepository.addFeatureProfile(coreProfile);
    }

-    public void enableWSS(SSLContext sslContext) {
-        listener.enableWSS(sslContext);
+    public void enableWSS(WssFactoryBuilder wssFactoryBuilder) {


The client returned an object of itself, this one doesn't.

TVolden · 2018-04-17T06:55:53Z

ocpp-v1_6/src/main/java/eu/chargetime/ocpp/WebSocketTransmitter.java

+
+        if(WSS_SCHEME.equals(resource.getScheme())) {
+            try {
+                client.setSocket(wssSocketBuilder


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.

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.

TVolden · 2018-04-17T07:59:07Z

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.
All fields are contracted by the interface eg. : proxy, sslSocketFactory, tcpNoDelay... but the only important part (as I see it) is that they can build a Socket (and WebSocketServerFactory for the other one).

I have been wrapping my head around it since yesterday (it was late), but I keep coming back to one solution.
Strip the interfaces for all methods except the Build() method and expose the concrete builder (it's only for Websocket after all). This will still make the builder an external API, but altso make it unlikely that the interfaces change, which makes it easy to implement a custom builders.

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?

sumlin · 2018-04-17T08:44:58Z

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 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.

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.

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-io · 2018-04-17T10:27:05Z

Codecov Report

Merging #42 into master will decrease coverage by 1.62%.
The diff coverage is 0%.

@@             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

sumlin · 2018-04-17T08:28:39Z

ocpp-v1_6/src/main/java/eu/chargetime/ocpp/WebSocketListener.java

        server.start();
        closed = false;
    }

-    public void enableWSS(SSLContext sslContext) {
-        server.setWebSocketFactory(new DefaultSSLWebSocketServerFactory(sslContext));
+    public void enableWSS(WssFactoryBuilder wssFactoryBuilder) {


if you throw IllegalStateException manually, you might say about it there.

Right. Corresponding Javadoc added.

sumlin · 2018-04-17T08:30:01Z

ocpp-v1_6/src/main/java/eu/chargetime/ocpp/WebSocketTransmitter.java

-    public void enableWSS(SSLContext sslContext) throws IOException {
-        SSLSocketFactory factory = sslContext.getSocketFactory();
-        client.setSocket(factory.createSocket());
+    public void enableWSS(WssSocketBuilder wssSocketBuilder) {


if you throw IllegalStateException manually, you might say about it there.

Right. Corresponding Javadoc added.

eupakhomov · 2018-04-17T11:02:35Z

@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

eupakhomov · 2018-04-17T14:50:47Z

@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).

TVolden

Changes look good.

TVolden · 2018-04-17T16:37:14Z

I think it's a good idea.

Pluggable WSS support added

050f5d5

TVolden requested changes Apr 17, 2018

View reviewed changes

sumlin reviewed Apr 17, 2018

View reviewed changes

Javadoc added for WSS related methods and classes

13c6b17

WssFactoryBuilder and WssSocketBuilder now expose only the minimum required API

TVolden approved these changes Apr 17, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into 0.5_wss_support

00f8bb9

TVolden merged commit 428466c into ChargeTimeEU:master Apr 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

0.5 wss support #42

0.5 wss support #42

eupakhomov commented Apr 16, 2018

coveralls commented Apr 16, 2018 •

edited

Loading

TVolden left a comment

TVolden Apr 17, 2018

eupakhomov Apr 17, 2018

TVolden Apr 17, 2018

eupakhomov Apr 17, 2018

TVolden Apr 17, 2018

eupakhomov Apr 17, 2018

TVolden Apr 17, 2018

eupakhomov Apr 17, 2018

TVolden Apr 17, 2018

eupakhomov Apr 17, 2018

TVolden commented Apr 17, 2018 •

edited

Loading

sumlin commented Apr 17, 2018 •

edited

Loading

codecov-io commented Apr 17, 2018 •

edited

Loading

sumlin Apr 17, 2018

eupakhomov Apr 17, 2018

sumlin Apr 17, 2018

eupakhomov Apr 17, 2018

eupakhomov commented Apr 17, 2018 •

edited

Loading

eupakhomov commented Apr 17, 2018 •

edited

Loading

TVolden left a comment

TVolden commented Apr 17, 2018

0.5 wss support #42

0.5 wss support #42

Conversation

eupakhomov commented Apr 16, 2018

coveralls commented Apr 16, 2018 • edited Loading

TVolden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TVolden commented Apr 17, 2018 • edited Loading

sumlin commented Apr 17, 2018 • edited Loading

codecov-io commented Apr 17, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eupakhomov commented Apr 17, 2018 • edited Loading

eupakhomov commented Apr 17, 2018 • edited Loading

TVolden left a comment

Choose a reason for hiding this comment

TVolden commented Apr 17, 2018

coveralls commented Apr 16, 2018 •

edited

Loading

TVolden commented Apr 17, 2018 •

edited

Loading

sumlin commented Apr 17, 2018 •

edited

Loading

codecov-io commented Apr 17, 2018 •

edited

Loading

eupakhomov commented Apr 17, 2018 •

edited

Loading

eupakhomov commented Apr 17, 2018 •

edited

Loading