EMAIL-204: Disable eager attachment loading on MimeMessageParser

[HiuKwok]

As discussed on the Jira ticket, this is the MR to update MimeMessageParser.createDataSource() to disable eager loading for email attachments, but instead keep the inputstream as a reference and only load the attachment binary into memory when .getInputStream() is called.

[garydgregory]

Would you please write a unit test that fails without the change to main?

[codecov-commenter]

Codecov Report

Merging #135 (094a2b3) into master (347d5d9) will decrease coverage by 0.07%. The diff coverage is 92.30%.

@@             Coverage Diff              @@
##             master     #135      +/-   ##
============================================
- Coverage     65.90%   65.84%   -0.07%     
- Complexity      305      308       +3     
============================================
  Files            18       19       +1     
  Lines          1053     1054       +1     
  Branches        138      137       -1     
============================================
  Hits            694      694              
- Misses          280      281       +1     
  Partials         79       79              

Impacted Files	Coverage Δ
...g/apache/commons/mail/LazyByteArrayDataSource.java	91.66% <91.66%> (ø)
...rg/apache/commons/mail/util/MimeMessageParser.java	84.14% <100.00%> (-1.88%)	:arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[HiuKwok]

@garydgregory Can you also advise on how unit can be written in this case? As this is a performance improvement rather than a bug fix, hence this is no change of behavior from an external perspective I can assert. I'm thinking to create a huge .eml file with an attachment in order to trigger an OOM exception as the original approach will load the whole file into the memory, which the revised method only loads a portion of the file into the input stream. But I don't think that makes sense to push a huge .eml under the test resources folder either.

[HiuKwok]

Actually, I can probably create a test to assert the object size of mimeMessageParser, the memory consumption should reduce after the revised code change.

[garydgregory]

Would mocking help here?

[HiuKwok]

I reckon so, to have a test check on .getInputStream() metho invoke count and make sure it is only called when the user is accessing the attachment content. I will update this MR once again with the test.

[HiuKwok]

This is not yet ready, I'm working on the test-cases

[HiuKwok]

@garydgregory I have updated the code change once again with two new test cases to represent the scenarios of lazy loading of Email attachments with appropriate mocking. Would that be possible for you to review it again? Appreciate that.

[HiuKwok]

I believe all comments are addressed, let me know if I missed any, thanks!

[garydgregory]

@HiuKwok The build fails. Run 'mvn' on the command line before you push. Or, did you not see failures on your set up? OS? Java version?

[HiuKwok]

Yes, my local build is green and I tried the exact commands used for the failed builds, which all build successfully (I couldn't reproduce this on my local). By checking on the error message it seems related to the Easymock, let me investigate, and will update you once again on this thread.

BTW my setup: OS: MAC OS 12.15.1 Java: Open JDK 17.0.2 MVN: 3.8.5

[garydgregory]

I would try locally with Java 8.

[HiuKwok]

Since it failed on Java 8 test, I will install Java8 locally and rerun the test.

[HiuKwok]

@garydgregory I can now reproduce the issue after running the unit test locally with jdk8. I will check and update the test case accordingly if need. Thx for the help!

[HiuKwok]

It turns out the easymock is having issues mocking the class DataHandler, for now, I have replaced the mock of the class with a constructor instead.

final DataHandler dataHandler = new DataHandler(dataSource);

I have re-run the test suite locally against JDK8, 11 and 17 and all tests are passing for all three versions :)

[HiuKwok]

@garydgregory all comments are addressed and I have fixed the test cases. Would that be possible to have another round of review?

Thanks,

[garydgregory]

Hello @HiuKwok,

I am sorry about not getting back to you sooner. I belive this functionality is currently implemented (see InputStreamDataSource), albeit differently, in git master. Please verify your use case and close this or update this PR as appropriate.

Thank you for your patience! 😄