Skip to content

Fixed how multi-value attributes are modified in jpa model adapters#41432

Closed
mduchrow wants to merge 2 commits intokeycloak:mainfrom
mduchrow:38526-consistent-multi-value-attributes
Closed

Fixed how multi-value attributes are modified in jpa model adapters#41432
mduchrow wants to merge 2 commits intokeycloak:mainfrom
mduchrow:38526-consistent-multi-value-attributes

Conversation

@mduchrow
Copy link
Copy Markdown
Contributor

Aligned handling of setAttribute() in UserAdapter, RoleAdapter and GroupAdapter.
Duplicate values in the same attribute are now handled correctly. Duplicate values can be added and removed.

Closes #38526

@mduchrow mduchrow requested a review from a team as a code owner July 25, 2025 14:59
Copy link
Copy Markdown
Contributor

@sguilhen sguilhen left a comment

Choose a reason for hiding this comment

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

I like the proposed changes to how the attributes are handled, but I think we could simplify the tests if we add them directly to tests/base/ along with other group/user tests there.

@sguilhen sguilhen requested review from ahus1 and pedroigor January 6, 2026 21:50
@sguilhen
Copy link
Copy Markdown
Contributor

sguilhen commented Jan 6, 2026

@ahus1 @pedroigor adding you as reviewers as well - when you have some time please take a look.

@dasniko
Copy link
Copy Markdown
Contributor

dasniko commented Mar 10, 2026

@mduchrow Could you please:

  • set back this PR to draft mode, so that it doesn't annoy the Keycloak team as long as we are working on it
  • remove your custom test classes again, as we discussed to run the tests with the new test framework
  • rebase(!) your actual changes on UserAdapter, RoleAdapter and GroupAdapter onto current main branch

I've already prepared a test method to be run in the current org.keycloak.tests.model.UserModelTest. I can add it once the rebase is done (and if you give editing permissions on your PR branch).

@mduchrow mduchrow marked this pull request as draft March 10, 2026 16:37
Aligned handling of setAttribute() in UserAdapter, RoleAdapter and GroupAdapter

Closes keycloak#38526

Signed-off-by: mduchrow <[email protected]>
@mduchrow mduchrow force-pushed the 38526-consistent-multi-value-attributes branch from be07987 to 7730f16 Compare March 11, 2026 05:56
@dasniko
Copy link
Copy Markdown
Contributor

dasniko commented Mar 11, 2026

Thanks for cleaning up! 👍

I tried to test it locally, but it's still not possible to remove single attribute values if you have the same value multiple times. What I tried:

  • add three times an attribute foo with value bar to a user on the user attributes page in admin ui
  • save it, works, after refresh it's available three times
  • remove one foo->bar line from attributes list
  • save the user/attributes
  • still the 3 bar values are available

@dasniko
Copy link
Copy Markdown
Contributor

dasniko commented Mar 11, 2026

For your reference, I added a test to a fork of your PR: dasniko@826848b

Unfortunately, I don't get it to provide my changes as a PR to this PR... 🤪

@ryanemerson
Copy link
Copy Markdown
Contributor

Thank you for the PR @mduchrow, but I am now closing this as the original issue has been resolved by #48384.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Duplicate user attribute values cannot be removed

4 participants