Mrc-6394 Fix issue of creating name with spaces #190
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #190 +/- ##
=======================================
Coverage 97.15% 97.15%
=======================================
Files 150 150
Lines 1476 1476
Branches 425 425
=======================================
Hits 1434 1434
Misses 41 41
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -24,7 +24,7 @@ class RoleController(private val roleService: RoleService, private val userRoleS | |||
| { | |||
| val role = roleService.createRole(createRole) | |||
|
|
|||
| return ResponseEntity.created(URI.create("/roles/${role.name}")).body(role.toDto()) | |||
| return ResponseEntity.created(URI.create("/roles/${role.id}")).body(role.toDto()) | |||
There was a problem hiding this comment.
I'm confused by this. The getRoleByName endpoint requires a name, not a numerical ID. How does this fix anything? Why do spaces actually cause an issue? Do we need to add a role "slug"? Should we ban spaces from role names?
There was a problem hiding this comment.
I think what you want is something like:
UriComponentsBuilder.fromUriString("/roles/{}").build(role.name)In general doing naive string interpolation anywhere involving URLs is a bad idea.
That will properly encode the argument into the URI string. Spaces in the name should become %20 and slashes should become %2F (and we should make sure requests to /roles/foo%2fbar actually works the way we want it to).
Or we could just not return any URI here. I don't think the frontend uses it anyway.
EDIT: turns out we disallow slashes in names, except we only enforce that in the frontend, not the backend. Doing a GET of /roles/foo%2fbar is interpreted as /roles/foo/bar, which doesn't match any route (I'm not 100% sure if the %2f -> / translation is done by Spring or nginx). You can create such a role with a POST /role, but you cannot access it or even delete it since DELETE /roles/foo/bar will not work. I had to manually delete the row from the database.
I think it would be nice to put some enforcement of the name restrictions in the backend instead/in addition to the FE (for non-username role names, username role names and user names).
There was a problem hiding this comment.
ohh yeah i thought we had /role/{id} endpoint... but you are right and we don't use URL anways so have removed... also good point with BE validation too so have added that
…emove URI creation; add validation pattern to CreateRole name
| import org.jetbrains.annotations.NotNull | ||
|
|
||
| data class CreateRole( | ||
| @field:NotNull | ||
| @field:Pattern( | ||
| regexp = "^[a-zA-Z0-9]+(\\s[a-zA-Z0-9]+)*\$", |
There was a problem hiding this comment.
same regex as FE
issue with create URI should be using role.id not role.name