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

chore(acl_family): add allcomands and nocommands #3783

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

Lakshyadevelops
Copy link
Contributor

Fixes #3458
TESTED = 1
I did not add tests as there was no test earlier for the same but it can be added.

@kostasrim kostasrim self-requested a review September 24, 2024 15:11
@romange romange added hacktoberfest Participates in Hacktoberfest hacktoberfest-accepted and removed hacktoberfest Participates in Hacktoberfest labels Sep 24, 2024
Copy link
Contributor

@kostasrim kostasrim left a comment

Choose a reason for hiding this comment

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

Hi @Lakshyadevelops,

Thank you so much for your contribution! Would you mind also adding a unit test ?

I also left a very small nit :)

@@ -990,6 +990,22 @@ using OptCat = std::optional<uint32_t>;
// bool == true if +
// bool == false if -
std::pair<OptCat, bool> AclFamily::MaybeParseAclCategory(std::string_view command) const {
if (absl::EqualsIgnoreCase(command, "ALLCOMMANDS")) {
auto res = cat_table_.find("ALL");
Copy link
Contributor

Choose a reason for hiding this comment

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

ALL will always exist in cat_table_ so you don't really need to call find and compare that cat_table_.end().

You can just use cat_table_["ALL"] :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you are right about comparing to cat_table_.end()

I tried to used operator[] but it was failing due to type mismatch and even after casting to std::string_view it was failing due to -fpermissive (this arguement discards qualifier) so finally used .at()

Copy link
Contributor

@kostasrim kostasrim left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @Lakshyadevelops

@kostasrim kostasrim merged commit fb2ee90 into dragonflydb:main Sep 25, 2024
9 checks passed
@Lakshyadevelops Lakshyadevelops deleted the allcommands branch September 25, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add allcomands and nocommands support for ACL's
3 participants