After #9504 we treat subcommands as commands, it's a good step, but it also introduced ambiguity, i.e. get|key is a bad command or a base command get? Maybe you will say it's a base command because get doesn't have subcommands, but it's very confused to users.
Here are some examples:
1. About COMMAND INFO
* command info config|get returns subcommand config|get's info.
* command info get|key returns base command get's info.
2. About ACL setuser
* acl setuser test +config|get allows user execute subcommand config|get.
* acl setuser test +config|get|dir allows user execute subcommand config|get with first arg dir.
* acl setuser test +config|xxx returns error.
* acl setuser test +get|key allows user execute base command get with first arg key.
* acl setuser test +get|key1|key2 allows user execute base command get with the second arg key2, key1 lost.
To me the result is very confused, I thinks we should treat get|key as a bad command, and use another character instead of | to indicate the first arg.
BTW the first-arg rule is very difficult to understand IMHO, it's too flexible.
Comment From: guybe7
as i see it the bugs are: 1. command info get|key returns base command get's info. 2. acl setuser test +get|key1|key2 allows user execute base command get with the second arg key2, key1 lost.
the rest are by design
Comment From: soloestoy
beside these bugs, I still wanna revert the first-arg feature, the first-arg has different meanings in different commands:
1. for config|get it's a configuration's name, but now we support multi-param config in #9748, that may let user ignore the ACL rule, for example:
* acl setuser test on +config|get|dir, but test user can get requirepass by config get dir requirepass.
2. for mget it's a key name, but like config|get, it also can ignore ACL rule to get more key-values:
* acl setuser test on +mget|key1, user can get other keys by mget key1 key2 key3.
3. for migrate command, the first arg is host, maybe you mean just allow user transfer a key to a special host? I think it's not your purpose.
4. for eval, it's a script, the first-arg is nonsense here I think.
Comment From: oranagra
i don't like this first-arg feature, and i agree that it lost some of its "capabilities" when variadic CONFIG SET and CONFIG GET were added. but people still use it for other purposes (including SELECT), so removing it is a breaking change.
maybe redis 7.0 is our opportunity to remove it (when we add sub-commands that handle part of it in a better way).
alternatively, if we don't wanna completely remove it, we can support it only on commands with no sub-commands.
if we do that in 7.0, it means we didn't add the feature of matching argv[2] on commands with sub-commands (like config|set|dir), and then it'l at least be easier to remove the first-arg completely later (won't break the new patters that the current code in unstable adds).
@redis/core-team PTAL and comment.
Comment From: soloestoy
i don't like this first-arg feature, and i agree that it lost some of its "capabilities" when variadic CONFIG SET and CONFIG GET were added. but people still use it for other purposes (including SELECT), so removing it is a breaking change.
we didn't release it yet (6.0/6.2 doesn't have this feature, it's just in unstable branch), so I don't think it's a breaking change.
Comment From: oranagra
the CONFIG|SET and SELECT|0 feature exists since 6.0, it was named sub_commands, and now it's named first_arg, but it's the same feature.
the new mechanism of sub-commands covers part of the use case for first-arg (it's intended use case), but not all (ab)use cases.
if we remove it, we would break the SELECT|0 and possibly others (even the ones using DEBUG, which we didn't make a container command).
so blocking it on sub-commands only, will mean we don't introduce a breaking change, and just avoid enabling new capabilities. removing it completely would be a breaking change, but since the sub-commands cover part of the past use cases, the break is maybe relatively small.
Comment From: soloestoy
oh, I see.. the sub command in 6.0 is actually implicitly include first-arg, what a ...
but it's not a good (right) use case I think, all document said it's subcommand not first-arg.
Comment From: oranagra
yes.. but we saw people use it.. here's one example: #8099. DEBUG command is another example.. (not currently a sub-command). maybe there are other non-container commands on which people use it.
Comment From: guybe7
alternatively, if we don't wanna completely remove it, we can support it only on commands with no sub-commands. if we do that in 7.0, it means we didn't add the feature of matching argv[2] on commands with sub-commands (like config|set|dir), and then it'l at least be easier to remove the first-arg completely later (won't break the new patters that the current code in unstable adds).
i think this is the best compromise
Comment From: guybe7
TBH i would really like to get rid of first-arg altogether but I feel like it's gonna break many existing use-cases... if I'm wrong and we think the damage would be minimal, let's get rid of it
Comment From: soloestoy
IMHO, select|0 it's an abuse case, we should get rid of it. And we can support database level ACL in another right way, it's a good requirement. Does ACL v2 support database control? Sorry I didn't look deep into #9974
Comment From: guybe7
decisions from discussion with @yossigo and @oranagra : 1. we will keep supporting first-arg for now, but not for subcommand (we feel removing this support will cause too much damage to existing users) 2. we will add a log message whenever someone is setting first-arg, warning about upcoming deprecation
Comment From: oranagra
i wanna stress that unlike the random flag, for which we don't know anyone who's using it and if there's any reason for using it (which is why i think we can afford to break it), here, we do know of use cases for it (SELECT and DEBUG), and we do know of people who rely on it, so although there's a bigger potential for tech-debt cleanup, i think it'll be wrong to break it.
also wanna mention that in the past we concluded that we don't wanna invest in database level ACL, we don't wanna promote the use of multiple databases in redis, it increases complexity and already unsupported in various configurations.
Comment From: soloestoy
it's OK to keep supporting first-arg for base commands.
also wanna mention that in the past we concluded that we don't wanna invest in database level ACL, we don't wanna promote the use of multiple databases in redis, it increases complexity and already unsupported in various configurations.
I don't think so, in our platform many users are using multiple databases (we even have to support it in cluster mode) and they also want database level ACL, we can discuss after 7.0.