Upgrade to Spring Boot 1.5.14 and 2.0.3

[jdubois]

Important CVE are fixed: https://spring.io/blog/2018/06/14/spring-project-vulnerability-reports-published

For JHipster 4 we will need to wait for the new Spring Platform BOM (should be Brussels-SR11) but it's not available yet on http://search.maven.org/#search%7Cgav%7C1%7Cg%3A%22io.spring.platform%22%20AND%20a%3A%22platform-bom%22

[DanielFran]

Brussels-SR11 expected tomorrow!

[jdubois]

For Spring Boot 2.0.3 the build is currently failing, it looks like they changed the way Ehcache is configured, I'm on it.

[atomfrede]

I tried to update a 4.x app to 1.5.14 this morning and had also problems with the cache configuration. Somehow it seems the order has changed again as it fails because ehcache is configured after datasource again. Works with 1.5.13 and applying your fix from this commit: 843a671

[jdubois]

Oh and my trainees also had this problem recently - what is happening here, this is making everything fails :-(
Thanks for the link to the commit!

[jdubois]

Nope @atomfrede - this is probably something close, but the fix doesn't work for me... In fact I have 2 caches, one created before (the correct one, with our setup), and then another one. It looks like Hibernate doesn't see the existing Ehcache instance and creates a second one.

[jdubois]

So for Spring Boot 2.0.3 this fails because we use our own io.github.jhipster.config.jcache.NoDefaultJCacheRegionFactory for the Hibernate hibernate.cache.region.factory_class property.

The idea is that if you don't configure your cache for each entity, it won't be cached. So our class is a hack that makes the app crash if those caches are not configured correctly, which I find is a good idea.

Then that class is kind of a hack, and for some reason it doesn't work anymore. I still find the idea very good, then it's also a bit against our Policy 1 - even if in that case the default config is pretty stupid (making you think your class is cached, when it is not...).

I'll have a closer look, but otherwise let's just remove this.

[jdubois]

This is so buggy...
So removing hibernate.cache.region.factory_class was a bad idea:

• Everything is correct in the code (the @Cache annotations, etc)
• At start up you have logs telling that the caches are created
• But in fact nothing goes into the cache!!!! It's just ignored

That's a really bad default configuration from Spring Boot! So let's not do this, and I'm stuck again :-(

[jdubois]

I think this part was coded by @henri-tremblay - Henri, does that ring a bell to you?

[jdubois]

A quick summary as I don't know if I'll have more time on this today.

With this new Spring Boot 2.0.3 release:

• We do have CacheConfiguration correctly setting up the caches, using JSR 107
• But the Hibernate entity manager does not "see" that JSR 107 instance, so it creates a new one, in which the caches are not correctly created (this second instance has no cache configured)
• As we have io.github.jhipster.config.jcache.NoDefaultJCacheRegionFactory which is checking for non-configured cache (read http://www.ehcache.org/blog/2017/03/15/spontaneous-cache-creation.html to understand why this is a good idea), this creates an error in this second JSR 107 instance, and this makes the application crash

So:

• io.github.jhipster.config.jcache.NoDefaultJCacheRegionFactory is a really great thing, we should definitely keep it
• We need to make the Hibernate entity manager "see" the existing instance, like it used to work before. I have no idea how to do that at the moment.

Any help here would be greatly appreciated!

[atomfrede]

And I think its the same for 1.5.14 at least thats what I can see in the logs (without investigating any further)

[henri-tremblay]

Hi. Just waked up :-) I will have a look today. NoDefaultJCacheRegionFactory is indeed a good thing. It is actually the default in the latest Hibernate version so we will be able to get rid of it. The JSR107 CachingProvider is a singleton. If Hibernate and Spring are not seeing the same cache manager, it means both are not on the same URI anymore.

[jdubois]

Thanks @henri-tremblay !! Indeed that's supposed to be a singleton, but caches are initialized twice, here are my logs if I use a org.hibernate.cache.jcache.JCacheRegionFactory instead of the NoDefaultJCacheRegionFactory:

2018-06-15 15:29:06.481 DEBUG 55270 --- [  restartedMain] c.ehcache.core.Ehcache-usersByLogin      : Initialize successful.
2018-06-15 15:29:06.509 DEBUG 55270 --- [  restartedMain] c.ehcache.core.Ehcache-usersByEmail      : Initialize successful.
2018-06-15 15:29:06.513 DEBUG 55270 --- [  restartedMain] c.e.c.E.mycompany.myapp.domain.User      : Initialize successful.
2018-06-15 15:29:06.517 DEBUG 55270 --- [  restartedMain] c.e.c.E.m.myapp.domain.Authority         : Initialize successful.
2018-06-15 15:29:06.521 DEBUG 55270 --- [  restartedMain] c.e.c.E.m.myapp.domain.User.authorities  : Initialize successful.
2018-06-15 15:29:06.525 DEBUG 55270 --- [  restartedMain] c.e.c.E.mycompany.myapp.domain.Foo       : Initialize successful.
2018-06-15 15:29:06.688 DEBUG 55270 --- [  restartedMain] c.m.myapp.config.DatabaseConfiguration   : Configuring Liquibase
2018-06-15 15:29:06.811  WARN 55270 --- [mple-Executor-1] i.g.j.c.liquibase.AsyncSpringLiquibase   : Starting Liquibase asynchronously, your database might not be ready at startup!
2018-06-15 15:29:07.511 DEBUG 55270 --- [mple-Executor-1] i.g.j.c.liquibase.AsyncSpringLiquibase   : Liquibase has updated your database in 698 ms
2018-06-15 15:29:07.627 DEBUG 55270 --- [  restartedMain] c.e.c.E.mycompany.myapp.domain.User      : Initialize successful.
2018-06-15 15:29:07.674 DEBUG 55270 --- [  restartedMain] c.e.c.E.mycompany.myapp.domain.Foo       : Initialize successful.
2018-06-15 15:29:07.678 DEBUG 55270 --- [  restartedMain] c.e.c.E.m.myapp.domain.Authority         : Initialize successful.
2018-06-15 15:29:07.816 DEBUG 55270 --- [  restartedMain] c.e.c.E.m.myapp.domain.User.authorities  : Initialize successful.

-> the first Initialize successful lines come from our CacheConfiguration, then Liquibase kicks in, and then it's Hibernate that is starting its own cache, giving identical Initialize successful lines, for caches that are created on the fly. That's also why you only have the first 2 lines (usersByLogin and usersByEmail) only on the first pass, as those are Spring Cache only caches (not JPA entities).

In that configuration, the second caches are used, so it looks like it's working, but it's not using the caches that we have configured in CacheConfiguration, which is wrong. And hard to spot if you don't look at the logs!

I'm sorry but I won't have much time this week-end on this, and this is of course very important, so help is really, really welcome.

[henri-tremblay]

Ok. And where are you testing it? To you have a branch or a test project I can try? Only the Spring-boot version was changed? Not the hibernate one for instance?

[jdubois]

I'm using the master branch from "jhipster/jhipster" and do a "mvn clean install" so I have version 2.0.12 in my local Maven repo.
Then I generate a project and upgrade JHipster dependencies in the pom.xml (just one line to point to 2.0.12).
I'm doing this from my phone, so not sure it's 100% correct, but this is the general idea.

[henri-tremblay]

So. The problem is there because Spring and Hibernate are no longer using the same class loader to find their CacheManager. Now the question is "why?". Which I will investigate right away.

[henri-tremblay]

Ok. Here is the spring-cache code that broke everything: spring-projects/spring-boot/pull/13338. However, the fix seems good so the problem is now to pass the right classloader to hibernate.

@snicoll You never experienced this in one of your samples?

[henri-tremblay]

Here are two solutions. They are not pretty but they work. Due to the fact that Hibernate can't take a Spring bean as a region factory, I don't see what else I can do unless something is modified in Hibernate or Spring (not sure what though).

Solution 1: Just guess the class loader by taking the one of the RegionFactory instance. Since Spring is loading Hibernate, in general it should work.

public class ClassLoaderFixJCacheRegionFactory extends NoDefaultJCacheRegionFactory {

    @Override
    protected CacheManager getCacheManager(Properties properties) {
        CachingProvider cachingProvider = getCachingProvider( properties );
        String cacheManagerUri = getProp( properties, CONFIG_URI );
        ClassLoader classLoader = getClass().getClassLoader();

        URI uri = getUri(cachingProvider, cacheManagerUri);
        return cachingProvider.getCacheManager(uri, classLoader);
    }

    private URI getUri(CachingProvider cachingProvider, String cacheManagerUri) {
        URI uri;
        if (cacheManagerUri != null) {
            try {
                uri = new URI(cacheManagerUri);
            }
            catch (URISyntaxException e) {
                throw new CacheException("Couldn't create URI from " + cacheManagerUri, e);
            }
        }
        else {
            uri = cachingProvider.getDefaultURI();
        }
        return uri;
    }
}

Solution 2: Take the real spring class loader and hide it in a static variable.

public class BeanClassLoaderAwareJCacheRegionFactory extends NoDefaultJCacheRegionFactory {

    private static volatile ClassLoader classLoader;

    @Override
    protected CacheManager getCacheManager(Properties properties) {
        Objects.requireNonNull(classLoader, "Please configure this class as a Spring Bean to get a class loader correctly");

        CachingProvider cachingProvider = getCachingProvider( properties );
        String cacheManagerUri = getProp( properties, CONFIG_URI );

        URI uri = getUri(cachingProvider, cacheManagerUri);
        CacheManager cacheManager = cachingProvider.getCacheManager(uri, classLoader);
        
        // To prevent some class loader memory leak this might cause
        setBeanClassLoader(null);
        
        return cacheManager;
    }

    private URI getUri(CachingProvider cachingProvider, String cacheManagerUri) {
        URI uri;
        if (cacheManagerUri != null) {
            try {
                uri = new URI(cacheManagerUri);
            }
            catch (URISyntaxException e) {
                throw new CacheException("Couldn't create URI from " + cacheManagerUri, e);
            }
        }
        else {
            uri = cachingProvider.getDefaultURI();
        }
        return uri;
    }

    public static void setBeanClassLoader(ClassLoader classLoader) {
        BeanClassLoaderAwareJCacheRegionFactory.classLoader = classLoader;
    }
}

The second solution is uglier but will always work.

[jdubois]

Thank you so much for the analysis! I'll have a look on Sunday (no computer ATM and this needs testing).
I'm worried the static variable could cause issues on an app server with multiple WARs, but who does this nowadays?

[henri-tremblay]

Yes. But this is for a little while. So it causes issues only if you are crazy enough to have a factory in a class loader common to two wars that are deployed at the same moment. The alternative is to use a thread local and assume Spring is always loaded in a single thread.

[snicoll]

Here is the spring-cache code that broke everything

If you have a sample that shows how it breaks, I am happy to revisit the code.

@snicoll You never experienced this in one of your samples?

We don't have support for 2nd level cache in Spring Boot and, as far as I understand, this is why you're having this issue. I'd be happy to include such support in Spring Boot itself but never had the time to look at it (PR welcome).

IMO, the code that was changed is better than the previous version. I might change my mind looking at the specifics though.

[henri-tremblay]

Thanks, @snicoll. Yes. It's not a Spring issue. Using this class loader make sense. The real problem is a mix of JSR 107 playing with class loaders and Hibernate factory that can't be injected from a Spring bean.

@jdubois A stupid solution could be to remove second level caching from Hibernate by arguing that the Spring cache layer should be enough.

[jdubois]

I'm quite happy that JHipster provides Hibernate 2nd level cache on top of Spring Boot :-)

For the record, for the last month, for all JHipster generated projects:

• 80% of all projects used an SQL database
• 70% of all projects used Hibernate 2nd level cache (which means 87,5% of projects using an SQL database used an Hibernate 2nd level cache)
• 80% of all projects used Spring Cache (usually with Hibernate 2nd level cache, but not always - for example for MongoDB users)
• 52% of all projects used Ehcache!

[jdubois]

Concerning the solutions: @henri-tremblay I guess solution 2 is better, as you said it will always work. It will be hidden in our library, so it's not too bad if the code isn't perfect.

We'll probably also need this for Spring Boot 1.4, so I'll backport it there.

I'm testing this!

[jdubois]

@henri-tremblay solution 1 in fact doesn't work with my setup - I don't understand why...
For solution 2, my problem is that it needs to be configured as a Spring Bean: if we auto configure it in the jhipster/jhipster lib, we would then need to add a property to disable it if needed. So in the end this is quite a complex configuration.
So at the moment I'd rather have solution 1, but I need to make it work on my machine.

-> if you have the time, could you do a PR? That way you would still be the author of the code, and that would prove that it works (through Travis)

[henri-tremblay]

I much prefer solution 2. And I think it is easier to convert into a PR to Spring. Right now I was injecting it as a bean in CacheConfiguration. But yes, I will do a PR.

[jdubois]

Yes you are right @henri-tremblay !

About solution 1, I found my issue: I was putting the class into the jhipster/jhipster lib, so it wasn't in the right classloader. I had to move my class into the project, so it has to be generated by jhipster/generator-jhipster, and that's indeed awful.

So I'd much prefer solution 2, if it can be put inside jhipster/jhipster, but that looks a bit hard to do. I'll also try to do it.

[jdubois]

@henri-tremblay got it... It's not even a Spring Bean... Really ugly code, but very easy to use from the user's point of view, and no risk to fail. I'll commit this and let you tell me what you think of it.

[jdubois]

For the record, a modified version of solution 2 was commited in jhipster/jhipster at jhipster/jhipster@662096a

[deepu105]

@jdubois but for the 4.x branch there shouldn't be any React test config in Travis right as its a different branch?

[deepu105]

can this ticket be closed?

[jdubois]

@deepu105 not sure what happens with Travis, but I don't think this is very important as the v4 branch is in maintenance mode now.

And I'll close the ticket as soon as I finish a couple of tests.

[henri-tremblay]

For the record, I think the final solution will be to configure Hibernate programmatically. I will look into it but if someone is an expert, I'm all ears.

[henri-tremblay]

For the record, I think the best solution would be to configure Hibernate programmatically and inject a RegionFactory that is Spring aware. I'll have a look but if some Hibernate+Spring wants to, I'm all ears.

[DanielFran]

@henri-tremblay I am working in a PR to migrate to spring-boot 2.1.0.x (M1 for now, but probably M2 next week).
I see now that spring migrated actually to hibernate 5.3.3.Final and as you know well the JCacheFactory has been changed (https://hibernate.atlassian.net/browse/HHH-12702?jql=project%20%3D%20HHH%20AND%20fixVersion%20in%20(5.2.18%2C%205.3.0.Beta1%2C%205.3.0.Beta2%2C%205.3.0.CR1%2C%205.3.0.CR2%2C%205.3.1%2C%205.3.2%2C%205.3.3%2C%205.3.4%2C%205.3.5)%20AND%20component%20%3D%20hibernate-jcache%20ORDER%20BY%20updated).

Since the current implementation in jhipster project is not complaint anymore, I would like to confirm with you what should be the new solution to avoid spontaneous cache creation?

See: https://github.com/DanielFran/jhipster/tree/spring-boot-2.1.0

Thanks

[henri-tremblay]

@DanielFran I will have a look this week.

[henri-tremblay]

So. We can remove the NoDefaultJCacheRegionFactory and replace it with a configuration hibernate.javax.cache.missing_cache_strategy=fail to keep the current configuration. Hibernate uses create-warn as a default right now. To have a "still works but warns you"behaviour to be nice to their users. That could work as well depending on how hard we want to be on it.

It means that BeanClassLoaderAwareJCacheRegionFactory can now inherit JCacheRegionFactory. And be replace by the following code:

public class BeanClassLoaderAwareJCacheRegionFactory extends JCacheRegionFactory {

    private static volatile ClassLoader classLoader;

    /**
     * This method must be called from a Spring Bean to get the classloader.
     * For example: BeanClassLoaderAwareJCacheRegionFactory.setBeanClassLoader(this.getClass().getClassLoader());
     *
     * @param classLoader The Spring classloader
     */
    public static void setBeanClassLoader(ClassLoader classLoader) {
        BeanClassLoaderAwareJCacheRegionFactory.classLoader = classLoader;
    }

    @Override
    protected ClassLoader getClassLoader(CachingProvider cachingProvider) {
        return Objects.requireNonNull(classLoader, 
            "Please set Spring's classloader in the setBeanClassLoader method before using this class in Hibernate");
    }
}

I can PR on master if it is now appropriate.

Also, that said, it should actually be possible to get rid of BeanClassLoaderAwareJCacheRegionFactory but I failed so far. Hibernate allows to plug things into their SPI so a SpringAwareJCacheRegionFactory should be possible. This class would be a Spring bean knowing about the right class loader to use. If someone move versed into Hibernate SPI wants to have a look, feel free.

[DanielFran]

Hi @henri-tremblay.

Thanks for your feedback.

I commited my changes in jhipster/jhipster#88 and included your suggestion.

Do you want I remove it and you PR yourself those changes?

[henri-tremblay]

No that's all right. I will survive not being the committer :-)

I think EXCEPTION_MESSAGE could be package scope instead of public.

I wasn't aware of the LogbackRecorder which is pretty handy. Cool!

[DanielFran]

Thanks for all your comments, I made the changes.

[DanielFran]

@jdubois @henri-tremblay Maybe the issue with the Cache 'order' will be fixed in next release: see spring-projects/spring-boot#14181

[jdubois]

@henri-tremblay @DanielFran I also add a try this morning on our spring-boot_2.1.0 branches, and indeed the L2 cache isn't configured properly.

• We have warnings like Missing cache[com.mycompany.myapp.domain.User.authorities] was created on-the-fly. The created cache will use a provider-specific default configuration: make sure you defined one. You can disable this warning by setting 'hibernate.javax.cache.missing_cache_strategy' to 'create'.
• The caches are correctly configured (I can see them in JMX), but are not used by Hibernate, it seems like it doesn't "see" the configured caches, and hence creates new ones

I don't know what the issue is here, but @henri-tremblay you seem to understand this very well, so help is really welcome. And if you want contributor access that can also be easily solved, if you want to be a "core dev team" member.

[DanielFran]

For reference, this is the changes applied: jhipster/jhipster@bf25417

[henri-tremblay]

Ok. I'll try to have a look today. This comes from changes discussed with Hibernate team. I need to put it back in my head and it shouldn't be too hard to fix.

[henri-tremblay]

@jdubois I tried spring-boot_2.1.0 but failed to compile the generated code. It's looking for io.github.jhipster:jhipster-dependencies:pom:2.1.0-SNAPSHOT

[DanielFran]

@henri-tremblay This is normal, no version of jhipster-bom has been released from spring-boot 2.1.0 branch.

You can download specific spring-boot jhipster project and compile it: https://github.com/jhipster/jhipster/tree/spring-boot_2.1.0

[henri-tremblay]

Ok. I was looking at the wrong jhipster-dependencies project. Now it's better. However, jhipster-framework is currently failing. error reading ~/.m2/repository/io/undertow/undertow-core/2.0.14.Final/undertow-core-2.0.14.Final.jar; zip file is empty

[jdubois]

You probably had a corrupted download, can you delete it in your Maven cache?

[henri-tremblay]

Ok. Don't worry about it. I fixed it. Not sure if it's an actual issue or my Maven repository that was unhappy.

[henri-tremblay]

@jdubois Have you dropped support for yarn?

[jdubois]

No but we changed the default, it's NPM by default now

[henri-tremblay]

But mvn won't use yarn anymore. :-( Ok

[pascalgrimaud]

@henri-tremblay : you can add the flag like this: jhipster --yarn so it will generate a JHipster project with Yarn instead of NPM :)

[henri-tremblay]

Ok. The problem comes from a "bug" in JCacheRegionFactory. I've created hibernate/hibernate-orm/pull/2656 to solve it.

Meanwhile, I will find a fix.

[henri-tremblay]

Here is a quick fix. However, I'm looking at how to simplify a bit to get rid of BeanClassLoaderAwareJCacheRegionFactory.