HierarchyPermissions refactoring
#4234
Conversation
Signed-off-by: Martin Nonnenmacher <[email protected]>
Turn the implementations of `HierarchyPermission` into classes to make it more obvious that there are exactly two implementations of the interface. Signed-off-by: Martin Nonnenmacher <[email protected]>
The function is only needed in a single place, inlining it avoids the need to pass arguments around and improves coherence. Signed-off-by: Martin Nonnenmacher <[email protected]>
The values of the map were redundant because they were always `true`, therefore the map can be turned into a set that only contains the `CompoundHierarchyId`s. Signed-off-by: Martin Nonnenmacher <[email protected]>
Signed-off-by: Martin Nonnenmacher <[email protected]>
The function is only needed in a single place, inlining it avoids the need to pass arguments around and improves coherence. Signed-off-by: Martin Nonnenmacher <[email protected]>
While at it, also simplify the implementation and add tests. Signed-off-by: Martin Nonnenmacher <[email protected]>
Signed-off-by: Martin Nonnenmacher <[email protected]>
Improve some docs in `HierarchyPermissions` and use more telling variable names to improve readability. Signed-off-by: Martin Nonnenmacher <[email protected]>
…ction Turn `findAssignment` into an extension function and give it a more generic name. Signed-off-by: Martin Nonnenmacher <[email protected]>
mnonnenmacher
requested review from
MarcelBochtler,
bs-ondem,
nnobelis,
oheger-bosch,
sschuberth and
wkl3nk
as code owners
| * Construct the [Map] with information about available permissions on different levels in the hierarchy based on | ||
| * the given [assignmentsByLevel] and the [checker] function. | ||
| */ | ||
| private fun constructAssignmentsMap( |
There was a problem hiding this comment.
I actually prefer the version with the dedicated function. It is a well-defined piece of work with a meaningful name and a comment, and it is explicit which input it requires. Same for other similar refactorings.
There was a problem hiding this comment.
While I personally don't have a very strong opinion here, I usually find it easier to follow the code without the need to jump around. Also the function name does not really tell in which way the assignments map is created, so I'd need to read the code or docs anyway. To me, the best argument for keeping it a function would have been testability, but this function is not covered by a test anyway.
There was a problem hiding this comment.
I usually find it easier to follow the code without the need to jump around
I first made it a class function but that was why it felt even better to me to move it to the initializer.
| * A list of all parent [CompoundHierarchyId]s of this instance, starting from the direct parent up to the highest | ||
| * level. | ||
| */ | ||
| val parents: List<CompoundHierarchyId> get() = generateSequence(parent) { it.parent }.toList() |
There was a problem hiding this comment.
Are there other use cases that would justify that this becomes part of the public API of CompoundHierarchyId? I thought that this is rather specific to the way the parents are processed by the permissions component.
There was a problem hiding this comment.
It just made sense to me that the class offers not only a way to get the direct parent but also all parents. I haven't checked if there is already another place that needs this but this way it's easy to find in case it is needed.
| in assignments -> id | ||
| else -> findAssignment(assignments, id.parent) | ||
| } | ||
| private tailrec fun Set<CompoundHierarchyId>.findIdOrParent(id: CompoundHierarchyId?): CompoundHierarchyId? = |
There was a problem hiding this comment.
I am not that happy with this change. The new name is not that meaningful to me. Also, I don't consider it necessarily as an improvement that the function is turned to an extension function.
There was a problem hiding this comment.
The new name is not that meaningful to me.
It's difficult to explain why but I found it particularly difficult to understand what this findAssignment function does when it was called like this before:
findAssignment(this, id.parent)
findAssignment(assignmentsMap, compoundHierarchyId)I think part of it was the terminology, because I was confused why a CompoundHierarchyId is called an assignment here when a role assignment is a combination of an ID with a role. It added to the confusion that the function docs called the assignment "permission check result". It also required reading the function docs or implementation to find out that the parents of the ID are also taken into account.
To me it is easier to understand the call sites when the function has the more generic name that focuses on behavior instead of domain intent.
Also, I don't consider it necessarily as an improvement that the function is turned to an extension function.
I made this because it's more idiomatic Kotlin, the vast majority of operations on collections is done via extension functions.
| implicits = implicitIncludes.first | ||
| causing = implicitIncludes.second |
There was a problem hiding this comment.
IMO it's nice to get rid of dealing with the non-telling first / second of the pair.
There was a problem hiding this comment.
Yes, but I actually introduced that in an earlier commit knowing that it would be temporary 😉
Some refactorings of the
HierarchyPermissionsclass to improve readability, see the commit messages for details.