Skip to content
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

Major regressions between 3.5 and 3.6 #82

Closed
jponge opened this issue Nov 13, 2018 · 15 comments
Closed

Major regressions between 3.5 and 3.6 #82

jponge opened this issue Nov 13, 2018 · 15 comments
Labels
Milestone

Comments

@jponge
Copy link
Member

jponge commented Nov 13, 2018

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
Copy link
Member

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
Copy link
Member Author

jponge commented Nov 18, 2018

@slinkydeveloper
Copy link
Member

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

@vietj vietj modified the milestones: 3.6.0, 4.0.0 Nov 29, 2018
@tsegismont
Copy link
Contributor

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

@jponge
Copy link
Member Author

jponge commented Nov 30, 2018

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

@vietj
Copy link
Contributor

vietj commented Dec 12, 2018

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
Copy link
Contributor

vietj commented Dec 12, 2018

do you have a ready reproducer to use @jponge ?

@vietj
Copy link
Contributor

vietj commented Dec 12, 2018

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

@vietj
Copy link
Contributor

vietj commented Dec 12, 2018

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

@jponge
Copy link
Member Author

jponge commented Dec 12, 2018

The best reproducer is here: vert-x3/vertx-guide-for-java-devs@a0617b5#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
Copy link
Contributor

vietj commented Dec 12, 2018

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
Copy link
Contributor

vietj commented Dec 12, 2018

here is the PR vert-x3/vertx-guide-for-java-devs#58

@tsegismont
Copy link
Contributor

tsegismont commented Dec 12, 2018 via email

@slinkydeveloper
Copy link
Member

Can we close it?

@jponge
Copy link
Member Author

jponge commented May 3, 2019

Yep

@jponge jponge closed this as completed May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

4 participants