Extend the API with a function which updates the client configuration #1275
Conversation
|
@srenatus Any chance to get this merged soon? Thanks |
|
@srenatus Do you see any issues which prevent merging this PR? |
There was a problem hiding this comment.
Sorry for radio silence on this for so long. Thank you for your contribution. 😃
Looks good so far -- I think the UpdateClientReq needs some wrappers (see inline comments). Also, to confidently merge this, I'd need some tests; do you think you could add some here? I guess we currently don't have too much in place around the client bits of the API, but that's for no good reason as far as I can tell... 🤔
server/api.go
Outdated
| if req.TrustedPeers != nil && len(req.TrustedPeers) > 0 { | ||
| old.TrustedPeers = req.TrustedPeers | ||
| } | ||
| old.Public = req.Public |
There was a problem hiding this comment.
So, we're "patchy" for Name and LogoUrl -- empty-string means no update -- but not for Public? There is ways to do this in GRPC, so I wonder if we want to do that.
Concretely, if Public was of type BoolValue, we could differentiate "not provided" from "false". I suppose if we do that, we should do the same with the other fields; as you currently cannot
- unset trusted peers
- unset redirect URIs
- set the name to empty-string
- set the logoURL to empty-string
💭 I guess one could argue whether this is needed. But as it stands, you cannot update the client without setting its public value; that at least should be fixed IMHO
There was a problem hiding this comment.
What would be your suggestion in this case? if it is a private client and the user tries to update to public we should return an error?
There was a problem hiding this comment.
It seems that I need to use an external package in the proto file in order to get these types working.
import "google/protobuf/wrappers.proto";
Some details here: golang/protobuf#438
I can't get it working. If you have any suggestion how to make this working, I am up to it.
An alternative solution is to not update the Public field in the update operation.
There was a problem hiding this comment.
I tried to address your concerns in the last commit. Please let me know if this solution is fine for you.
There was a problem hiding this comment.
Yeah, I'm not sure how to best deal with this one. An alternative to introducing wrappers could be to use a FieldMask in UpdateClientReq. And of course, we could just leave it as-is. What do you think, @ericchiang?
Re 49fa5ee, I'm not sure I follow... From what I can tell, it's now an append-only operation? I.e. whatever is in the UpdateClientReq.RedirectUris will be added to old.RedirectUris; but nothing could ever get removed? I'm not sure if this really is what we want... I think something like
if req.RedirectUris != nil {
old.RedirectURIs = req.RedirectUris
}would do the trick -- it lets you determine what the RedirectUris are after the update, and it allows you to set them to []string{}. Before, what last thing wasn't possible (unless I'm mistaken). Likewise for TrustedClients.
Thanks for bearing with me here 😃
There was a problem hiding this comment.
😄 This was what I understood from your first comment. I reverted the last commit, and also allow to update the redirect URIs and trusted peers to []string{}.
The Public filed update was removed as well. I think is not required to be updated after the initial registration.
Hope this is what you are expecting! Thanks
d092ad4 to
c9b18b2
Compare
|
@srenatus I addressed the review issues and also add some tests. Do you have any other concerns? Thanks |
|
@ccojocar thanks for adding those tests 👍 -- I'm not sure I understand how the other comment has been resolved, though; so I've unresolved it 😄 |
…g the client Also skip configure the Public field.
… updating the client" This reverts commit 49fa5ee.
|
@srenatus Are there any pending issues related to this PR? |
|
@ccojocar thanks for the bump. I'll look into this again today. 😃 |
api/api.proto
Outdated
| string id = 1; | ||
| repeated string redirect_uris = 2; | ||
| repeated string trusted_peers = 3; | ||
| bool public = 4; |
There was a problem hiding this comment.
I think we should either just overwrite the public field with whatever was in the UpdateClientReq (what you had pushed before; sorry about that) -- and ensure that documentation makes it obvious that the behaviour isn't patchy when it comes to public; or keep the code as-is and remove the field from this message.
There was a problem hiding this comment.
I would prefer to remove the public field since is not used. Please could you point me to the documentation, which should I update?
There was a problem hiding this comment.
The filed is removed.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Thank you for implementing this! 🎉
Re: docs -- it seems like the only docs we really have are the comments on the messages. Adding a note there that public cannot (currently) be updated seems superfluous, as there clearly is no public field. So, I think this is OK now. 👍
|
Ooops, I've merged this without checking the commits. I guess this could have been a candidate for a squash before merging. My fault! 😅 |
Extend the API with a function which updates the client configuration
No description provided.