MOD-8036 Add tests for 393 and 394

[ephraimfeldblum]

• Fixes the problem of naming the meta-variable class $acl_category identically to its identifier.
• Reduces code duplication on the module side (eg RedisJSON), all commands from the module are given the ACL category of the module without having to be expressly listed per command.
  (This is not guaranteed to be correct behavior for all modules. removed).
• Adds a test on the redismodule-rs side to ensure it works as expected.
• Allows for a combination of optional ACL categories and mandatory ACL categories to allow for the case of optionally adding categories to commands depending on the state of the Redis Server being accessed (ie v7.2 will fail to add a new ACL category, and therefore any commands that should be linked to that category should be optional). Failing to add optional categories results in a warning. Failing to add mandatory categories is an error.

#393
#394

[eranhd]

please elaborate what is this PR come to fix

[ephraimfeldblum]

please elaborate what is this PR come to fix

• it fixes the problem of naming the meta-variable class $acl_category identically to its identifier.
• it reduces code duplication on the module side (eg RedisJSON), all commands from the module are given the ACL category of the module without having to be expressly listed per command.
• it adds a test on the redismodule-rs side to ensure it works as expected.

[MeirShpilraien]

Please modify ACL test to verify the new functionality, and also add this option to the command proc macro (and verify it with test)

[iddm]

For the simplicity, I'd do two things:

1. Add a separate enum: CommandAclCategory:

enum CommandAclCategory {
    Write,
    Read,
    Fast,
    Other(String),
}

impl std::fmt::Display for CommandAclCategory {
    // ...
}

Where we would add all the already existing, commonly used acl categories which have already existed for a long time, and would always specifying a new one by having the Other variant.

2. If the mandatory ACL category is mandatory, why not simply omit it and add it automatically? We are not in C, so we can have two macros, with it and without. In the "without" macro branch, I'd invoke the "with" branch with the mandatory ACL category name as the module name (this is the case for all the modules we are currently adding the ACLs anyway).
3. Rename the acl_categories arguments, as we specify them separately, not all within one argument: mandatory_acl_category and optional_acl_category.

[ephraimfeldblum]

1. doable. will take under advisement.
2. there is no one singular mandatory ACL category. the mandatory ACL categories are those that are mandatory for the module to work at all, and loading the module should fail if they are not added. optional ACL categories are those that we warn on failure to add, but do not fail.
3. we do specify multiple categories within one argument. as a space delimited string.

[iddm]

Thanks for the response!

How we want to specify them for redis - this is the implementation detail. The interface here is in Rust, and we should allow the safest, most idiomatic way possible, which is an explicit set of type-safe values. Do you agree? For the user of this macro it shouldn't matter how exactly those end up being sent to the server, the user shouldn't care about this, nor should he know about this, if this is hidden behind the interface. This is not why this crate is simply forwarding the rust types to the C-api, right?

[ephraimfeldblum]

it's the user that decides which categories are mandatory and which are optional, not us.

[iddm]

Yes, okay, that I am not discussing anymore. I am talking now about the type safety and the explicit lists.

[iddm]

Again, this is plural but represents just one category; IIRC, there is just one mandatory category now, or am I wrong? Either way, I understand the precaution here of actually passing more than one as a string separated with spaces, but I'd really like to have it more verbose and explicit by having the enum and a [,] expression instead then.

[ephraimfeldblum]

there may be many mandatory categories. mandatory just means that if adding any of these categories fails, the module loading should fail.

[iddm]

Alright, then I'd like to have those not as string literals but as a comma-separated values list, which implements From<T> for CommandAclCategory enum either way, so either a string, or a CommandAclCategory enum variant itself.

[iddm]

Nice! I don't strictly require any changes, just suggesting a few more things, but this is good to go.

[iddm]

What should the result be in the case of read or read ? Is it even possible?

[ephraimfeldblum]

error?
passing that to Redis will be rejected

[iddm]

I'd personally add a method for this AclCategory::is_none() and is_some(). It may clash with the interface of Option and confuse the reader, though, but there is no better alternative.

[iddm]

Can we just $optional_acl_categories.to_string()?

[ephraimfeldblum]

it needs to be a c_string to pass to Redis

[iddm]

I know, I meant instead of using the format! macro we could have simply used the to_string() method of the fmt::Display trait, no?