HHH-13586 : ClassCastException when using a single region name for both entity and query results

[gbadner]

https://hibernate.atlassian.net/browse/HHH-13586

[sebersole]

This is probably the best we can do with the API as-is, however I think this change misses the opportunity to change the API to prepare for a proper solution in 6.0. What we are seeing is that both a DomainDataRegion and QueryResultsRegion can be named the same, but that parts of the code still assume that generic access to a Region by name is possible. Basically, I'm not sure there is a time when generalized access to Region by name is needed. And a usage search backs that up. Use of this method from Hibernate itself fall into 2 categories:

1. Calls from within the CacheImplementor itself
2. Calls from Statistics

For CacheImplementor the difference can be easily managed internally. For Statistics, even though it calls #getRegion it actually then handles the return specifically as either a DomainDataRegion or QueryResultsRegion.

So essentially, even though these regions share name they are still different things entirely and really ought to be split into 2 separate storage (Maps) and accessed specifically by type. Again, CacheImplementor itself does currently expose some behavior that might need to look at both types but that is all internal - it can easily access both Maps for these calls.

So I would like to see this changed to be a bit more proactive in fixing the problem, which is partially API based (assuming we want to allow both types to use the same name). I'd suggest the following changes:

1. Add a CacheImplementor#getDomainDataRegion method. Specific access to QueryCacheRegions already exist via CacheImplementor#getDefaultQueryResultsCache, CacheImplementor#getQueryResultsCache and CacheImplementor#getQueryResultsCacheStrictly;
2. Use these methods instead of #getRegion in places where we know we want one type or the other specifically - which, again, outside of the CacheImplementor impls seems to be all of our uses;
3. Add CacheImplementor#getDomainDataRegionNames and CacheImplementor#getQueryCacheRegionNames methods;
4. (?) deprecate #getRegion

[gbadner]

@sebersole, I purposely left SPI/API changes out of this fix, and was intending to follow this up with a separate issue to adress API/SPI changes. I'll look into your suggested changes though, and I'll include that in this jira if it doesn't look like it will break anything.

[sebersole]

Understandable about splitting changes into different commits. A PR of course can contain multiple commits but its fine if you wanted to hold off for that until later. However, IMO this change needs to be done. It won't break anything so long as we simply deprecate #getRegion.

For now you could leave the internals as-is, although IMO this should be implemented through 2 separate Maps inside EnabledCaching. That is how it would make the most sense to implement in 6 with the split methods, and it will work in 5 as well.

It you are not able to make these changes let me know and I will add it.

[gbadner]

@sebersole, I agree with what you wrote.

The only exception is for org.hibernate.Cache#evictRegion. Currently, in this PR, data in both types of regions with the specified name get evicted.

I see that org.hibernate.Cache already has methods for each type of region, so the application can already use these methods instead.

I'm in favor of deprecating both org.hibernate.Cache#evictRegion and CacheImplementor#getRegion

WDYT?

[gbadner]

@sebersole, while I'm at it, what do you think about deprecating CacheImplementor#getCacheRegionNames, since #getDomainDataRegionNames and getQueryCacheRegionNames will be available?

[gbadner]

@sebersole, after looking more closely, I see that Cache#evictRegion really can't be deprecated because the other "evict" methods in Cache do not evict by region name.

I've made the other SPI changes.

I still need to add some tests.

Please take a look and let me know how it looks to you.

[sebersole]

This looks fine to me.

WRT #evictRegion ... it can be deprecated so long as new methods such as #evictDataRegion and #evictQueryRegion are added. Of course it is fine to just leave it also if you prefer, so long as it clears the data from all matching regions by name regardless of the type of data stored there.

[gbadner]

@sebersole, I thought about adding Cache#evictDataRegion and #evictQueryRegion but decided against it because DomainDataRegion and QueryResultsRegion are both SPIs, not APIs. I preferred to keep Cache not knowing about the different types of regions.

Currently, calling Cache#evictRegion will evict all caches with the provided region name.

I realized there was still a little fuzziness to the new methods that I added. I figured it would be best to fix it now.

#getQueryCacheRegionNames included the default QueryResultsRegion region name, but #getQueryResultsCacheStrictly( defaultQueryResultsRegionName ) returns null.

That makes it a bit clumsy for an application to use.

I am changing #getQueryCacheRegionNames to exclude the default QueryResultsRegion region name, since it is intended for use with #getQueryResultsCacheStrictly.

The deprecated getCacheRegionNames will continue to include the default QueryResultsRegion region name, and the deprecated getRegion( defaultQueryResultsRegionName ) will continue return the default QueryResultsRegion.

Going forward, getDefaultQueryResultsCache().getRegion() should be used for getting the default QueryResultsRegion, not the other methods.

I am clarifying the Javadoc so it is more obvious how the various methods deal with the default QueryResultsRegion/QueryResultsCache.

Does this make sense to you in principal?

[gbadner]

Added more test cases and made the changes proposed in previous comment.

[gbadner]

The first 2 commits have be pushed to master.

The remainder have been squashed and pushed to a new PR for HHH-13613: #3029