mybatis-3

mybatis-3 copied to clipboard

#101 Add support for constructor collection injection

It is now possible to create completely immutable objects (including nested collections) with MyBatis.

This has been done by keeping another meta object in memory until it is ready for creation, the detection for this is based on CacheKey.

This functionality is fully hidden behind a configuration flag called experimentalConstructorCollectionMapping, the idea is that MyBatis will have exactly the same (current) behaviour when this flag is not set (default false) and the community can enable this to test different use cases and report possible issues.

This would allow us to integrate this behaviour while gathering community feedback while not affecting any current workloads.

New functionality is tested in src/test/java/org/apache/ibatis/immutable/ImmutableConstructorTest.java.

I added debug symbols as an additional patch which really helps understanding what is happening.

~~What still needs needs to be done~~

• [x] Documentation
• [x] Testing with more than two levels of nesting (currently Blog -> Post > [Comment + Tag])
  • See ImmutableCollectionConstructorTest.java - House -> Room -> Furniture -> Defect
• [x] Testing result ordering
  • We cannot reliably determine when to build the final object if the results are not ordered, so we require this attribute
• [x] Testing with multiple result sets (throwing exception now)
  • Descoping this for now. could create follow up.
• [x] Testing with mixed mappings (including nestedQuery)
  • See postForConstructorInit in BoundBlogMapper.xml
• [x] Testing with very large resultSets (performance)

coverage: 87.446% (+0.09%) from 87.359% when pulling fd69cf0335edf71ff60e329b3b09b43c82ea3413 on epochcoder:feature/101-support-constructor-collection-injection into a3d1d14a28e73c2b6e18027bfc793c637445327f on mybatis:master.

Add_debugging_info_for_DefaultResultHandler.patch

Here is the patch to add debug symbols and print lines to see what is happening visually

This would be awesome to have as it is currently the main reason why MyBatis entity objects often can't be immutable Java objects in our code base.

Thank you for the PR, @epochcoder ,

I will take a look, but it may take some time as I don't have much spare time right now. Please try to eliminate unrelated changes like formatting, import order, etc..

Thank you for your feedback @harawata! I completely understand, take your time. Ill make sure to focus on the relevant changes and eliminate any unrelated ones (most of them are as a result of format & impsort plugins), Ill try to revert it to make the PR easier to review. Your input on this is very valuable!

@harawata I've removed all unnecessary formatting. I left the headers in tact (2023 -> 2024) please let me know in case you would like me to revert that as well! ;-)

FWIW, I tested this change on a large part of our stack which includes 10's of thousands of mapped statements, both with the experimentalConstructorCollectionMapping enabled, and disabled.

However, given that this code activates an entirely new path which used to fail, there are no current use cases which actively invoke it, and by extension, any other users of MyBatis.

But from a backward compatibility point of view, this is solid.

@harawata I'm thinking we cut a last 3.5.x release, move this to 3.6.x and then officially move to java 11 or maybe even 17. WDYT?

I wrote a rudimentary performance test for this (not using JMH)

The setup is 100 houses (main rows) with 200 rooms subdivided between them, with 50 furniture objects subdivided among those rooms, and 10 defects spread among them.

I then created a identical result maps for both property and constructor injection retrieving the same data. Each retrieval uses a new sql session.

The results from what I could gather were that using constructor injection (this functionality) is very slightly slower than using property injection.

Please let me know if you would like me to add this setup as another commit (though i'm leaning a bit against it as this PR is huge already).

The results from the following code is:

 Per iteration:
    Property:2.488ms
    Constructor:3.0828ms
 over 10_000 iterations with a warmup of 50 cycles

Ill attach the patch if anyone wants to check it out (based on this branch)

101-perf-test.patch

PS: This is the core of the measurement code:

    for (int i = 0; i < iterations * 2; i++) {
      if (random.nextInt() % 2 == 0) {
        try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
          final HouseMapper mapper = sqlSession.getMapper(HouseMapper.class);
          final long startTime = System.currentTimeMillis();
          assertThat(mapper.getAllHouses()).isNotNull().hasSize(100);
          times.get(HouseMapper.class).addAndGet(System.currentTimeMillis() - startTime);
        }
      } else {
        try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
          final ImmutableHouseMapper mapper = sqlSession.getMapper(ImmutableHouseMapper.class);
          final long startTime = System.currentTimeMillis();
          assertThat(mapper.getAllHouses()).isNotNull().hasSize(100);
          times.get(ImmutableHouseMapper.class).addAndGet(System.currentTimeMillis() - startTime);
        }
      }
    }

I managed to improve the performance past the point of the property test baseline, Im doing this by only verifying we can create the result once, (per result set) as we can reasonably assume that the next row would have the exact same mapping and thus would not fail if the first verification did not fail.

Im adding two more commits, one with the improvement and the second with the performance test itself. Please let me know if this is too much and I should rather remove the latter.

The basic test also now verifies that the output of property- and constructor based mapping with collections are identical.

EDIT

I wanted to be a bit more scientific about this and used a JMH Benchmark (code on a separate branch based on this one), here are the results:

Benchmark                                                                        Mode  Cnt  Score   Error  Units
PropertyVsConstructorInjectionBenchmark.retrieveAllUsingConstructorInjection     avgt    5  1.015 ± 0.023  ms/op
PropertyVsConstructorInjectionBenchmark.retrieveAllUsingPropertyInjection        avgt    5  1.213 ± 0.069  ms/op
PropertyVsConstructorInjectionBenchmark.retrieveSingleUsingConstructorInjection  avgt    5  0.049 ± 0.002  ms/op
PropertyVsConstructorInjectionBenchmark.retrieveSingleUsingPropertyInjection     avgt    5  0.039 ± 0.001  ms/op

Hi @epochcoder ,

I'm sorry, but could you add new set(s) of test classes/resources instead of modifying existing ones?

The existing tests should pass as-is after the changes in this PR are applied. I understand why you did that, but if you move/modify existing files, it's difficult to verify that.

Thank you!

Hey @harawata, are you referring to the test related to BoundBlogMapper (postConstructorForInit) in BindingTest?

I changed it because the original test itself was flawed in the sense that it was impossible to make it work with the new functionality.

So I had to make the correct resultMap for it, I then changed the test to assert an exception when the flag is false, which is what happened anyway (it was disabled) then made a copy of it with the flag enabled.

But, if this is not the one you are referring to, could you please let me know which ones, and I can do my best to change / rewrite it ;)

Oops, sorry, I was diff-ing wrong commits. Please disregard my last comment.

Oops, sorry, I was diff-ing wrong commits. Please disregard my last comment.

No worries, I understand this PR is absolutely huge, and I'll do my best to make it easier to review ;-)

I forgot to actually cache the value of the experimentalConstructorCollectionMapping flag while building the ResultMap (similar to hasNestedResultMaps and hasNestedQueries), After doing this, performance has increased even more ;-)

Additionally, it does not affect the default case so much anymore.

Benchmark                                                                        Mode  Cnt  Score   Error  Units
PropertyVsConstructorInjectionBenchmark.retrieveAllUsingConstructorInjection     avgt    5  0.834 ± 0.015  ms/op
PropertyVsConstructorInjectionBenchmark.retrieveAllUsingPropertyInjection        avgt    5  1.164 ± 0.055  ms/op
PropertyVsConstructorInjectionBenchmark.retrieveSingleUsingConstructorInjection  avgt    5  0.045 ± 0.001  ms/op
PropertyVsConstructorInjectionBenchmark.retrieveSingleUsingPropertyInjection     avgt    5  0.038 ± 0.002  ms/op

@harawata @hazendaz Is there anything else I can/should do for this PR?

@epochcoder ,

Not at the moment.

I finally found some time to review this, but it's not exactly ready-to-merge state. I'll revisit this as part of v4 enhancements later.

Thanks @harawata , would you mind leaving some comments so I could get it into the desired state, or is that something you prefer we do later?

@epochcoder Does it work with java 16 record https://www.guiadojava.com.br/2021/04/java-records.html ?

@GeorgeSalu Indeed it does, I converted the test cases to use records in the attached patch, and it worked out of the box.

101_use_records.patch

#101 Add Support for Constructor Collection Injection

MyBatis now supports creating completely immutable objects, including nested collections.

To achieve this, another meta object is maintained in memory until it is ready for creation. This detection is based on CacheKey.

This feature is hidden behind a configuration flag called experimentalConstructorCollectionMapping. By default, this flag is set to false, so MyBatis retains its current behavior unless the flag is enabled. This approach allows the community to test different use cases and report possible issues without affecting current workloads.

The new functionality is tested in src/test/java/org/apache/ibatis/immutable/ImmutableConstructorTest.java.

I have also added debug symbols as an additional patch, which helps in understanding the process.

Remaining Tasks:

• Documentation
• Testing with more than two levels of nesting (currently Blog -> Post -> [Comment + Tag])
  • Refer to ImmutableCollectionConstructorTest.java - House -> Room -> Furniture -> Defect
• Testing result ordering
  • Reliable determination of when to build the final object requires ordered results, so this attribute is necessary
• Testing with multiple result sets (currently throws an exception)
  • This is being descoped for now but could be addressed in a follow-up.
• Testing with mixed mappings (including nestedQuery)
  • See postForConstructorInit in BoundBlogMapper.xml
• Testing with very large result sets (performance)

This PR would be very helpful for me! Is there any update?

This PR would be very helpful for me! Is there any update?

sounds like this was candidate for mybatis 4.0.0. So I don't know when it would get into the project. At the moment, those of us working on mybatis are somewhat spread thin with other projects. We are raising up all of mybatis to java 17 soon and doing a small update to 11 from 8 currently. Also addressing some longer standing issues and that all takes a bit of extra time. I'm pretty sure we will have one more java 8 release on the core here before the jump to 11 and then we have a good number of repos to sync up then jump to 17 before we get to this. So it could be a good number of months from now. Its also been a while since anyone looked at this and there are a lot of files so that will again take some time. Sorry for the delay this causes.

I added new tests with less complex objects and some of them are failing. Please take a look and see if you can make them pass.

I haven't looked into the details, but here are a few quick observations.

• The method PendingConstructorCreation.verifyCanCreate() seems to check the existence of constructor, but we can/should blindly call the ObjectFactory. One of the new tests testObjectWithBuilder fails because of the method, I think.
• You can remove the global switch experimentalConstructorCollectionMapping. The condition that sets ResultMap.hasResultMapsUsingConstructorCollection is mostly sufficient. It may need a small adjustment to pass testCollectionArgWithTypeHandler, though.

And, from now on, please avoid force-push as we will heavily rely on diff. You may be aware, but you can merge master to keep this branch up-to-date.

I added new tests with less complex objects and some of them are failing. Please take a look and see if you can make them pass.

I haven't looked into the details, but here are a few quick observations.

• The method PendingConstructorCreation.verifyCanCreate() seems to check the existence of constructor, but we can/should blindly call the ObjectFactory. One of the new tests testObjectWithBuilder fails because of the method, I think.
• You can remove the global switch experimentalConstructorCollectionMapping. The condition that sets ResultMap.hasResultMapsUsingConstructorCollection is mostly sufficient. It may need a small adjustment to pass testCollectionArgWithTypeHandler, though.

And, from now on, please avoid force-push as we will heavily rely on diff. You may be aware, but you can merge master to keep this branch up-to-date.

Agreed, will merge from now on.

• Experimental switch removed
• All other tests fixed, except one: testImmutableNestedObjects()

Please let me know your thoughts.

@harawata Looking at the result for testImmutableNestedObjects , it is being built correctly, we just need to final signal somehow; ~~let me know if I should investigate this~~

I found a way to map these cases, will update the pr.

Thanks @harawata! Also appreciate all the time you have put into this :)

Yeah, the discriminator is definitely a bit more tricky, i.e we won't be able to discriminate results used for constructor injection on the base object itself.