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

Support for ACL commands #1538 #1602

Closed
wants to merge 1 commit into from

Conversation

sokomishalov
Copy link
Collaborator

Draft PR for ACL commands API.
My main concern - should we provide higher abstractions for rules?

@mp911de
Copy link
Collaborator

mp911de commented Feb 1, 2021

Good first draft. Let's move AclCategory into an api package so we don't depend on the model package.

Can we also make the username a String instead of using the keytype K?

Re rules abstraction: I think it makes sense to provide an args object to provide rules.

@mp911de
Copy link
Collaborator

mp911de commented Feb 3, 2021

In preparation for Lettuce 6.1 M1, I'd like to move this change to M2 so that we can ship 6.1 M1 in the next days.

@mp911de mp911de added this to the 6.1 M2 milestone Feb 3, 2021
@codecov
Copy link

codecov bot commented Feb 5, 2021

Codecov Report

Merging #1602 (a12d34c) into main (4a98e5b) will decrease coverage by 0.47%.
The diff coverage is 52.89%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1602      +/-   ##
============================================
- Coverage     78.77%   78.30%   -0.48%     
- Complexity     6443     6509      +66     
============================================
  Files           481      487       +6     
  Lines         21735    22061     +326     
  Branches       2377     2412      +35     
============================================
+ Hits          17122    17275     +153     
- Misses         3525     3671     +146     
- Partials       1088     1115      +27     
Impacted Files Coverage Δ Complexity Δ
...io/lettuce/core/AbstractRedisReactiveCommands.java 86.91% <0.00%> (-2.51%) 442.00 <0.00> (ø)
.../io/lettuce/core/models/command/CommandDetail.java 100.00% <ø> (ø) 18.00 <0.00> (ø)
.../java/io/lettuce/core/output/ListOfMapsOutput.java 90.47% <0.00%> (-9.53%) 7.00 <0.00> (ø)
...e/api/coroutines/RedisAclCoroutinesCommandsImpl.kt 6.25% <6.25%> (ø) 1.00 <1.00> (?)
src/main/java/io/lettuce/core/AclSetuserArgs.java 19.87% <19.87%> (ø) 10.00 <10.00> (?)
...n/java/io/lettuce/core/output/UserRulesOutput.java 35.71% <35.71%> (ø) 5.00 <5.00> (?)
...in/java/io/lettuce/core/output/EnumListOutput.java 76.47% <76.47%> (ø) 5.00 <5.00> (?)
...va/io/lettuce/core/AbstractRedisAsyncCommands.java 96.31% <100.00%> (+0.11%) 467.00 <15.00> (+15.00)
src/main/java/io/lettuce/core/AclCategory.java 100.00% <100.00%> (ø) 1.00 <1.00> (?)
...main/java/io/lettuce/core/RedisCommandBuilder.java 95.46% <100.00%> (+0.14%) 511.00 <16.00> (+16.00)
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a98e5b...a12d34c. Read the comment docs.

@sokomishalov sokomishalov force-pushed the feature/acl branch 12 times, most recently from cc83288 to 232fbe7 Compare February 9, 2021 00:07
@sokomishalov sokomishalov marked this pull request as ready for review February 9, 2021 00:15
@sokomishalov
Copy link
Collaborator Author

I've made some fixes according to the first review

* @param <V> Value type.
* @author Mikhael Sokolov
*/
public class EnumListOutput<K, V, E extends Enum<E>> extends CommandOutput<K, V, List<E>> implements StreamingOutput<E> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be rather an EnumSet. I will take care of this during the merge.

@mp911de
Copy link
Collaborator

mp911de commented Mar 2, 2021

Thank you for your contribution. That's merged and polished now. I removed UserRulesOutput because it didn't consider nested lists. We could introduce a parser for List<Object> that creates an user object and that can be translated into ACL SET USER args.

@mp911de mp911de closed this Mar 2, 2021
@mp911de mp911de linked an issue Mar 2, 2021 that may be closed by this pull request
mp911de pushed a commit that referenced this pull request Mar 2, 2021
Original pull request: #1602.
mp911de added a commit that referenced this pull request Mar 2, 2021
Tweak Javadoc. Remove UserRulesOutput in favor of introducing a model parser later on.
Switch Enum output to construct an enum set.
Reformat code. Update since tags.

Original pull request: #1602.
@sokomishalov sokomishalov deleted the feature/acl branch March 9, 2021 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for ACL commands
2 participants