Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/optional jcache #198

Merged
merged 10 commits into from
Nov 2, 2017
Merged

Feature/optional jcache #198

merged 10 commits into from
Nov 2, 2017

Conversation

benfortuna
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Nov 2, 2017

Codecov Report

Merging #198 into develop will increase coverage by 0.19%.
The diff coverage is 75%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop     #198      +/-   ##
=============================================
+ Coverage      65.13%   65.33%   +0.19%     
- Complexity      1794     1823      +29     
=============================================
  Files            282      285       +3     
  Lines           7990     8030      +40     
  Branches        1149     1150       +1     
=============================================
+ Hits            5204     5246      +42     
+ Misses          2388     2385       -3     
- Partials         398      399       +1
Impacted Files Coverage Δ Complexity Δ
.../fortuna/ical4j/model/TimeZoneRegistryFactory.java 100% <100%> (+37.5%) 3 <1> (ø) ⬇️
...n/java/net/fortuna/ical4j/util/DecoderFactory.java 100% <100%> (+37.5%) 3 <1> (ø) ⬇️
...va/net/fortuna/ical4j/util/CompatibilityHints.java 87.5% <100%> (ø) 5 <0> (ø) ⬇️
...net/fortuna/ical4j/data/CalendarParserFactory.java 100% <100%> (+22.22%) 3 <1> (ø) ⬇️
src/main/java/net/fortuna/ical4j/model/Recur.java 78.26% <100%> (+0.25%) 150 <1> (ø) ⬇️
...c/main/java/net/fortuna/ical4j/util/TimeZones.java 70% <100%> (ø) 6 <2> (ø) ⬇️
...n/java/net/fortuna/ical4j/util/EncoderFactory.java 100% <100%> (+37.5%) 3 <1> (ø) ⬇️
.../java/net/fortuna/ical4j/model/TimeZoneLoader.java 71.61% <38.46%> (+0.52%) 21 <2> (ø) ⬇️
...ain/java/net/fortuna/ical4j/util/Configurator.java 57.14% <52.17%> (-12.09%) 8 <5> (+5)
...java/net/fortuna/ical4j/util/MapTimeZoneCache.java 83.33% <83.33%> (ø) 5 <5> (?)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7416495...3535a5d. Read the comment docs.

@benfortuna benfortuna merged commit d04e0e0 into develop Nov 2, 2017
@benfortuna benfortuna deleted the feature/optional-jcache branch November 2, 2017 05:09
@rfc2822
Copy link
Contributor

rfc2822 commented Nov 2, 2017

Thank you!

Without an ical4j.properties (I have configured it within a static code block), I get a NullPointerException because CONFIG seems to be null:

java.lang.ExceptionInInitializerError
    at net.fortuna.ical4j.util.Configurator.getProperty(Configurator.java:75)
    at net.fortuna.ical4j.util.CompatibilityHints.<clinit>(CompatibilityHints.java:87)
    at net.fortuna.ical4j.util.CompatibilityHints.setHintEnabled(CompatibilityHints.java:107)
    at at.bitfire.ical4android.iCalendar.<clinit>(iCalendar.kt:36)
    at at.bitfire.davdroid.resource.LocalEvent.<clinit>(LocalEvent.kt:26)
...
 Caused by: java.lang.NullPointerException: Attempt to invoke virtual method 'int java.io.Reader.read(char[])' on a null object reference
    at java.util.Properties$LineReader.readLine(Properties.java:435)
    at java.util.Properties.load0(Properties.java:354)
    at java.util.Properties.load(Properties.java:342)
    at net.fortuna.ical4j.util.Configurator.<clinit>(Configurator.java:58)
    at net.fortuna.ical4j.util.Configurator.getProperty(Configurator.java:75) 
    at net.fortuna.ical4j.util.CompatibilityHints.<clinit>(CompatibilityHints.java:87) 
    at net.fortuna.ical4j.util.CompatibilityHints.setHintEnabled(CompatibilityHints.java:107) 
    at at.bitfire.ical4android.iCalendar.<clinit>(iCalendar.kt:36) 
    at at.bitfire.davdroid.resource.LocalEvent.<clinit>(LocalEvent.kt:26) 
...

Using an ical4j.properties file instead of static configuration works as expected.

The same for loading custom aliases from tz.alias, which causes a NullPointerException when the file is not present (this can be easily worked around with an empty file, too).

@rfc2822
Copy link
Contributor

rfc2822 commented Nov 2, 2017

Another problem is that Optional.orElse() is called with an object argument instead of a lambda function:

return property.orElse(new JCacheTimeZoneCache());

This causes new JCacheTimeZoneCache() to be evaluated even when property is already set (to the Map config cache, for instance). This constructor call looks for a JCache implementation, which of course fails when there is no JCache implementation:

Caused by: javax.cache.CacheException: No CachingProviders have been configured
at javax.cache.Caching$CachingProviderRegistry.getCachingProvider(Caching.java:381)
at javax.cache.Caching$CachingProviderRegistry.getCachingProvider(Caching.java:351)
at javax.cache.Caching.getCachingProvider(Caching.java:142)
at net.fortuna.ical4j.util.JCacheTimeZoneCache.<init>(JCacheTimeZoneCache.java:15)

Then it throws a CacheException, which is not catched.

So I suggest to use Optional.orElseGet() [https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html#orElseGet-java.util.function.Supplier-] instead. Using an anonymous Supplier object as lambda pattern will avoid calling the new JCacheTimeZoneCache() constructor.

@benfortuna
Copy link
Member Author

Ok, thanks for pointing out the bug. I've put in a fix now as you suggested - I actually implemented my own Optional class to avoid a dependency on java 8, but wasn't hard to add Supplier also.

I think I fixed the null pointer issue also when ical4j.properties doesn't exist. Let me know if you see further issues.

@omarkilani
Copy link
Contributor

omarkilani commented Nov 6, 2017

Hi Ben,

We updated from 2.1.0 to 2.1.3 and started seeing this:

Exception in thread "pool-5-thread-16" java.lang.NoClassDefFoundError: javax/cache/configuration/Configuration
        at net.fortuna.ical4j.model.TimeZoneLoader$2.get(TimeZoneLoader.java:283)
        at net.fortuna.ical4j.model.TimeZoneLoader$2.get(TimeZoneLoader.java:280)
        at net.fortuna.ical4j.util.Optional.orElseGet(Optional.java:31)
        at net.fortuna.ical4j.model.TimeZoneLoader.cacheInit(TimeZoneLoader.java:280)
        at net.fortuna.ical4j.model.TimeZoneLoader.<init>(TimeZoneLoader.java:80)
        at net.fortuna.ical4j.model.TimeZoneRegistryImpl.<init>(TimeZoneRegistryImpl.java:125)

Is there some way to opt out of this jcache dependency?

@benfortuna
Copy link
Member Author

Hi Omar,

Yes you can opt out of the jcache dependency by adding the following entry to your configuration (i.e. ical4j.properties):

net.fortuna.ical4j.timezone.cache.impl=net.fortuna.ical4j.util.MapTimeZoneCache

This will override the default caching implementation to use a simple HashMap instead of the more sophisticated Jcache implementation.

The alternative is to just add a Jcache implementation (such as ehcache) to your dependencies.

@stellingsimon
Copy link

For those trying to configure the cache implementation: Note that ical4j-2.1.1 uses JCache regardless of what is configured in net.fortuna.ical4j.timezone.cache.impl. I didn't test ical4j-2.1.2.

@omarkilani
Copy link
Contributor

@benfortuna is there any chance you could be convinced to default to MapTimeZoneCache if there's no ical4j.properties found? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants