Handling of Id and ClientId in Keycloak.Net.Core/Clients/KeycloakClient.cs
Opened this issue · 2 comments
Hi,
in Keycloak.Net.Core/Clients/KeycloakClient.cs (and elsewhere where client info is required), there are methods that take string clientId
as an input parameter to generate URLs to /admin/realms/{realm}/clients/...
. For example, GetClientAsync, UpdateClientAsync, etc.
However, looking at the documentation of Keycloak, the API wants the Id
property of Client, not ClientId
. Because the parameter gets pasted into an URL, your code still works as long as the Id is supplied in the call, obviously, but the name of the parameter is misleading. Using ClientId
in place of Id
gets rejected with 400 Bad Request by the API (and Keycloak's own frontend uses the Id in the relevant links, as well).
Is the name of the parameter deliberate? It's very easy to confuse it with Client.ClientId
.
Hi @jurc11,
I will admit that I have also found this confusing. The use of clientId
was carried over from the project that this one was forked from, so it was established long before I was involved in the project and I can't really comment on why the name was chosen.
It's worth noting that this pattern is present in most of the methods in this library, e.g. GetUserAsync(string realm, string userId)
which in the Keycloak documentation is GET /admin/realms/{realm}/users/{id}
. If I had to guess as to why this pattern is used I would assume that it was created to avoid confusion when a method accepts multiple Ids, for example DeleteUserGroupAsync(string realm, string userId, string groupId)
could be confusing with the signature DeleteUserGroupAsync(string realm, string id, string groupId)
as it might not be clear what the id
parameter represents in this case (the User or the Group). The Keycloak documentation is able to get around this by showing the parameter as it is used in the request URL /users/{id}/groups/{groupId}
which makes it pretty obvious that the id
parameter is the Id of a User, unfortunately this is not true in C# methods.
This pattern works fine in most cases, but as you discovered the main disadvantage comes with the Client methods, as Clients have two Ids (ClientId
and Id
), so it is not clear which one clientId
is intended to represent.
The two solutions that I can think of to resolve this are:
- We add XML comments to the Client methods indicating that
clientId
should be the Id of the Client, not the ClientId - Change all of the
clientId
,userId
, etc. to justid
, which has the potential to cause confusion as described in the above example, so we fix one source of confusion but add another.
Do you have any other ideas as to how this might be resolved?
Hey,
I did figure out the naming pattern shortly after posting the original post. You are correct to point that out and I agree w.r.t. consistency. The naming scheme makes sense in light of that, except in this one case.
Solution (1) is a good idea, it would be at least something marking the ambiguous name. One other solution that comes to mind is using the same approach as with the Realm._Realm
property. There, the underscore is used to circumvent the "property with the same name as the type" issue. Here, you could name the property as ClientClientId
or simply _ClientId
, directly on the Client type, thus leading to the method parameter being named clientClientId
or _clientId
. Well, at least the former, the latter seems a bit stupid. Or just renaming the parameter, but not the actual property. That's inconsistent but possibly the most understandable.
I'll be in the process of building our KC related solution over the next week or two and I'll probably have a better a better understanding of the library by then. I may be able to suggest something better then. Given the low quality of suggestions above, doing nothing and closing this issue might be sensible, even.