vert-x3 / vertx-service-proxy

EventBus Proxy generation
Apache License 2.0
67 stars 58 forks source link

Major regressions between 3.5 and 3.6 #82

Closed jponge closed 5 years ago

jponge commented 5 years ago

While upgrading code from 3.5.x to 3.6 (the guide for Java developers), I encountered 2 regressions.

Need for @VertxGen

We now need to also add @VertxGen.

Before:

@ProxyGen
public interface WikiDatabaseService { ... }

After:

@ProxyGen
@VertxGen
public interface WikiDatabaseService { ... }

Otherwise, compilation fails.

Static methods aren't being ignored anymore

Code generation used to ignore static methods, such as the idiomatic create methods we have in services:

  static WikiDatabaseService create(JDBCClient dbClient, HashMap<SqlQuery, String> sqlQueries, Handler<AsyncResult<WikiDatabaseService>> readyHandler) {
    return new WikiDatabaseServiceImpl(dbClient, sqlQueries, readyHandler);
  }

  static WikiDatabaseService createProxy(Vertx vertx, String address) {
    return new WikiDatabaseServiceVertxEBProxy(vertx, address);
  }

We now need to explicitly ignore them otherwise compilation fails:

  @GenIgnore
  static WikiDatabaseService create(JDBCClient dbClient, HashMap<SqlQuery, String> sqlQueries, Handler<AsyncResult<WikiDatabaseService>> readyHandler) {
    return new WikiDatabaseServiceImpl(dbClient, sqlQueries, readyHandler);
  }

  @GenIgnore
  static WikiDatabaseService createProxy(Vertx vertx, String address) {
    return new WikiDatabaseServiceVertxEBProxy(vertx, address);
  }
slinkydeveloper commented 5 years ago

Second one seems like mine error during porting to plain java codegen. I'm trying to reproduce it without success. Can you share the generated code involved?

jponge commented 5 years ago

It's from the Java guide, see https://github.com/vert-x3/vertx-guide-for-java-devs/blob/master/step-8/src/main/java/io/vertx/guides/wiki/database/WikiDatabaseService.java

slinkydeveloper commented 5 years ago

Maybe I'm wrong, but seems like the error is thrown by rx codegen (I'm testing with 3.6.0.CR1). Proxy and handler are generated correctly:

SEVERE: Could not generate model for io.vertx.guides.wiki.database.WikiDatabaseService#create(io.vertx.ext.jdbc.JDBCClient,java.util.HashMap<io.vertx.guides.wiki.database.SqlQuery,java.lang.String>,io.vertx.core.Handler<io.vertx.core.AsyncResult<io.vertx.guides.wiki.database.WikiDatabaseService>>): type java.util.HashMap<io.vertx.guides.wiki.database.SqlQuery,java.lang.String> is not legal for use for a parameter in code generation
io.vertx.codegen.GenException: type java.util.HashMap<io.vertx.guides.wiki.database.SqlQuery,java.lang.String> is not legal for use for a parameter in code generation
    at io.vertx.codegen.ClassModel.checkParamType(ClassModel.java:275)
    at io.vertx.codegen.ClassModel.getParams(ClassModel.java:1023)
    at io.vertx.codegen.ClassModel.createMethod(ClassModel.java:871)
    at io.vertx.codegen.ClassModel.lambda$traverseType$14(ClassModel.java:720)

Nov 19, 2018 10:45:13 AM io.vertx.codegen.CodeGenProcessor lambda$process$7
INFO: Generated model io.vertx.guides.wiki.database.WikiDatabaseService: io.vertx.guides.wiki.database.WikiDatabaseServiceVertxProxyHandler
Nov 19, 2018 10:45:13 AM io.vertx.codegen.CodeGenProcessor lambda$process$7
INFO: Generated model io.vertx.guides.wiki.database.WikiDatabaseService: io.vertx.guides.wiki.database.WikiDatabaseServiceVertxEBProxy

Looking inside generated classes, everything seems fine

tsegismont commented 5 years ago

What shall we do with this one? Is it documentation only?

jponge commented 5 years ago

For me this is breaking, it worked before, and the same code could not be upgraded directly to 3.6.

vietj commented 5 years ago

I will try to fix today and see if that is expected or not to require @VertxGen. Previously it was required but it might be because vertx-codegen and vertx-service-proxy were too tightly coupled.

In 3.6 it has been decoupled, so the fix or not will depend on what makes most sense, but it's not a strong thing. We might also make @VertxGen not required in 3.6 and required in master and make this as a v4 breaking change.

Stay tuned

vietj commented 5 years ago

do you have a ready reproducer to use @jponge ?

vietj commented 5 years ago

I will try build the guide and remove the @VertxGen annotation

vietj commented 5 years ago

so there are two separate issues highlighted in this issue right ?

jponge commented 5 years ago

The best reproducer is here: https://github.com/vert-x3/vertx-guide-for-java-devs/commit/a0617b5579569cf757c7a40539185636817fbaa3#diff-3ded3ce14c17ee0f03d4b71f8b2787b8R38

Yes, there are 2 issues: one for static methods but in my other Vert.x examples this seems to have been fixed, while the other is still present (@ProxyGen used to imply @VertxGen).

vietj commented 5 years ago

I'm hitting the rxjava issue and I think it comes from the fact that proxies have been decoupled from vertx-codegen.

Before it was a weird thing, i.e something annotated with @ProxyGen implies it is also an @VertxGen and proxy-gen relaxes the @VertxGen rules by simply ignoring them.

Now, since we need to have both @ProxyGen and @VertxGen (otherwise the RxJava code won't be generated anymore I assume), @VertxGen does not inherit the @ProxyGen rule and thus indeed declaring a static method with an RX type is illegal.

I think the solution is that WikiDatabaseService should not declare an RxJava method and instead it should declare a proxy factory and this method should be used from the Rx generated method.

I will make a PR to demonstrate.

vietj commented 5 years ago

here is the PR https://github.com/vert-x3/vertx-guide-for-java-devs/pull/58

tsegismont commented 5 years ago

@vietj it's good to have a PR for the guide. But I believe we should also document the breaking change.

slinkydeveloper commented 5 years ago

Can we close it?

jponge commented 5 years ago

Yep