Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maintain order in AclSetuserArgs #1848

Closed
wants to merge 1 commit into from

Conversation

RohanNagar
Copy link
Contributor

Make sure that:

  • You have read the contribution guidelines.
  • You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

Closes #1847.

@mp911de this is my attempt at fixing the bug #1847. I added internal private classes that represent each type of argument and they all inherit from a private interface SetuserArg, which has a build method that adds the argument. AclSetuserArgs then just maintains a List of SetuserArg which allows for maintaining order when adding arguments.

A few other things fixed in this change:

  • Previously, AclSetuserArgs was always adding the off argument if none of on() or off() were called. This could result in a bug where a user of the client would expect something like AclSetuserArgs.Builder.addPassword("pass") to not change the on/off state. Now, on or off arguments will only be added if they are added to the builder.
  • Previously, if you used the reset() option on AclSetuserArgs, then no other arguments would be added to the command. Redis allows for a command like: acl setuser test reset on +hello. Such a command would first reset the user to a newly created state, and then turn it on and allow the hello command. This has been fixed so that any commands can be included with the reset command.
  • The resetpass argument was missing, I have added that argument here as well.

Please let me know what you think about this change. Thank you!

@mp911de
Copy link
Collaborator

mp911de commented Sep 9, 2021

Great work, it's exactly what we discussed. I'm going to take this PR from here.

@mp911de mp911de added the type: task A general task label Sep 9, 2021
mp911de pushed a commit that referenced this pull request Sep 9, 2021
mp911de added a commit that referenced this pull request Sep 9, 2021
Original pull request: #1848.
mp911de pushed a commit that referenced this pull request Sep 9, 2021
mp911de added a commit that referenced this pull request Sep 9, 2021
Original pull request: #1848.
@mp911de mp911de added this to the 6.1.5 milestone Sep 9, 2021
@mp911de
Copy link
Collaborator

mp911de commented Sep 9, 2021

Thank you for your contribution. That's merged, polished, and backported now.

@mp911de mp911de closed this Sep 9, 2021
@RohanNagar RohanNagar deleted the aclsetuser-ordering branch September 9, 2021 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ACL SETUSER not retaining argument order
2 participants