[WEJBHTTP-106] Allow multi-versioned HTTP handlers per HTTP Service

[fl4via]

Jira: https://issues.redhat.com/browse/WEJBHTTP-106

[fl4via]

@ropalka for now, this is for discussion, not for merging, yet.
@rachmatowicz please have a look and validate or not this change according to what we discussed. If validated, it can be merged, but notice that we must bump the version to 2.1.0.Final @ropalka

[rachmatowicz]

Hi @fl4via
I had a look at your changes and it seems to me that they will allow me to version the Http*Service protocol handlers in the way that I need to. Thanks for creating this PR!
There is one other small addition which I think might help and that is having an enum to represent the existing Protocol versions and some helper methods to generate Protocol version related strings. Something like this:

public enum Protocol {
   JAVAEE_PROTOCOL_VERSION(1),
   JAKARTAEE_PROTOCOL_VERSION(2),
   PROTOCOL_VERSION(3);

   private final int version;

   Protocol(int version) {
      this.version = version;
   }
   public String getVersion() {
      return "v" + version;
   }

  public boolean since(Protocol protocol) {
    return this.version >= protocol.version;
  }
}

The idea is to have a single implementation of an HttpHandler which accepts a protocol version and to use the protocol version and case statements within the implementation to accommodate changes between versions. The versions for each handler will have a lot in common. Then initialization might be able to do something like:

for (Protocol protocol : Protocol.getValues()) {
  PathHandler ph = new PathHandler();
  ph.addPrefixPath("/" + protocol.getVersion() + EJB_INVOKE_PATH, new AllowedMethodHandler(new HttpInvovationHandler(protocol, association, executorService, localTransactionContext, ..., Methods.POST)
  ...
  } 
}

This one idea. Would you have any objections to this or is there a better way to allow the HttpHandler implementations to be versioned?

[rachmatowicz]

On second thought, I could probably just include an anonymous class as described above in the base class used for the handler classes, as this may be the only place such a class would be used.
One way to proceed is to give me a few days to rebase my branch on these changes and check that they work, and then merge the branch once that has been confirmed. Does that sound OK? I'll get started on the rebasing today.

[ropalka]

. . . having an enum to represent the existing Protocol versions and some helper methods to generate Protocol version related strings . . .

public enum Protocol {
   JAVAEE_PROTOCOL_VERSION(1),
   JAKARTAEE_PROTOCOL_VERSION(2),
   PROTOCOL_VERSION(3);

   private final int version;

   Protocol(int version) {
      this.version = version;
   }
   public String getVersion() {
      return "v" + version;
   }

  public boolean since(Protocol protocol) {
    return this.version >= protocol.version;
  }
}

The idea is to have a single implementation of an HttpHandler which accepts a protocol version and to use the protocol version and case statements within the implementation to accommodate changes between versions. The versions for each handler will have a lot in common. Then initialization might be able to do something like:

for (Protocol protocol : Protocol.getValues()) {
  PathHandler ph = new PathHandler();
  ph.addPrefixPath("/" + protocol.getVersion() + EJB_INVOKE_PATH, new AllowedMethodHandler(new HttpInvovationHandler(protocol, association, executorService, localTransactionContext, ..., Methods.POST)
  ...
  } 
}

This one idea. Would you have any objections to this or is there a better way to allow the HttpHandler implementations to be versioned?

I like @rachmatowicz proposal. I also have no objections to your proposal PR @fl4via

[rachmatowicz]

Hi @fl4via, @ropalka

I now have a branch of wildfly-http-client which merges my stickiness changes for wildfly-http-client onto this branch.
The branch is here: #108

I only had two issues when using your PR for multiple handler versions:
(1) because Protocol is used extensively in your code changes for interoperability, I did not make any changes to it but added a temporary class called Version which defines protocol handler versions. I'm hoping that you could look at it and see a clean way to integrate the functionality of the class Protocol and the enum Version together.
(2) the call to register the multi-version handlers, as described here (https://github.com/wildfly/wildfly-http-client/pull/102/files#diff-8b824111c08ca224431abad5b2e2edbcfa49e20f7dd35392e3456014f2db591cR110), gave a little bit of discomfort, as two versions of the Protocol are mapped to the same index in the handler array:
v1 -> handlerArray[0]
v2 -> handlerArray[0]
v3 -> handlerArray[1]
instead of
v1 -> handlerArray[0]
v2 -> handlerArray[1]
v3 -> handlerArray[2]
The only negative point was that I had to create a workaround in my Version class so that only two handlers would be registered (instead of three); the handler for versions 1 and 2, and the handler for version 2. This might not make any sense now, but you'll see what I mean in the implementation.

There are two tests failing in the wildfly-http-client testsuite which I am currently trying to fix. They involve the use of affinity in the wildfly-http-client test framework. The changes I made do on the other hand work with Wildfly and the test described here (https://issues.redhat.com/browse/WFLY-16950).

The final point is that the tests that pass are non-transactional only. There is still an outstanding problem with transactional contexts being lost during invocation (https://issues.redhat.com/browse/WEJBHTTP-103).

[fl4via]

That's great news @rachmatowicz ! I'm having a look at the PR and the involved code to give you some feedback. Hopefully we will have all this sorted out and a new wildfly-http-client release with the fixes pretty soon!

[fl4via]

I am closing this because it is clear that this is not exactly the feature we wanted in wildlfy-http-client to move forward.