Add ACL GETUSER Command Initial Implementation #959
Conversation
| /// Returns flags for the <see cref="User"/>. | ||
| /// </summary> | ||
| /// <returns>A <see cref="List{T}"/> of <see cref="string"/> representing flags for the user.</returns> | ||
| public List<string> GetFlags() |
There was a problem hiding this comment.
Why allocate here? It's just switching between two constants.
| /// <returns>A <see cref="List{T}"/> of <see cref="string"/> representing password hashes for the user.</returns> | ||
| public List<string> GetPasswordHashes() | ||
| { | ||
| return _passwordHashes.Select(hash => $"#{hash}").ToList(); |
There was a problem hiding this comment.
This is not thread safe, _passwordHashes is mutable.
Also, there's no need to allocate here, we're just prepending a # so that can be done while writing the response out.
There was a problem hiding this comment.
Are you onboard with making this field accessible to clients to avoid the allocations?
There was a problem hiding this comment.
Given the thread safety concerns, I don't think exposing the field is appropriate.
| while (!RespWriteUtils.TryWriteArrayLength(6, ref dcurr, dend)) | ||
| SendAndReset(); | ||
|
|
||
| var flags = user.GetFlags(); |
There was a problem hiding this comment.
I think broadly this will run into issues if the user is modified while the command is running.
GetFlags(), GetPasswordHashes(), and GetEnabledCommandsDescription() won't necessarily return consistent results.
There was a problem hiding this comment.
I see that enabledCommands has a copy method. What are your thoughts on returning a copy of the commands back to the client to avoid any issues with modifications?
There was a problem hiding this comment.
I think some variant of "one method (that internally takes appropriate) locks, and copies the relevant data into the return"-is the only path forward.
I'd be tempted to try and fit things into Span's created by the caller to avoid allocations (or rent some arrays, or something), so the there's some flexibility in the implementation.
This PR provides the implementation of the
ACL GETUSERcommand which is used to obtain ACL rules for a specified user. The change is accompanied by tests for the command.The changeset includes:
This resolves #958 where further details can be found.
Additionally a few instances of unnecessary whitespace were cleaned up.