refactor: Simplify the secrets API via an enum for the id's entity #271
Conversation
Signed-off-by: Sebastian Schuberth <[email protected]>
While it did not seem to make a difference regarding the success of the test, semantically the product id is the correct one to use. Signed-off-by: Sebastian Schuberth <[email protected]>
Signed-off-by: Sebastian Schuberth <[email protected]>
| this.description = description | ||
| this.organization = id.takeIf { entity == Entity.ORGANIZATION }?.let { OrganizationDao[it] } | ||
| this.product = id.takeIf { entity == Entity.PRODUCT }?.let { ProductDao[it] } | ||
| this.repository = id.takeIf { entity == Entity.REPOSITORY }?.let { RepositoryDao[it] } |
There was a problem hiding this comment.
if would be shorter and to me also more readable:
this.organization = if (entity == Entity.ORGANIZATION) OrganizationDao[it] else nullBut it should also work with when:
when (entity) {
Entity.ORGANIZATION -> this.organization = OrganizationDao[id]
...
}There was a problem hiding this comment.
What I usually like about the takeIf approach is that the thing to assign from is listed first, instead of last / in the middle with the if approach.
Anyway, if when works here as entities are null by default, that's yet nicer. I'll use that.
There was a problem hiding this comment.
What I usually like about the
takeIfapproach is that the thing to assign from is listed first, instead of last / in the middle with theifapproach.
It's a matter of taste and being familiar with the pattern, but to me it's actually the opposite, close to an anti-pattern, especially if the condition does not refer to the thing takeIf is called on, like in this case. The if condition I can read from left to right like human language and understand on the first read what happens: "If the condition is fulfilled do this, otherwise that."
With takeIf in combination with let I always need additional thinking steps, I have to jump back to the beginning after understanding the condition and then see how the thing takeIf was called on is used in the let block, and remember that if the condition is false the whole expression evaluates to null.
Anyway, if
whenworks here as entities arenullby default, that's yet nicer. I'll use that.
I only hope it works but I'm not sure. I guess you'll find out.
There was a problem hiding this comment.
I guess you'll find out.
I hope the exitsing test coverage does for me 😉
There was a problem hiding this comment.
Just a point, for someone not too familiar with Kotlin, of the options above, the one below is by far the simplest to quickly comprehend for me.
this.organization = if (entity == Entity.ORGANIZATION) OrganizationDao[it] else null
| @@ -50,8 +52,8 @@ interface SecretsProvider { | |||
| fun removeSecret(path: Path) | |||
|
|
|||
| /** | |||
| * Generate a [Path] for the secret belonging to the given [organizationId], [productId] and [repositoryId], as well | |||
| * as the [secretName]. | |||
| * Generate a [Path] for the secret belonging to the given [entity] with [id], as well as the [secretName]. | |||
There was a problem hiding this comment.
Please also describe the new default implementation.
There was a problem hiding this comment.
Done. For the record, the default implementation is not "new" in the sense that it would be doing something different than before. It's just a simpler, centralized implementation of the original logic.
I decided to move this refactoring to another preceding commit to make that more clear.
| null, | ||
| null | ||
| Entity.ORGANIZATION, | ||
| organizationId |
There was a problem hiding this comment.
For most callers it would be nicer to have convenience functions like createRepositorySecret or createProductSecret (probably with default implementations in the interface as implementations would be identical).
There was a problem hiding this comment.
Where's the convenient in using these functions that basically just encode the entity parameter as part of the function name? They are hardly shorter, and create a maintenance burden if more entities (like business units, groups of organizations etc.) would be added in the future.
There was a problem hiding this comment.
I guess it's a matter of taste as well, personally I'd rather call getOrganizationSecrets(orgId) than getSecrets(Entity.ORGANIZATION, orgId) because it has less parameters.
There was a problem hiding this comment.
So, not a must to address? Because I'd prefer not to.
There was a problem hiding this comment.
No, not a must, but as this is still a draft maybe there is some time to get an opinion from another contributor (@oheger-bosch @MarcelBochtler @bs-ondem)?
| @@ -221,7 +223,7 @@ fun Route.organizations() = route("organizations") { | |||
| val organizationId = call.requireIdParameter("organizationId") | |||
| val secretName = call.requireParameter("secretName") | |||
|
|
|||
| secretService.deleteSecretByOrganizationAndName(organizationId, secretName) | |||
| secretService.deleteSecret(Entity.ORGANIZATION, organizationId, secretName) | |||
There was a problem hiding this comment.
Similar to the comment on the first commit, it would be nice to have convenience functions like getRepositorySecret, deleteRepositorySecret, or listRepositorySecrets.
|
Not that convinced from the current proposal. If we change something in this API, we should go a step further and make it type-safe. This could be achieved by introducing value classes for the IDs of organizations, products, and repositories. I am not sure, does Kotlin support the inheritance of value classes from a sealed interface? Then we could have something like The repository then only needs one |
It does, and I like that idea. I'll probably follow-up on that later. |
No description provided.