Fix #1384 camel-mybatis native support #2525

zhfeng · 2021-04-28T06:19:09Z

leverage quarkus-mybatis
should enable quarkus.mybatis.xmlconfig.enable=true in application.properties
must use <dataSource type="QUARKUS"> in mybatis xml configuration file
have to use --report-unsupported-elements-at-runtime in the native mode currently and should be removed when Investigate to remove the --report-unsupported-elements-at-runtime in native mode quarkiverse/quarkus-mybatis#10 get resolved

zhfeng · 2021-04-28T06:20:39Z

This needs a new release of quarkus-mybatis

ppalaga

I vote for merging this only if --report-unsupported-elements-at-runtime is not required.

ppalaga · 2021-04-28T13:23:59Z

docs/modules/ROOT/pages/reference/extensions/mybatis.adoc

+[source, xml]
+----
+<properties>
+    <quarkus.package.type>native</quarkus.package.type>


<quarkus.package.type>native</quarkus.package.type> is not MyBatis specific and it does not need to be documented here.

ppalaga · 2021-04-28T13:26:55Z

docs/modules/ROOT/pages/reference/extensions/mybatis.adoc

+----
+<properties>
+    <quarkus.package.type>native</quarkus.package.type>
+    <quarkus.native.additional-build-args>--report-unsupported-elements-at-runtime</quarkus.native.additional-build-args>


Could you please explain why is this required? We generally do not want any additional-build-args.

OK, now I see there is quarkiverse/quarkus-mybatis#10 . I'd prefer not merging this unless quarkiverse/quarkus-mybatis#10 is fixed, or unless there is a clear idea how it can be fixed.

ppalaga · 2021-04-28T13:30:52Z

...nt/src/main/java/org/apache/camel/quarkus/component/mybatis/deployment/MybatisProcessor.java

+    @Record(ExecutionTime.STATIC_INIT)
+    @BuildStep
+    CamelBeanBuildItem mybatisComponent(
+            List<SqlSessionFactoryBuildItem> sqlSessionFactoryBuildItem, CamelMyBatisRecorder recorder) {


Why List<SqlSessionFactoryBuildItem>? Can there be more than one SqlSessionFactoryBuildItem?

yeah, this was introduced to support multi datasource support see quarkiverse/quarkus-mybatis#68

ppalaga · 2021-04-28T13:31:35Z

...nt/src/main/java/org/apache/camel/quarkus/component/mybatis/deployment/MybatisProcessor.java

+    CamelBeanBuildItem mybatisBeanComponent(
+            List<SqlSessionFactoryBuildItem> sqlSessionFactoryBuildItem, CamelMyBatisRecorder recorder) {
+        return new CamelBeanBuildItem("mybatis-bean", MyBatisBeanComponent.class.getName(),
+                recorder.createMyBatisBeanComponent(sqlSessionFactoryBuildItem.get(0).getSqlSessionFactory()));


Can we be sure that sqlSessionFactoryBuildItem is not empty?

sure, I create quarkiverse/quarkus-mybatis#74 for improving it when using mybatis xml configuration.

I did not ask for not creating sqlSessionFactory for each data source. (I have no idea whether that makes sense for Quarkivese MyBatis at all). I asked about the case when the list of sqlSessionFactoryBuildItems is empty. Is it made impossible somewhere in Quarkivese MyBatis or here? Because otherwise sqlSessionFactoryBuildItem.get(0) may throw an ArrayIndexOutOfBoundsException or similar and it would not inform the end user that (and where) he needs to define a datasource.

OK, I get it and I think it should be in Quarkivese MyBatis to make sure to create at least one sqlSessionFactory. Also do you know if it is possible to set quarkus.mybatis.xmlconfig.enable=true automatically when the camel-quarkus-mybatis extension is activated ?

I think the cleanest way would be to have a dedicated XmlConfigEnabledBuildItem for that in Quarkivese MyBatis. Other options I can think of are hacks.

Thanks @ppalaga and I will try this idea in quarkiverse/quarkus-mybatis#84

...sts/mybatis/src/main/java/org/apache/camel/quarkus/component/mybatis/it/MybatisResource.java

...n-tests/mybatis/src/test/java/org/apache/camel/quarkus/component/mybatis/it/MyBatisTest.java

ppalaga · 2021-04-28T13:45:06Z

...n-tests/mybatis/src/test/java/org/apache/camel/quarkus/component/mybatis/it/MyBatisTest.java

+        RestAssured.delete("/mybatis/deleteOne?id=456")
+                .then()
+                .statusCode(200)
+                .body(equalTo("1"));


Running testDelete() before testSelectOne() would make testSelectOne() fail. You should not assume any particular test method execution order. You may want to move this code to testSelectOne().

The MyBatisResource uses @transactional and marks the transaction with rollback only. So all of the changes will not persist in the database.

I have not noticed that, thanks for the explanation. However, to ensure that the persistence layer really works, I think it would be better to test with a non-rollback transaction manager and always do a followup read call to ensure that the insert/update/delete was successful.

I just move all of the tests in one @test method so them can run orderly

I just move all of the tests in one @test method so them can run orderly

Yeah, thanks, that's what we mostly do in case like this one.

zhfeng · 2021-04-28T14:04:05Z

Thanks @ppalaga for reviewing and I agree to not merge this PR until quarkiverse/quarkus-mybatis#10 get resolved. I just raise it for getting some feedback and make sure the mybatis xml configuration works with camel-mybatis.

zhfeng · 2021-07-09T09:01:02Z

@ppalaga I got some progress on quarkiverse/quarkus-mybatis#10 and I think it could be fixed with mybatis/mybatis-3#2284. So I'm waiting for the new mybatis release of 3.5.8 and come back to work with this PR.

zhfeng · 2022-01-04T16:03:45Z

quarkus-mybatis 0.0.11 bump mybatis to 3.5.8 and it still needs to bump mybatis version in camel-3.14.x refer to apache/camel#6632

zhfeng · 2022-02-08T15:33:14Z

@ppalaga I upgrade the GA release of quarkus-mybatis and it should fix to remove the --report-unsupported-elements-at-runtime option in native building. I think it is a good shape now.

ppalaga · 2022-02-08T16:21:14Z

@zhfeng could you please rebase on top recent main to get rid of the ipfs failures?

zhfeng · 2022-02-09T10:39:46Z

@ppalaga can we go for merging it since CI looks good ?

ppalaga · 2022-02-09T16:29:50Z

@ppalaga can we go for merging it since CI looks good ?

Yes, thank you very much for the contribution!

zhfeng marked this pull request as draft April 28, 2021 06:19

zhfeng marked this pull request as ready for review April 28, 2021 12:57

ppalaga requested changes Apr 28, 2021

View reviewed changes

zhfeng marked this pull request as draft April 28, 2021 14:04

ppalaga force-pushed the main branch from 0424b03 to 5fdc350 Compare June 23, 2021 12:59

zhfeng force-pushed the issue_1384_mybatis_native_support branch from 4316402 to 3cd88ed Compare January 4, 2022 16:00

zhfeng force-pushed the issue_1384_mybatis_native_support branch from 3cd88ed to 5c9c973 Compare February 8, 2022 13:04

zhfeng changed the title ~~[WIP] Fix #1384 camel-mybatis native support~~ Fix #1384 camel-mybatis native support Feb 8, 2022

zhfeng marked this pull request as ready for review February 8, 2022 15:25

Fix apache#1384 camel-mybatis native support

3b79122

zhfeng force-pushed the issue_1384_mybatis_native_support branch from 5c9c973 to 3b79122 Compare February 9, 2022 02:43

ppalaga merged commit 143ee21 into apache:main Feb 9, 2022

Fix #1384 camel-mybatis native support #2525

Fix #1384 camel-mybatis native support #2525

Uh oh!

Conversation

zhfeng commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhfeng commented Apr 28, 2021

Uh oh!

ppalaga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhfeng commented Apr 28, 2021

Uh oh!

zhfeng commented Jul 9, 2021

Uh oh!

zhfeng commented Jan 4, 2022

Uh oh!

zhfeng commented Feb 8, 2022

Uh oh!

ppalaga commented Feb 8, 2022

Uh oh!

zhfeng commented Feb 9, 2022

Uh oh!

ppalaga commented Feb 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zhfeng commented Apr 28, 2021 •

edited

Loading