CAMEL-18262 - Fixing the Template issue

[rhuan080]

Signed-off-by: Rhuan Rocha rhuan080@gmail.com

• Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
• Each commit in the pull request should have a meaningful subject line and body.
• If you're unsure, you can format the pull request title like [CAMEL-XXX] Fixes bug in camel-file component, where you replace CAMEL-XXX with the appropriate JIRA issue.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Run mvn clean install -Psourcecheck in your module with source check enabled to make sure basic checks pass and there are no checkstyle violations. A more thorough check will be performed on your pull request automatically.
  Below are the contribution guidelines:
  https://github.com/apache/camel/blob/main/CONTRIBUTING.md

[github-actions]

🚫 There are (likely) no components to be tested in this PR

[essobedo]

@rhuan080 Thx for the PR, just one remark, as you have a clear reproducer, would you mind adding a test to ensure that the problem is fixed and will not occur anymore?

[rhuan080]

Hi @essobedo the camel-model-core does not have a unit test. I have seen the tests are applied to components. Thus, I had doubts about providing unit tests here.

[zhfeng]

@rhuan080 can you add a unit test at https://github.com/apache/camel/tree/main/core/camel-core/src/test/java/org/apache/camel/model

[rhuan080]

Sure! I`ll send a commit soon.

[zhfeng]

Thanks @rhuan080 and that could be great!

[rhuan080]

@zhfeng and @essobedo the unit test is done!

[essobedo]

Thx for the unit test. Just to be sure, What happens when you remove your changes, does the test fail as expected?

[essobedo]

I guess this can be removed

[rhuan080]

Done!

[github-actions]

There are too many components to be tested in this PR, components were removed or the code needs a rebase: (303 likely to be tested)

[essobedo]

There are some checkstyle errors to fix:

[ERROR] /home/runner/work/camel/camel/core/camel-core/src/test/java/org/apache/camel/model/RouteConfigurationOnExceptionTest.java:1: Line does not match expected header line of '/*'. [Header]
[ERROR] /home/runner/work/camel/camel/core/camel-core/src/test/java/org/apache/camel/model/RouteConfigurationOnExceptionTest.java:13:1: Wrong order for 'java.net.ConnectException' import. [ImportOrder]

[rhuan080]

Done!

[github-actions]

There are too many components to be tested in this PR, components were removed or the code needs a rebase: (303 likely to be tested)

[github-actions]

There are too many components to be tested in this PR, components were removed or the code needs a rebase: (303 likely to be tested)

[zhfeng]

The stylecheck failure is related to GoogleSecretManagerPropertiesFunction.java. So I think this PR looks good.

[essobedo]

The PR doesn't really fix the bug, please don't merge

[essobedo]

The test case doesn't really prove that the bug is fixed.

Here is a good test case with normal route and route template:

class RouteConfigurationOnExceptionTest extends ContextTestSupport {

    @Override
    protected RouteBuilder[] createRouteBuilders() {
        return new RouteBuilder[] {
            new RouteBuilder() {
                @Override
                public void configure() {
                    routeTemplate("route-template")
                        .from("direct:start-template")
                        .routeConfigurationId("my-error-handler")
                        .throwException(RuntimeException.class, "Expected Error");
                }
            },
            new RouteBuilder() {
                @Override
                public void configure() {
                    TemplatedRouteBuilder.builder(context, "route-template")
                        .routeId("my-test-file-route")
                        .add();
                }
            },
            new RouteBuilder() {
                @Override
                public void configure() {
                    from("direct:start-normal")
                        .routeConfigurationId("my-error-handler")
                        .throwException(RuntimeException.class, "Expected Error");
                }
            },
            new RouteConfigurationBuilder() {
                @Override
                public void configuration() {
                    routeConfiguration("my-error-handler").onException(Exception.class).handled(true)
                        .transform(constant("Error Received"))
                        .to("mock:result");
                }
            }
        };
    }

    @Test
    void testRouteTemplateCanSupportRouteConfiguration() throws Exception {
        getMockEndpoint("mock:result").expectedMessageCount(1);
        getMockEndpoint("mock:result").expectedBodiesReceived("Error Received");
        template.sendBody("direct:start-template", "foo");
        assertMockEndpointsSatisfied();
    }

    @Test
    void testNormalRouteCanSupportRouteConfiguration() throws Exception {
        getMockEndpoint("mock:result").expectedMessageCount(1);
        getMockEndpoint("mock:result").expectedBodiesReceived("Error Received");
        template.sendBody("direct:start-normal", "foo");
        assertMockEndpointsSatisfied();
    }
}

You will see that the test with the route template fails and the other passes. After a quick investigation, the main additional problem seems to be the fact that the route is not prepared when registering it.

[rhuan080]

Hi @essobedo,

Actually, the tests work with the new code and doesn`t work with the old code. I think we have two problems here, the code update I have done is correct. However, the test works because it is calling the Advice.adviceWith. This code is passing on tests

@Test
    void testRouteTemplateCanSupportRouteConfiguration() throws Exception {
        AdviceWith.adviceWith(context, "my-test-file-route", routeBuilder -> {
            
        });
        getMockEndpoint("mock:result").expectedMessageCount(1);
        getMockEndpoint("mock:result").expectedBodiesReceived("Error Received");
        template.sendBody("direct:start-template", "foo");
        assertMockEndpointsSatisfied();
    }

[essobedo]

I believe that it works with the AdviceWith.doAdviceWith because in this case the route is properly prepared but please note that AdviceWith.doAdviceWith is only meant for testing purpose, we don't use it in production code so the code proposed in the new version of test case is more what we will do in practice.

[essobedo]

In other words, if you compare how a normal route and a templated route are initialized, you should see that in case of a templated route, this method is never called https://github.com/apache/camel/blob/main/core/camel-core-model/src/main/java/org/apache/camel/model/RoutesDefinition.java#L233

[rhuan080]

Hi @essobedo,

I have used your unit test code! Thank you for helping me with this! I sent another commit fixing the template that does not prepare the RouteDefinition. Please, look at this and tell me what you think. The first commit is right, however, it really don't solves the issue totally. Now it is solving.

[essobedo]

We are very close, one small comment

[essobedo]

It is not meant to be shared, I rather prefer that you create a new instance at each call instead.

[rhuan080]

Done!

[essobedo]

LGTM thx!

[github-actions]

There are too many components to be tested in this PR, components were removed or the code needs a rebase: (303 likely to be tested)

[github-actions]

There are too many components to be tested in this PR, components were removed or the code needs a rebase: (303 likely to be tested)

[github-actions]

There are too many components to be tested in this PR, components were removed or the code needs a rebase: (303 likely to be tested)

[davsclaus]

Thanks for the PR - I would like to prepare the route before adding