GROOVY-11578: provide default `FastStringService` in case of service loader failure
@paulk-asert could we already go for this workaround for the linked ticket, it should not break something and will at least fix the groovy-json bundle which is the only one specifically with this kind of logic where it searches for a service loader or else returns null
Codecov Report
:x: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 67.0074%. Comparing base (d0540bc) to head (ac2ce2a).
:warning: Report is 69 commits behind head on master.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| ...g/apache/groovy/json/internal/FastStringUtils.java | 0.0000% | 0 Missing and 1 partial :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## master #2330 +/- ##
==================================================
+ Coverage 67.0045% 67.0074% +0.0028%
- Complexity 29330 29357 +27
==================================================
Files 1382 1382
Lines 116613 116623 +10
Branches 20432 20445 +13
==================================================
+ Hits 78136 78146 +10
- Misses 32038 32041 +3
+ Partials 6439 6436 -3
| Files with missing lines | Coverage Δ | |
|---|---|---|
| ...g/apache/groovy/json/internal/FastStringUtils.java | 55.5556% <0.0000%> (-5.5555%) |
:arrow_down: |
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
@eric-milles would you be able to review this :?
@eric-milles can you recheck with my smaller list of changes :)?
You can probably remove the null check on lines 55-57. Is there any hope for a test case?
If the DefaultFastStringService comes from the groovy-json module, is it supposed to be available via service loading? Sorry, I'm a bit behind in the discussion.
@eric-milles yes, it should become available through Service Loading but there are two issues:
- Now that groovy-json is set as a Fragment, it does not work anymore, only Bundles in OSGi will be loaded through service loading, hence the change of removing the Fragment-Host property that was initially part of this PR
- There can be a race condition, where the Bundle is still starting up and registering it's service loaders (which happens sort of async), but the groovy-json class FastStringUtils is already called, meaning that it doesn't find the service loader at that moment. So the code how it's written now is never going to work in that case because it only allows you to get the string service instance once. I hope this will be resolved in https://github.com/apache/felix-dev/pull/455 and https://github.com/osgi/osgi/pull/882
I will check if I find some time to add test cases :) I expect it will be a simple test hehe.
@eric-milles test case added :)