Mrc-5298-Role Update Endpoints #64

absternator · 2024-05-01T11:06:38Z

The work here contains 2 endpoints that encompass role update. They are addPermissionsToRole and removePermissionsToRole. They respectively add and remove permissions from a role ..

NOTE:
For removePermissionsToRole we had to delete the rolepermission rows directly. Hibernate didn't work with just removing the rolePermission from the role. I will create a new ticket that can come back and investigate this further

codecov · 2024-05-01T11:29:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.98%. Comparing base (52c32c2) to head (6be3222).
Report is 10 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #64   +/-   ##
=======================================
  Coverage   93.98%   93.98%           
=======================================
  Files          67       67           
  Lines         532      532           
  Branches      131      131           
=======================================
  Hits          500      500           
  Misses         30       30           
  Partials        2        2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

into mrc-5298-role-update

… in RolePermission constructor

into mrc-5298-role-update

into mrc-5296-role-read

into mrc-5298-role-update

into mrc-5296-role-read

into mrc-5298-role-update

into mrc-5296-role-read

into mrc-5298-role-update

into mrc-5296-role-read

EmmaLRussell

Could you explain a bit more about the need for equality/hash checking for the role permissions? Is it related to trying to get hibernate to handle the deletion of the rows in the link table? Or was it to support this line in RoleService:
if (rolePermissionsToAdd.any { role.rolePermissions.contains(it) })
...
or in RolePermissionService
val matchedPermission = role.rolePermissions.find { rolePermissionToRemove == it }
?
Those would be easier if you were dealing with the data classes (dto's) which would support equality automatically.

EmmaLRussell · 2024-05-06T10:33:00Z

api/app/src/main/kotlin/packit/controllers/RoleController.kt


        return ResponseEntity.ok(mapOf("message" to "Role deleted"))
    }
+
+    @PutMapping("/add-permissions/{roleName}")


These endpoints don't seem very RESTful.
Would usually be something like
POST or DELETE /role/{name}/permission

its not really a post or delete because we are not creating/deleting the role. We are updating the role object i.e its a update operation. technically should be patch but spring didn't like patch - said it doesn't accept them

Maybe not POST, but should at least be PUT and DELETE then, as described here:
https://www.mscharhag.com/api-design/rest-many-to-many-relations

REST is generally disapproving of using verbs like "add" which could be expressed as http verbs. I guess it's a bit muddier if you're not providing the GET equivalent, but I still think it would be better than embedding the verb in the endpoint name.

And even if we do keep add-permissions, it should go after the roleName not before (route to the role, then say what you're going to do to it).

Is this discussion all irrelevant though, as didn't you say you were going to replace these with a single PUT update endpoint?

EmmaLRussell · 2024-05-06T10:34:46Z

api/app/src/main/kotlin/packit/controllers/RoleController.kt

+    {
+        roleService.addPermissionsToRole(roleName, addRolePermissions)
+
+        return ResponseEntity.ok(mapOf("message" to "Permissions added"))


Again, not sure this is useful. Empty response or updated role would be more so.

yup agree have created ticjet fir all these as prev pr comment

EmmaLRussell · 2024-05-06T10:38:30Z

api/app/src/main/kotlin/packit/model/dto/UpdateRolePermission.kt

+data class UpdateRolePermission(
+    @field:NotNull
+    val permission: String,
+    val packetId: String? = null,
+    val tagId: Int? = null,
+    val packetGroupId: Int? = null
+)


So we're only allowing global permissions when we create a role, but can update with scoped permissions? That seems inconsistent. Could accept a list of these on create role, rather than just permission names.. and then wouldn't this type also be the scoped permission type returned from the GET request too? ...oh, minus the id of course

I think for the create the FE most likely wont pass any permissions.. And here update is where it can pass in scoped if needed...The idea was it would be weird adding scoped permissions when creating a role as well... But adding a scoped permission when updating makes sense also.... This logic will be mirrored on the FE.

OK, but you still want to allow global permissions on create to make life easier for the admin cli/setup scripts?
I'm not really convinced it's a useful distinction. Setup might want to make roles with scoped permissions in some cases for example. I think it would be fine to make role create take no perms and require that to be done in a second request.

EmmaLRussell · 2024-05-06T10:43:21Z

api/app/src/main/kotlin/packit/model/dto/UpdateRolePermission.kt

+    init
+    {
+        val nonNullFields = listOf(packetId, tagId, packetGroupId).count { it != null }
+        require(nonNullFields <= 1) {
+            "Either all of packetId, tagId, packetGroupId should be null or only one of them should be not null"
+        }
+    }


Yeah, this is nice, and I think we could still keep this anyway, even this type was also re-used as the RolePermission dto.

EmmaLRussell · 2024-05-06T10:48:17Z

api/app/src/main/kotlin/packit/model/RolePermission.kt

+{
+    init
+    {
+        val nonNullFields = listOf(packet, tag, packetGroup).count { it != null }
+        require(nonNullFields <= 1) {
+            "Either all of packet, tag, packetGroup should be null or only one of them should be not null"
+        }
+    }


Repeated logic... Given that you've got this constraint on the db, and also in the dto, maybe you don't need it here as well. But if so, should be able to implement once and use from both types.

yeah i thought that too but couldnt find a way to implement once and use both places.. Also they a bit different as one is ids and other is whole objects... And probs don't need but put here to for safety and mirror this constraint on DB and make more visible (as many people don't check db constraints haha)

EmmaLRussell · 2024-05-06T11:09:05Z

api/app/src/main/kotlin/packit/service/RoleService.kt

+        val role = roleRepository.findByName(roleName)
+            ?: throw PackitException("roleNotFound", HttpStatus.BAD_REQUEST)
+        rolePermissionService.removeRolePermissionsFromRole(role, removeRolePermissions)


It seems a bit odd to check that permissions being added don't yet exist here in RoleService but that permissions deing deleted do exist, in RolePermissionService service. Should both be in the same service, probably RolePermissionsService

EmmaLRussell · 2024-05-06T11:11:46Z

api/app/src/main/kotlin/packit/service/RolePermissionService.kt

+{
+    override fun getRolePermissionsToUpdate(
+        role: Role,
+        addRolePermissions: List<UpdateRolePermission>


Suggested change

addRolePermissions: List<UpdateRolePermission>

updateRolePermissions: List<UpdateRolePermission>

i named like that to differentiate the adding and deleting of role permissions

But you're using it for permissions being removed too:
https://github.com/mrc-ide/packit/pull/64/files#diff-91662b556b4c3ee7dc5037863b1a55465c6b8218de7a6675c404baec77f0fb66R56

oh yup was mistaken have updated

EmmaLRussell · 2024-05-06T11:17:07Z

api/app/src/main/kotlin/packit/service/RolePermissionService.kt

+        return addRolePermissions.map { addRolePermission ->
+            val permission = permissionRepository.findByName(addRolePermission.permission)
+                ?: throw PackitException("permissionNotFound", HttpStatus.BAD_REQUEST)
+            RolePermission(
+                role = role,
+                permission = permission,
+                packet = addRolePermission.packetId?.let {
+                    packetRepository.findById(it)
+                        .orElseThrow { PackitException("packetNotFound", HttpStatus.BAD_REQUEST) }
+                },
+                packetGroup = addRolePermission.packetGroupId?.let {
+                    packetGroupRepository.findById(it)
+                        .orElseThrow { PackitException("packetGroupNotFound", HttpStatus.BAD_REQUEST) }
+                },
+                tag = addRolePermission.tagId?.let {
+                    tagRepository.findById(it).orElseThrow { PackitException("tagNotFound", HttpStatus.BAD_REQUEST) }
+                }
+            )
+        }


This is quite dense visuall,y but makes sense on closer inspection.
I think an empty line at l.34 would make it more readable by splitting off the main chunk.

EmmaLRussell · 2024-05-06T11:45:27Z

api/app/src/main/kotlin/packit/service/RolePermissionService.kt

+        val rolePermissionsToRemove = getRolePermissionsToUpdate(role, removeRolePermissions)
+
+        val matchedRolePermissionsToRemove = rolePermissionsToRemove.map { rolePermissionToRemove ->
+            val matchedPermission = role.rolePermissions.find { rolePermissionToRemove == it }
+                ?: throw PackitException("rolePermissionDoesNotExist", HttpStatus.BAD_REQUEST)
+            matchedPermission
+        }
+


I don't think you really need to do a map here, just a forEach then just map the original rolePermissionsToRemove to ids to pass to the deleteAllByIdIn method

unsure how the foreach would work here? and the benefits of doing it that way?

Yeah sorry that doesn't work because you don't actually have the ID on the UpdateRolePermissions.

EmmaLRussell · 2024-05-06T11:51:26Z

api/app/src/test/kotlin/packit/integration/controllers/RoleControllerTest.kt

@@ -83,4 +97,50 @@ class RoleControllerTest : IntegrationTest()
        assertSuccess(result)
        assertNull(roleRepository.findByName("testRole"))
    }
+
+    @Test
+    @WithAuthenticatedUser(authorities = ["user.manage"])


Could test for unauthorized request too.

into mrc-5298-role-update

absternator · 2024-05-07T09:26:02Z

Could you explain a bit more about the need for equality/hash checking for the role permissions? Is it related to trying to get hibernate to handle the deletion of the rows in the link table? Or was it to support this line in RoleService: if (rolePermissionsToAdd.any { role.rolePermissions.contains(it) }) ... or in RolePermissionService val matchedPermission = role.rolePermissions.find { rolePermissionToRemove == it } ? Those would be easier if you were dealing with the data classes (dto's) which would support equality automatically.

Yup so pretty much used to compare equality fo the entities.. For delete we need to filter out some out and things just in general for match... In general good to have them for these as can be used for comparison.. Sadly if we were to use dtos we would have to first convert and then compare and then convert back to entity

into mrc-5296-role-read

EmmaLRussell · 2024-05-13T09:52:52Z

api/app/src/main/kotlin/packit/controllers/RoleController.kt

-    @PutMapping("/add-permissions/{roleName}")
-    fun addPermissionsToRole(
-        @RequestBody @Validated addRolePermissions: List<UpdateRolePermission>,
+    @PutMapping("/update-permissions/{roleName}")


OK, this overrides my previous comment!
I think this should just be PUT /role/{roleName}

have done for all PUT as suggested as above in PR #68.. /role/{rolenmame}/permissions & /role/{rolenmame}/users

EmmaLRussell · 2024-05-13T10:00:57Z

api/app/src/main/kotlin/packit/service/RolePermissionService.kt

+        rolePermissionRepository.deleteAllByIdIn(matchedRolePermissionsToRemove.map { it.id!! })
+    }
+
+    override fun getAddRolePermissionsFromRole(


It's quite confusing having getRolePermissionsToUpdate and getAddRolePermissionsFromRole (that sounds like it's getting the permissions from the role, but if the permissions are already in the role, you're going to throw an exception!). I wouldn't mind just making the method into a checkPermissionsToAddAreNotOnRole or something, and have the main update method call `getRolePermissionsToUpdate directly, and pass the perms into here to just do the check...

yup agree ended up changing to getRolePermissionsToAdd and making the getRolePermissionsToUpdate internal so not visible to public

into mrc-5296-role-read

Mrc-5296 Role read

absternator added 3 commits May 1, 2024 07:59

feat: get updates working

58c6150

Remove unused code in RoleService

9580f70

test: test logic

49dd4aa

absternator requested a review from EmmaLRussell May 1, 2024 11:06

Fix formatting and remove unnecessary code

8baee2c

absternator added 22 commits May 1, 2024 14:09

Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/packit

12a3d3c

into mrc-5298-role-update

Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/packit

aa7b95e

into mrc-5298-role-update

Update RoleController endpoints

ce58d17

Refactor variable name in RolePermissionService.kt

bd0145b

Add exception handler for IllegalArgumentException and add validation…

1095f2c

… in RolePermission constructor

feat: get reads working

5cf4187

test: unit + integration tests

98e3062

Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/packit

5ed4a0d

into mrc-5298-role-update

Merge branch 'mrc-5298-role-update' of https://github.com/mrc-ide/packit

5f1f1a3

into mrc-5296-role-read

Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/packit

e223d24

into mrc-5298-role-update

Merge branch 'mrc-5298-role-update' of https://github.com/mrc-ide/packit

a83cacf

into mrc-5296-role-read

Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/packit

252975a

into mrc-5298-role-update

Merge branch 'mrc-5298-role-update' of https://github.com/mrc-ide/packit

8658e23

into mrc-5296-role-read

Refactor model classes

27d5b0e

Add getUsernameRoles and getNonUsernameRoles endpoints

bf920c5

Fix roleDto saving in RoleControllerTest

ecb871e

Refactor test case for getting non-username roles with relationships

1ce3d7d

Refactor RoleControllerTest to improve readability and add assertions

169ea1d

Refactor RoleControllerTest to use usernameRole instead of roleDto

5765f22

Refactor RoleControllerTest to improve username role retrieval

a413033

Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/packit

f25fcf0

into mrc-5298-role-update

Merge branch 'mrc-5298-role-update' of https://github.com/mrc-ide/packit

54eabf2

into mrc-5296-role-read

EmmaLRussell reviewed May 6, 2024

View reviewed changes

Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/packit

513285e

into mrc-5298-role-update

absternator added 2 commits May 7, 2024 10:16

Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/packit

eb95c4c

into mrc-5298-role-update

Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/packit

fa37abd

into mrc-5298-role-update

update as per pr coments

c6d2bc5

absternator requested a review from EmmaLRussell May 7, 2024 10:09

absternator added 9 commits May 7, 2024 11:14

Merge branch 'mrc-5298-role-update' of https://github.com/mrc-ide/packit

dab6a1b

into mrc-5296-role-read

Refactor RoleControllerTest***

27d0103

Refactor RoleController and RoleService

163e5c0

Remove empty lines in RoleControllerTest

30d168b

feat: make add/remove into update

bc05370

Merge branch 'mrc-5298-role-update' of https://github.com/mrc-ide/packit

5e07232

into mrc-5296-role-read

Refactor imports in RoleControllerTest

1ea9487

Update assertSuccess to assertEquals for result status code

771eead

Refactor imports in RoleController.kt

a98ea9e

EmmaLRussell reviewed May 13, 2024

View reviewed changes

Base automatically changed from mrc-5297-role-delete to main May 13, 2024 13:39

Refactor role permission service and role service

8130999

absternator requested a review from EmmaLRussell May 13, 2024 14:04

absternator added 2 commits May 13, 2024 15:05

Merge branch 'mrc-5298-role-update' of https://github.com/mrc-ide/packit

d6d9e5f

into mrc-5296-role-read

Merge pull request #65 from mrc-ide/mrc-5296-role-read

6be3222

Mrc-5296 Role read

EmmaLRussell approved these changes May 14, 2024

View reviewed changes

absternator merged commit 60e1bbb into main May 14, 2024
5 checks passed

absternator deleted the mrc-5298-role-update branch May 14, 2024 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mrc-5298-Role Update Endpoints #64

Mrc-5298-Role Update Endpoints #64

absternator commented May 1, 2024

codecov bot commented May 1, 2024 •

edited

EmmaLRussell left a comment

EmmaLRussell May 6, 2024

absternator May 7, 2024

EmmaLRussell May 13, 2024 •

edited

EmmaLRussell May 6, 2024

absternator May 7, 2024

EmmaLRussell May 6, 2024

absternator May 7, 2024 •

edited

EmmaLRussell May 13, 2024

EmmaLRussell May 6, 2024

EmmaLRussell May 6, 2024

absternator May 7, 2024

EmmaLRussell May 6, 2024

EmmaLRussell May 6, 2024

absternator May 7, 2024

EmmaLRussell May 13, 2024

absternator May 13, 2024 •

edited

EmmaLRussell May 6, 2024

EmmaLRussell May 6, 2024

absternator May 7, 2024

EmmaLRussell May 13, 2024

EmmaLRussell May 6, 2024

absternator commented May 7, 2024

EmmaLRussell May 13, 2024

absternator May 13, 2024

EmmaLRussell May 13, 2024

absternator May 13, 2024

	addRolePermissions: List<UpdateRolePermission>
	updateRolePermissions: List<UpdateRolePermission>

Mrc-5298-Role Update Endpoints #64

Mrc-5298-Role Update Endpoints #64

Conversation

absternator commented May 1, 2024

codecov bot commented May 1, 2024 • edited

Codecov Report

EmmaLRussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmaLRussell May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absternator May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absternator May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absternator commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 1, 2024 •

edited

EmmaLRussell May 13, 2024 •

edited

absternator May 7, 2024 •

edited

absternator May 13, 2024 •

edited