Generated code of camel-connector-maven-plugin must not be below src/

rhuss commented 6 years ago

Generated code of camel-connector-maven-plugin changed signature between 2.20.0 and 2.20.1. I.e some call to component constructors now requires a string argument, which our connector definitions (twitter. htp-get) do not have. @lburgazzoli any idea what changed here ?

Actually, I found it quite unfortunate that the generated code is created in src/main and not below target as it should be for generated stuff. Also, we must not commit generated code when its generated during each run as part of the regular build.

rhuss commented 6 years ago

Well, looks like it already has changed before 2.20.0 but the connectors has not been updated yet. A bit lost.

rhuss commented 6 years ago

The compile error is

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.6.0:compile (default-compile) on project http-get-connector: Compilation failure
[ERROR] /Users/roland/Development/syndesis/syndesis/connectors/connectors/http/http-get-connector/src/main/java/io/syndesis/connector/http/springboot/HttpGetConnectorAutoConfiguration.java:[93,42] constructor HttpGetComponent in class io.syndesis.connector.http.HttpGetComponent cannot be applied to given types;
[ERROR]   required: no arguments
[ERROR]   found: java.lang.String
[ERROR]   reason: actual and formal argument lists differ in length
[ERROR]
[ERROR] -> [Help 1]

rhuss commented 6 years ago

and the generator code in question:

 for (Map.Entry<String, HttpGetConnectorConfigurationCommon> entry : configuration
                .getConfigurations().entrySet()) {
            parameters.clear();
            HttpGetComponent connector = new HttpGetComponent(entry.getKey());
             .....

rhuss commented 6 years ago

but HttpGetComponent (our code) has only a no-arg constructor.

zregvart commented 6 years ago

See this change in Camel.

rhuss commented 6 years ago

Actually this is the breaking change --> https://github.com/apache/camel/blame/master/connectors/camel-connector-maven-plugin/src/main/java/org/apache/camel/maven/connector/SpringBootAutoConfigurationMojo.java#L448

rhuss commented 6 years ago

@zregvart ah, yes ;-) So, what would I have todo to fix this in our components ?

btw, feels a bit strange for a patch level version to have an API change (but I'm not complaining as we are probably the only user of thise feature ...)

zregvart commented 6 years ago

@rhuss I guess the same thing that was introduced in that change for the Foo and Bar connectors:

https://github.com/apache/camel/commit/410cb46585ea18c2332f4a5d5e04f4417efc2ca9#diff-14e3264af0f8415831eb44d55d17af4cR27

Add constructor argument for componentScheme, haven't tried but it looks like a solution.

rhuss commented 6 years ago

@zregvart thanks ! I'm going to fix this. Nevertheless we should not generate into the source directory. I change the title of the issue accrordingly.

lburgazzoli commented 6 years ago

@rhuss this is a unfortunate consequence of two things:

there was a critical bug so the connectors were fundamentally broken as the underlying component was not per connector son in case you have the same connector aliased, that would result in an undefined behaviour at runtime
the camel connector class is not auto generated by the maven plugin as the spring boot bits but it is generated by the archetype so because we used to simply copy and paste project, any change on the underlying code could cause incompatibilities

I will work upstream to improve the maven plugin but I think for syndesis we should work on having a simplified version of the connectors stuffs first so we can get rid of the generated code and once we have something stable we can eventually contribute it back to camel upstream.

rhuss commented 6 years ago

@lburgazzoli sounds like a plan, thanks ;-)

I updated everything for Camel 2.20.1 in #453 (sorry for this all-in-one PR but I'm really busy fixing build stuff so please forgive me to not bother much about separate PRs)

rhuss commented 6 years ago

Oops, was already merged ;-) that's the next one --> #459

rhuss commented 6 years ago

@lburgazzoli is it correct that in this line --> https://github.com/rhuss/syndesis/blob/4db9c6839a6e2011970d3fcc9edcb4fefb6e4a1a/connectors/connectors/twitter/twitter-search-connector/src/main/java/io/syndesis/search/TwitterSearchComponent.java#L31

the "twitter-" prefix needs to be removed because its already part of the base scheme ? (same for all other connectors, too ?)

rhuss commented 6 years ago

That worked for the other connectors, however for twitte the baseScheme is "twitter-timeline" not "twitter" (so leading to a "twitter-timeline-mention-connector")

I wonder how this affects the deployment.json and the data contained in our database ?

rhuss commented 6 years ago

Not sure how to resolve this since it seems that "twitter-mention-connector" (instead of "twitter-timeline-mention-connector" as derived from the baseScheme) is quite used a bit in the code (just search for "twitter-mention-connector")

rhuss commented 6 years ago

Hmm, I really dont get it:

twitterEndpoint.getEndpointUri().equals("twitter-timeline-mention-connector")

but in this test the from still needs to be twitter-mention-connector (??)

 @Configuration
    public static class TestConfiguration {
        @Bean
        public RouteBuilder routeBuilder() {
            return new RouteBuilder() {
                @Override
                public void configure() throws Exception {
                    from("twitter-mention-connector")
                        .noAutoStartup()
                        .to("mock:result");
                }
            };
        }
    }

rhuss commented 6 years ago

Sorry, I have to give up. For the twitter search connector I get a 'twitter-search-search-connector' URI when I use search-connector as name in the TwitterSearchComponent. I reduced it now to connector only for the TwitterSearchComponent, so that the overall URI name is calculated again to twitter-search-connector (where twitter-search comes from the baseScheme).

Not really sure what I'm doing here ....

lburgazzoli commented 6 years ago

That is the generated underlying scheme which is not supposed to be directly used, for an end user it would be search-connector (or something like that) but the underlying component the connector configures needs to have a different name which is the result of the concatenation of some of the info the connector has at construction time.

It needs to be improved but it require a review of the connectors code.

rhuss commented 6 years ago

Thanks, however still not sure that I understand it properly ;-)

The PR #459 for migration to 2.20.1 has grown a bit (because of other fixes like license headers), so I suggest to merge #459 and then revisit the connector names as used within the code and referenced from the outside.

rhuss commented 6 years ago

@lburgazzoli @zregvart Now that PR #459 is merged, could you please have a quick look at the TwitterMentionConnector and TwitterSearchConnector and the corresponding unit tests, especially whether the names chosen make sense ?

jimmidyson commented 6 years ago

@lburgazzoli @zregvart Any comment on this? Do we need this for TP3, is it already fixed, or can we defer?

lburgazzoli commented 6 years ago

I think that this can be deferred as we need to fix it at camel and with new connectors stuffs there is no more code generation involved.

lburgazzoli commented 6 years ago

I think we can close this one as new connectors do not have any generated code, @rhuss is that ok ?

rhuss commented 6 years ago

Of course, if there is no code generated anymore than this issue is obsolete.

syndesisio / syndesis

Generated code of camel-connector-maven-plugin must not be below src/ #457