Refactor: AssertJ best practices by @timtebeek

[lprimak]

Introduce AssertJ by @timtebeek
Supersedes #1450 and #1452

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

• Make sure there is a GitHub issue filed
  for the change (usually before you start working on it). Trivial changes like typos do not
  require a GitHub 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 [#XXX] - Fixes bug in SessionManager,
  where you replace #XXX with the appropriate GitHub issue. Best practice
  is to use the GitHub 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.
• add fixes #XXX if merging the PR should close a related issue.
• Run mvn verify 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.
• Committers: Make sure a milestone is set on the PR

Trivial changes like typos do not require a GitHub 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.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
• In any other case, please file an Apache Individual Contributor License Agreement.

[lprimak]

Unfortunately, there are many warnings. See https://jenkins.hope.nyc.ny.us/job/docker-shiro-ci/job/main/38/analysis/

[lprimak]

Hi, @timtebeek
Unfortunately, I had to revert the AssertJ refactor. There are 66 new warnings that have resulted from the compiler.

[timtebeek]

It seems 51 of those warnings are in JndiLdapContextFactoryTest, with cases like this one:

For comparison, this is what was there before, with assertEquals taking two objects as input:

What would you recommend we do about such instances?

[lprimak]

And once again, proper testing reveals underlying code issues :) working on it

[lprimak]

yay! It's green https://jenkins.hope.nyc.ny.us/job/docker-shiro-ci/job/assert-j-best-practices/1/

[lprimak]

Please pay special attention to this commit: 798cd18

I believe this is safe to put into 2.0.1 version because the API doesn't really change and just the generics are made explicit.
What do you think?

[bmarwell]

Please pay special attention to this commit: 798cd18

I believe this is safe to put into 2.0.1 version because the API doesn't really change and just the generics are made explicit.
What do you think?

Urgh. By definition a breaking change! Maybe put the 3.x label on that commit?

It's probably safe though

[lprimak]

My understanding of Java generics erasure is that this change is source and binary compatible with the previous, thus not a breaking change... but I could be wrong.
In former case it would be safe to merge for 2.0.1 and latter it isn't :)
I also think it's early enough in the lifecycle of 2.x that this would be safe to merge

[bmarwell]

Binary: yes.
Source compatible: most likely yes, but not if a user explicitly defined something else like Object,Object. Then no.

I'd be okay with this one as it is early 2.0.x for that reason.

@bdemers @fpapon Opinions?

[lprimak]

but not if a user explicitly defined something else like Object,Object.

Since previous code was "raw" class without any parameters, they would have to cast in that case anyway...

[fpapon]

I think the problem can be this one about api change:

public void setEnvironment(Map<String, Object> env) {

For now user can put any type in the key and we can imagine that he should use a String but we are not sure.

I understand the purpose of the best practice but it's not a good idea to update our api because of unit test framework change.

[bmarwell]

I think the problem can be this one about api change:

public void setEnvironment(Map<String, Object> env) {

For now user can put any type in the key and we can imagine that he should use a String but we are not sure.

I understand the purpose of the best practice but it's not a good idea to update our api because of unit test framework change.

Yes, that's a real no go and needs to wait until 3.

[lprimak]

Yup. As disappointing as this sounds, this one will have to wait

[bmarwell]

@lprimak just modify your branch to leave that one out and merge it into 2.0.1

Then do another for 3.0.0 with that change.

[bdemers]

Just catching up after some travel.

Would most of the concerns here go away if we created a custom assertion for JndiLdapContextFactory and PrincipalCollection?

That way we would at least get the migration to AssertJ in (thanks again @timtebeek).
And we can create an issue/TODO to cleanup the type <> everywhere.
Speaking of... is there an OpenRewrite recipe for that? 😄

[bdemers]

(I can volunteer to write the assertion class, if that resolves the issues), it would probably read something like:

Shiro.assertThat(factory).containsEnvironment("foo, "bar");

[timtebeek]

And we can create an issue/TODO to cleanup the type <> everywhere. Speaking of... is there an OpenRewrite recipe for that? 😄

Absolutely! Here's a quick recipe run: https://app.moderne.io/results/j2iJu9Xxx/
And a branch you can use to create a PR to clear out those 152 cases.
https://github.com/apache/shiro/compare/main...timtebeek:shiro:refactor/use-diamond-operator?expand=1

[lprimak]

Yea, that sounds great! If there are no warnings it's all good by me :)

[lprimak]

I have reverted the API change commit.

[bdemers]

I started hacking a custom Assertion class up, and IMHO, it's too complex for the value it provides.

The value is essentially, working around a valid compiler warning. The better solution for that is doing something like:
@SuppressWarnings("unchecked") to the relevant tests. We have a bunch of suppressions in the tests already.

My current plan is to open an issue to fix the generic type defs, and mark the effected test methods with something like:

@SuppressWarning("unchecked") // TODO: Remove this warning when issue #xxxx is resolved
public void someTestMethod() {
    // ...
    assertThat(env).containsEntry(Context.SECURITY_PRINCIPAL, "foo");
    // ...
}

Any objections to this approach?

Note

I do think custom assertion classes could be useful in other places, just in this case.

[lprimak]

No onjections from me.