[SHIRO-816] Hazelcast support does not support HZ v4

[cstamas]

The reason is that getMap method return value changed, and JVM does not find the method (if compiled with HZ3 and runtime uses HZ4 - same would happen other way around).

Fix has two parts: determines IMap class (v3 or v4), and them use MethodHandle to invoke it.

[bmarwell]

What is the benefit over an explicit hz4 implementation? This implementation might slow down hz3 a bit. But furthermore, if we replaced this with hz4 at some point in the future, hz3 would intransparently stop working (at least for Shiro 1.8). Thus, might adding a new module be an option?

Otherwise I'd say yes here, too.

[cstamas]

@bmarwell as I see (in POM), the intent was to seamlessly work with both, HZ 3.x and HZ 4.x (at least that's what import range suggests). But alas, this never worked with both, as AFAIK the IMap package change was present in very first 4.0.x release as well. So, at first, it fixes it. Second, about "slow down", I am unaware of any code that would call getCache at some steady pace/rate. This calls AFAIK happens it majority of cases once in an application (correct me if I'm wrong here). So, while the getCache method is "slowed down", nothing else is slow, so unsure what you refer to "might slow down hz3". Finally, as I wrote in JIRA, consider this "hotfix", makes expectation come to reality in Shiro 1.x line. In 2.x line of Shiro I'd drop HZ3 support completely, and would compile (and run against) Hazelcast 4.x only. Drop the Hazelcast 3.x support in short.

[jherkel]

Shouldn't the import for OSGI be modified as well?

[bmarwell]

@bmarwell as I see (in POM), the intent was to seamlessly work with both, HZ 3.x and HZ 4.x (at least that's what import range suggests). But alas, this never worked with both, as AFAIK the IMap package change was present in very first 4.0.x release as well. So, at first, it fixes it.

Ah, missed that part. Good thing it is now documented here. 😊

Second, about "slow down", I am unaware of any code that would call getCache at some steady pace/rate. This calls AFAIK happens it majority of cases once in an application (correct me if I'm wrong here). So, while the getCache method is "slowed down", nothing else is slow, so unsure what you refer to "might slow down hz3".

Sure, the reflection will only occur once. Nothing to correct here.

Thanks for confirming. LGTM then! 👍🏻

[cstamas]

Shouldn't the import for OSGI be modified as well?

@jherkel hm, now I realize I am wrong, it is currently <hazelcast.osgi.importRange>[3, 4)</hazelcast.osgi.importRange> and see [1] (sorry, am not OSGi guy).

So maybe my whole premise ("intent was to work with HZ 3 AND HZ4") is completely wrong? As it may also mean that SHIRO-816 is a "non issue" actually (should be closed as such: "Shiro 1.x Hazelcast support supports HZ3 only"), given the module was never intended to work with HZ4 in the first place?

As then, IMO proper solution is to

• drop this PR, close out issue SHIRO-816 ("not an issue: 1.x supports HZ3 only")
• Shiro 1.x is fine as is (will not work with HZ4, works with HZ3)
• in shiro to-be-2.0 introduce support for Hazelcast 4.x, drop support for Hazelcast 3.x (could be this very same module but built against Hazelcast 4)
• done :smile:

[1] https://www.eclipse.org/virgo/documentation/virgo-documentation-3.7.0.M01/docs/virgo-user-guide/html/ch02s02.html

[bmarwell]

I think someone came up on slack that this could easily be made compatible with both 3 and 4. That's why I asked for a new hz4 module before that came up.

And yes, as we are having a maintenance release here, I'd suggest: Add hz4 support in main and 1.8 branch Remove hz3 support from main branch Check that hz3 support works well in the 1.8 branch.

WDYT?

[cstamas]

@bmarwell

And yes, as we are having a maintenance release here, I'd suggest: Add hz4 support in main and 1.8 branch Remove hz3 support from main branch Check that hz3 support works well in the 1.8 branch.

IMO (here am talking about current main branch, not this PR changes):

• "maintenance release" should not up major version of a dependency, source code is good as is (this especially if major version of dependency introduces intentional API breakage)
• "add hz4 support" is really just "up the dependency and compile" as then byte code will then work with hz4 (but not with hz3)
• "remove hz3 support from main branch" see above, byte code of this source as-is compiled with hz4 will NOT work with hz3 (just as today compiled against hz3 does NOT work with hz4)
• "check that hz3 support works well in 1.8 branch" it does, I hope tests are in place

Again, IMO here is nothing to be done, but:

• drop this PR
• set HZ version on main branch to hz 4.x
• (maybe) up the HZ version on 1.8 branch to latest hz 3.x
• (maybe) clarify that Shiro 1.x works with hz3 and upcoming Shiro 2.x works with hz4

Personally, I'd not complicate things, by adding burden of "magic" to make something work that was never specd/promised to work. HZ4 did intentional Java package changes, similar to javax.servlet vs jakarta.servlet... so next will be to support those transparently as well?

[bdemers]

Could also tweak the OSGI config and run another set of UT's to run with hz4?

Another option would be to create a new module shiro-hazelcast4 though that gets a little messy as the Shiro v2 version of shiro-hazelcast would also be v4.
An alternative to that would be to make the current module v4 and add a new shiro-hazelcast3, but as @cstamas mentioned that might make for a lot of upgrade confusion.

[bmarwell]

Another option would be to create a new module shiro-hazelcast4

That was my initial idea. Sorry for my brevity, this is what I meant all along.

though that gets a little messy as the Shiro v2 version of shiro-hazelcast would also be v4.

-1. Both Shiro major versions should use shiro-hazelcast4 for hz4 and stick with shiro-hazelcast for hz3. Shiro2 is unreleased. We can change this.

An alternative to that would be to make the current module v4 and add a new shiro-hazelcast3, but as @cstamas mentioned that might make for a lot of upgrade confusion.

Agreed on this one.

[cstamas]

FWIW, I just dropped shiro-hazelcast as dependency from my project: if you consider it, that one class dependency is really an overkill, is not worth it. Hence, am building my CacheManager (is in my sources), so the project does not suffer from this issue. Really, dependency carrying one single class is an overkill IMHO.

[bmarwell]

@cstamas is right. A one-class dependency is probably an overkill for most users. I will suggest to drop it completely, so this becomes a won't fix.

[lprimak]

Will rebase for 1.x though

[lprimak]

@dependabot recreate

[lprimak]

Superseded by #1002