Skip to content

[SHIRO-849] Adds cache module for JCache #332

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.

Sign up for GitHub

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

Jump to bottom
Closed
bdemers wants to merge 2 commits into from
Closed

[SHIRO-849] Adds cache module for JCache #332

bdemers wants to merge 2 commits into from

Conversation

@bdemers
Copy link
Member

@bdemers bdemers commented Jan 14, 2022 •
edited
Loading

This will allow for any JCache implementation to work with Shiro, as well as new versions of EhCache & Hazelcast.

Fixes: SHIRO-816
Fixes: SHIRO-479

NOTE: This currently targets 1.8 (which would need to be bumped to 1.9 before merging)
Going forward we could replace org.apache.shiro.cache with JCache, and update MemoryConstrainedCacheManager to be a safe fallback.

@bdemers
Adds cache module for JCache
74b1a1d 
This will allow for any jcache implementation to work with Shiro, as well as new erversions of EhCache & Hazelcast.

Fixes: SHIRO-816
Fixes: SHIRO-813
@bdemers
Add missing license header
1aa455e
bmarwell
bmarwell reviewed Jan 24, 2022
View reviewed changes
Copy link
Contributor

@bmarwell bmarwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can open an issue, I'd say let's go for 1.9 or 1.10! :)

@bdemers
Copy link
Member Author

bdemers commented Jan 24, 2022

Created issue: https://issues.apache.org/jira/browse/SHIRO-849
(fix version left unset for now, but we can fill it in once we move forward)

@bdemers bdemers changed the title Adds cache module for JCache [SHIRO-849] Adds cache module for JCache Jan 24, 2022
@fpapon fpapon self-requested a review February 14, 2022 04:06
@lprimak
Copy link
Contributor

lprimak commented Jan 26, 2023

LGTM. Can we move this forward? Thanks!

@bdemers
Copy link
Member Author

bdemers commented Jan 26, 2023

I'm not sure what the osgi related config should be: https://github.com/apache/shiro/pull/332/files#diff-fe1d5bdae7660405b02de35c849a3f235600c2e7c318f64b8635accfabf4b1abR77-R81

I'm guessing the above is wrong (given the hazelcast package name) anyone want to take a pass at that part?
After that I can clean it up (update the dependencies) and retarget main (potentially backporting where needed)

@bmarwell
Copy link
Contributor

bmarwell commented Jan 27, 2023

Maybe we could replace synchronized(this) with an AtomicWrapper?

@fpapon
Copy link
Member

fpapon commented Jan 27, 2023

I'm not sure what the osgi related config should be: https://github.com/apache/shiro/pull/332/files#diff-fe1d5bdae7660405b02de35c849a3f235600c2e7c318f64b8635accfabf4b1abR77-R81

I'm guessing the above is wrong (given the hazelcast package name) anyone want to take a pass at that part? After that I can clean it up (update the dependencies) and retarget main (potentially backporting where needed)

Let me take a look, we just need to import the jcache package to not depend on jcache implementation. For Hazelcast, user needs to install the hazelcast jcache osgi bundle.

I will make some tests.

@fpapon
Copy link
Member

fpapon commented Jan 27, 2023

And agree, I will work on main branch first.

@fpapon fpapon mentioned this pull request Jan 29, 2023
Merged
fpapon
fpapon reviewed Jan 29, 2023
View reviewed changes
Copy link
Member

@fpapon fpapon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be closed, I submitted a new PR to replace this one:

  • Rebase with 1.11.x
  • Update osgi metadata
  • Add Karaf feature

#696

support/jcache/pom.xml
<Export-Package>org.apache.shiro.jcache*;version=${project.version}</Export-Package>
<Import-Package>
org.apache.shiro*;version="${shiro.osgi.importRange}",
com.hazelcast*;version="${jcache.osgi.importRange}",
Copy link
Member

@fpapon fpapon Feb 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot see where we are using the com.hazelcast package, I think it's more like javax.cache*

@fpapon fpapon self-requested a review January 29, 2023 18:30
fpapon
fpapon requested changes Jan 29, 2023
View reviewed changes
Copy link
Member

@fpapon fpapon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See last comment

@fpapon fpapon closed this Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bmarwell bmarwell bmarwell left review comments

@fpapon fpapon fpapon requested changes

@lprimak lprimak lprimak approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bdemers @lprimak @bmarwell @fpapon