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.

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

[SHIRO-898] Jakarta EE integration #563

Merged
merged 1 commit into from
Jan 23, 2023
Merged

[SHIRO-898] Jakarta EE integration #563

merged 1 commit into from
Jan 23, 2023

Conversation

lprimak
Copy link
Contributor

@lprimak lprimak commented Dec 8, 2022 •
edited
Loading

Feature content

https://github.com/lprimak/shiro/blob/jakartaee-integration/support/jakarta-ee/README.adoc

Integrates Shiro functionality into Jakarta EE Web / Full Profile

Notes for reviewers:
This is an integration module with the full Jakarta EE web profile.
The purpose of this module is for the consumer to integrate with Jakarta EE Web Profile to the fullest extent possible.
As an integration module, it pulls in the pieces necessary to develop a complete application.
It is not the intent of this module to be minimalistic. Core Shiro packages already serve that purpose well.
This module incorporates features of the whole Web Profile..
I thought long and hard which dependencies to pull in, and I think I have achieved good balance between minimal necessary, quality of the resulting code, and ease of maintenance.

I would like for the reviewers to look for the following:

  • Bugs (of course)
  • Code style compatibility with Shiro and Apache (the code has been checkstyle'd so there should be no issues)
  • Any glaring features that are missing
  • Any overall issues with compatibility how Shiro and Apache does things
  • Things that are not 'basic' (the code is 10+ years old, the basics should be covered pretty well)

What I think is out of scope for this review:

  • Rearchitecting the project, unless there is something truly catastrophic I missed (I do not have the time, and I will likely withdraw the PR)
  • Reducing / Increasing scope and intent (again, for the same reason as above)
  • Dependencies can be discussed off-line, as those are always a sticking point, and a point of preference, but I would prefer to leave things as they are (again, there only so much time I can devote to such non-zero-risk endeavor)

See README for the documentation on feature set.

Thank you very much and I appreciate all the feedback

Following this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [SHIRO-XXX] - Fixes bug in SessionManager,
    where you replace SHIRO-XXX with the appropriate JIRA issue. Best practice
    is to use the JIRA issue title in the pull request title and in the first line of the commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean install apache-rat:check to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If you have a group of commits related to the same change, please squash your commits into one and force push your branch using git rebase -i.

Trivial changes like typos do not require a JIRA issue (javadoc, comments...).
In this case, just format the pull request title like (DOC) - Add javadoc in SessionManager.

If this is your first contribution, you have to read the Contribution Guidelines

If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement
if you are unsure please ask on the developers list.

To make clear that you license your contribution under the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.
@lprimak lprimak force-pushed the jakartaee-integration branch 2 times, most recently from 6eeeb71 to a51fd7f Compare December 15, 2022 00:06
@lprimak lprimak marked this pull request as ready for review December 15, 2022 00:13
bmarwell
bmarwell previously requested changes Dec 15, 2022
View reviewed changes
support/jakarta-ee/pom.xml Outdated Show resolved Hide resolved
support/jakarta-ee/src/main/resources/META-INF/beans.xml Show resolved Hide resolved
support/jakarta-ee/src/main/java/org/apache/shiro/ee/cdi/AnnotatedTypeWrapper.java
Comment on lines +21 to +22
import lombok.Getter;
import lombok.experimental.Delegate;
Copy link
Contributor

Choose a reason for hiding this comment

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

-1 for lombok as it does bytecode modification.
We can use immutables.org if you like which generates source code files instead.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using maven-lombok-plugin which generates source code instead of manipulating bytecode directly, and does not touch bytecode. It's in generated-source directory and that's what gets used
support/jakarta-ee/pom.xml
Comment on lines +60 to +59
<groupId>org.omnifaces</groupId>
<artifactId>omnifaces</artifactId>
<version>3.14.1</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for testing, -0.5 for pulling it in into prod code for just util classes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just did some OF audit and there are literally 100s of feature that are on by default it's a huge loss not to have it.
Copy link

Choose a reason for hiding this comment

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

Most JSF peope will use OF anyway (afaik) - I have no opinion here ;-)
support/jakarta-ee/pom.xml Outdated
Comment on lines 64 to 73
<dependency>
<groupId>org.jsoup</groupId>
<artifactId>jsoup</artifactId>
<version>1.15.3</version>
</dependency>
<dependency>
<groupId>com.flowlogix</groupId>
<artifactId>flowlogix-jee</artifactId>
<version>5.1.5</version>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. I would love to see if we could go without those.
Copy link
Contributor Author

@lprimak lprimak Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

JSoup is necessary to parse HTML. I don't think there any build-in html parsers in Jakarta EE and jsoup is widely used and tiny.
I removed FL dependency from code as it's no longer required
support/jakarta-ee/pom.xml Outdated Show resolved Hide resolved
support/jakarta-ee/src/main/java/org/apache/shiro/ee/cdi/AnnotatedTypeWrapper.java
private final @Delegate(types = AT.class) AnnotatedType<T> wrapped;
private final @Getter Set<Annotation> annotations;
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't justify lombok, we can use our IDEs here.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IDE won't maintain the generated code. Neither does 'delombok'
Lombok discussion in slack
support/jakarta-ee/src/main/java/org/apache/shiro/ee/cdi/KeyGen.java
* @author lprimak
*/
public class KeyGen {
private final Lazy<AesCipherService> cipherService = new Lazy<>(AesCipherService::new);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very elegant and easy to read, but a static inner Holder class would also do.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to be concise and limit the lines of code here
Copy link

@rzo1 rzo1 Dec 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Guess it is a question, if one want to replace OF or not ;-)
rzo1
rzo1 reviewed Dec 20, 2022
View reviewed changes
support/jakarta-ee/pom.xml Outdated Show resolved Hide resolved
integration-tests/jakarta-ee/pom.xml Outdated Show resolved Hide resolved
support/jakarta-ee/pom.xml Outdated Show resolved Hide resolved
integration-tests/jakarta-ee/pom.xml Outdated Show resolved Hide resolved
integration-tests/jakarta-ee/src/main/java/org/apache/shiro/testing/jakarta/ee/FormBean.java Outdated Show resolved Hide resolved
support/jakarta-ee/pom.xml Show resolved Hide resolved
support/jakarta-ee/src/main/java/org/apache/shiro/ee/cdi/AopHelper.java Outdated Show resolved Hide resolved
support/jakarta-ee/src/main/java/org/apache/shiro/ee/cdi/KeyGen.java
* @author lprimak
*/
public class KeyGen {
private final Lazy<AesCipherService> cipherService = new Lazy<>(AesCipherService::new);
Copy link

@rzo1 rzo1 Dec 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Guess it is a question, if one want to replace OF or not ;-)
support/jakarta-ee/src/main/java/org/apache/shiro/ee/cdi/ShiroOmniViewScoped.java Outdated Show resolved Hide resolved
support/jakarta-ee/src/main/java/org/apache/shiro/ee/cdi/ShiroSecurityInterceptor.java Outdated Show resolved Hide resolved
@lprimak
Copy link
Contributor Author

lprimak commented Dec 20, 2022

Thanks @rzo1 for your review! All good comments and I’ll work on those.
@rzo1
Copy link

rzo1 commented Dec 20, 2022

Thanks @rzo1 for your review! All good comments and I’ll work on those.

Some of my comments were responses on the review by @bmarwell - it was presented to me nicely in the GitHub web view but seems to be out of context in this view now.
@lprimak lprimak self-assigned this Dec 20, 2022
@lprimak lprimak added shiro-2.0.0 java Pull requests that update Java code labels Dec 20, 2022
@lprimak lprimak added this to the 2.0 milestone Dec 20, 2022
@lprimak lprimak changed the title [SHIRO-898] Jakarta EE integration Dec 21, 2022
@lprimak lprimak force-pushed the jakartaee-integration branch 7 times, most recently from f7df574 to 20f1c65 Compare December 25, 2022 22:27
@lprimak lprimak changed the title [SHIRO-898] (wip) Jakarta EE integration Dec 25, 2022
@lprimak lprimak force-pushed the jakartaee-integration branch 2 times, most recently from 11551b2 to 30f70f3 Compare December 27, 2022 20:07
@lprimak lprimak changed the title [SHIRO-898] Jakarta EE integration Dec 30, 2022
@lprimak lprimak changed the title [SHIRO-898] (wip) Jakarta EE integration Dec 31, 2022
@lprimak lprimak force-pushed the jakartaee-integration branch 2 times, most recently from 3bd8593 to 305af0d Compare January 5, 2023 07:03
@lprimak lprimak dismissed bmarwell’s stale review January 7, 2023 04:32

Assuming Java 11 base for Shiro 2.x, the comments no longer apply

@lprimak lprimak force-pushed the jakartaee-integration branch 2 times, most recently from 69e7732 to 7ce5039 Compare January 10, 2023 19:05
bmarwell
bmarwell reviewed Jan 13, 2023
View reviewed changes
pom.xml Outdated Show resolved Hide resolved
bmarwell
bmarwell reviewed Jan 13, 2023
View reviewed changes
pom.xml Outdated Show resolved Hide resolved
bmarwell
bmarwell reviewed Jan 13, 2023
View reviewed changes
pom.xml Outdated Show resolved Hide resolved
bmarwell
bmarwell reviewed Jan 13, 2023
View reviewed changes
integration-tests/pom.xml Outdated Show resolved Hide resolved
@lprimak lprimak force-pushed the jakartaee-integration branch 2 times, most recently from 38d275d to 8bd55b2 Compare January 13, 2023 22:29
@lprimak lprimak force-pushed the jakartaee-integration branch 2 times, most recently from f2913c3 to 8b4d0a5 Compare January 22, 2023 00:36
@lprimak lprimak merged commit 7b4604c into apache:main Jan 23, 2023
@lprimak lprimak deleted the jakartaee-integration branch January 23, 2023 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
java Pull requests that update Java code shiro-2.0.0
4 participants
@lprimak @rzo1 @bmarwell @fpapon