Skip to content

[#1548] - redefines PrincipalCollection interface to be intended as immutable#1582

Merged
lprimak merged 1 commit intoapache:3.xfrom
janitza-mage:issue-1548-immutable-principal-collection
Aug 4, 2024
Merged

[#1548] - redefines PrincipalCollection interface to be intended as immutable#1582
lprimak merged 1 commit intoapache:3.xfrom
janitza-mage:issue-1548-immutable-principal-collection

Conversation

@janitza-mage
Copy link
Copy Markdown

@janitza-mage janitza-mage commented Jul 10, 2024

Closes #1548. Declare PrincipalCollection interface to be intended to be immutable; provide an immutable implementation including a builder class; deprecate the mutable implementation; use the immutable one in SimpleAuthenticationInfo for merging.

Background: Merging AuthenticationInfo and the contained PrincipalCollections previously lead to one of the involved PrincipalCollections be selected by undefined means and being mutated, propagating the changes to other callers that did not expect such changes, including the authentication cache.

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.

Copy link
Copy Markdown
Member

@bdemers bdemers left a comment

Choose a reason for hiding this comment

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

I took a quick pass through, it looks great! thank you!
One nit, can you add the @Deprecated annotation as well (to MutablePrincipalCollection anywhere it's used (and set forRemoval=true)
(and possibly the @deprecated javadoc tag to SimplePrincipalCollection, with a note to use ImmutablePrincipalCollection instead.

Thanks again!!
(I'll trigger the CI builds for this)

… intended to be immutable; provide an immutable implementation including a builder class; deprecate the mutable implementation; use the immutable one in SimpleAuthenticationInfo for merging.

Background: Merging AuthenticationInfo and the contained PrincipalCollections previously lead to one of the involved PrincipalCollections be selected by undefined means and being mutated, propagating the changes to other callers that did not expect such changes, including the authentication cache.
@janitza-mage janitza-mage force-pushed the issue-1548-immutable-principal-collection branch from f6a10ba to e63aeae Compare July 11, 2024 06:20
@janitza-mage
Copy link
Copy Markdown
Author

I have added @deprecated to SimplePrincipalCollection. That was the only place where MutablePrincipalCollection is referenced.

@lprimak lprimak requested a review from bdemers July 11, 2024 17:17
@lprimak lprimak dismissed bdemers’s stale review July 11, 2024 17:18

Problem has been fixed

@lprimak lprimak added this to the 3.0.0 milestone Jul 11, 2024
@lprimak
Copy link
Copy Markdown
Contributor

lprimak commented Jul 11, 2024

@janitza-mage Can you please restore the template for this PR?
We would need you to click appropriate checkboxes for a contribution license acceptance.

@lprimak lprimak added pending-cla core Core Modules labels Jul 11, 2024
@janitza-mage
Copy link
Copy Markdown
Author

I edited the initial comment. Remarks:

  • The "fixes #XXX" is N/A because no related issue gets fixed by this
  • about the milestone: I have no clue what is meant by that
  • the code is apache-licensed, so I ticked the first box and left the second one unticked -- is that correct?

@lprimak
Copy link
Copy Markdown
Contributor

lprimak commented Jul 12, 2024

Thanks.
I fixed up the remaining issues.

@janitza-mage
Copy link
Copy Markdown
Author

Is there anything left for me to do in this PR, then? I think I can't merge it myself, right?

@lprimak
Copy link
Copy Markdown
Contributor

lprimak commented Jul 16, 2024

No, you are good.
We need to create 3.x development branch, make some new infra etc.
It's gonna be a while until we merge this unfortunately.

@lprimak lprimak added CLA and removed pending-cla labels Aug 3, 2024
@lprimak lprimak changed the base branch from main to 3.x August 4, 2024 02:17
@lprimak lprimak merged commit b516082 into apache:3.x Aug 4, 2024
@lprimak lprimak mentioned this pull request Aug 5, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA core Core Modules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enhancement] Mutability of PrincipalCollection and AuthenticationInfo

3 participants