Update composite roles on child role removal #11816
Conversation
ahus1
left a comment
There was a problem hiding this comment.
I looked at the changes for LDAP and briefly at the other changes as well.
The LDAP changes are fine for now as will retrieve all roles, and then use the CHM criteria to filter out the right roles.
Please open a LDAP issue to add a more efficient implementation for LDAP and assign this to me.
| return new JpaRoleModelCriteriaBuilder((cb, root) -> | ||
| cb.isTrue(cb.function("@>", | ||
| Boolean.TYPE, | ||
| cb.function("->", JsonbType.class, root.get("metadata"), cb.literal("fCompositeRoles")), | ||
| cb.literal(convertToJson(value[0])))) |
There was a problem hiding this comment.
@hmlnarik - probably not. At the same time I would assume that composite roles might soon move to its own table to follow along the discussion found here to allow adding or removing an entry to the composite roles without loading the rest of the entity: #11799
Depending on where that other PR will move to and how the performance improvements are, I wanted to call for a meeting on that topic to discuss how to handle situation where an attribute can contain a big collection of entries.
There was a problem hiding this comment.
I'm happy to keep this as is in this PR. Could you please file a GH issue that points to this discussion?
| return new JpaRoleModelCriteriaBuilder((cb, root) -> | ||
| cb.equal(root.get(modelField.getName()), value[0]) | ||
| ); | ||
| } else if (modelField == SearchableFields.COMPOSITE_ROLE){ |
There was a problem hiding this comment.
I just realized I am using a different comparison method here. I am using == while everywhere else is the equals method call. If I am not mistaken, both should work the way we want. Should I change this to use the equals method as well? Or it is okay to leave this as is?
There was a problem hiding this comment.
Actually, when thinking about it and reading[1], I believe we should be using == instead of equals. I'd create follow-up task to update it in all other places if we agree on it. I'll bring it up in the mtg.
[1] https://docs.oracle.com/javase/specs/jls/se9/html/jls-8.html#jls-8.9
There was a problem hiding this comment.
I agree with changing it to == as we want to have only one instance of each SearchableField, however, SearchableFields are not enum. That is probably why there was equals method used.
There was a problem hiding this comment.
+1 for using ==, the fields should be the same instances as the predefined ones.
Closes #11769