don't we have that problem in many many commands?
fixing it is a breaking change, so from one perspective, maybe we rather not fix it.
on the other hand, users that accidentally use it, may have a bug in their application and might be using the wrong value, in which case triggering an error will alert them on their bug (but might also silently break the app without anyone noticing too).
@yossigo what do you think? (considering this might be a wide spread issue)
i.e. the problem existed in set k v ex 123 ex 123 since forever.

@oranagra I tend to agree with you. This is not necessarily a bug and if we're able to define how this uncommon case is expected to behave across all commands, that may be a better option vs. changing behavior.

FWIW, I think it should be considered a bug and fixed. The documented syntax only allows one option. Nobody wants hidden bugs in their application.

Btw, the PR seems to affect all SET and GET related commands with expire options. It needs to be reflected in the title and top comment.

The problem is even deeper than just these two commands, if we wanna considering fixing it, we need to first compose a full list of the affected commands.

Note that even fixing it can possibly lead to bugs in some apps, the ones who log the error and keep running. with the above example, in the past these some set and expiration succeed, and now it would be empty.

If we don't fix it, then I think we should document the behaviour (the last occurrence of the duplicated option wins) so that we can avoid future PRs fixing this again and again. (I think I have seen some similar fix before, with a similar discussion.)

In either case new tests would be a good idea.

GETEX K V EX ttl1 EX tt2
SET K V PXAT ttl1 PXAT ttl2
etc.

The same tests for both cases, just different expected results.

@oranagra "we need to first compose a full list of the affected commands"

There are exactly 2 affected commands: GETEX and SET. Those are the only commands that call the code with the bug ... oops, I mean the undocumented feature :)

@daniel-house these are the two commands that call the code modified here. but there are a ton of other commands that probably do the same practice.
for instance, RESTORE IDLETIME.
there's no sense to start patching SET and GETEX if we don't consider the full picture.

@oranagra I'm not sure what you mean by "the same practice". I looked at the code for RESTORE and I might make a guess that you mean "any command that takes options". Could you please elaborate on your criteria for a command being affected?

yes, any command that takes argument, for instance
RESTORE <key> <ttl> <payload> idletime 10 idletime 20
COPY <key> <dst> DB 2 DB 3

My vote would be to go through all of the commands that have the issue and fix it for Redis 7. I think some specific use cases aren't a big deal, since adding the flag multiple times is idempotent (ZADD with XX for example), but the other cases where providing the same argument multiple times can cause different behavior can be a source of bugs.

My vote would be to go through all of the commands that have the issue and fix it for Redis 7. I think this specific use case isn't a big deal, since adding the flag multiple times is idempotent, but the other cases where providing the same argument multiple times can cause different behavior can be a source of bugs.

I could help community summarize the command list, and find a proper way to solve this issue.

@madolson The specific use case that I am thinking of is GETEX key EX ttl1 EX ttl2. In this case adding EX is only idempotent when ttl1 = ttl2. You mentioned a flag, so may be you are thinking of a different use case.

Note, my testing shows that GETEX key EX a-non-integer-string EX a-valid-ttl will be accepted, which is weird. This happens because the code does not attempt to parse a-non-integer-string.

@daniel-house Yes, my brain was not working. The getex case is an example where it does matter. I was thinking about cases like ZADD with XX (Which my brain somehow made the jump too)

I agree that in some cases like ZADD with XX adding the same flag more than once does not change the behavior. However, if I do something like that it will be a mistake on my part. I find it difficult to imagine a use case where anyone would want to do that. My personal preference when I write queries is to get strict feedback about mistakes as early as possible, and for me a mistake like this could be a symptom of bigger problems.

we discussed this in a core-team meeting, and came to two important realizations:

1. most apps use a client library and not form their own command arguments, so they're unlikely to hit this.
2. fixing this could cause nasty data inconsistencies in upgrade. i.e. you upgrade the replica first, it'll then start rejecting commands that successfully executed on the master.

so if we wanna fix it safely, we may need to reject commands with repeated arguments, only when they didn't originate from the master or AOF client.

i went over other breaking changes we recently made, to check which of them could have caused issues of rejecting (or resulting with a different outcome) when executed on an upgraded replica, vs it's master.

a few examples that are not a problem:

• Set default channel permission to resetchannels for 7.0 #10181 - not a problem because the master client doesn't have ACL
• Make BITFIELD_RO reject OVERFLOW subcommand  #9930 - not a problem because it's an RO command that's not propagated
• Obliterate STRALGO! add LCS (which only works on keys) #9799 - RO command
• Fail Exec command in case a watched key is expired #9194 - master client doesn't watch or check for expired keys
• GEO* STORE with empty src key delete the dest key and return 0, not empty array #9271 - due to the bug the command will NOP on the master, and not get propagated.

the only one i could find that can cause such issues are:

• Fix accidental deletion of sinterstore command when we meet wrong type error. #9032 (already released in 6.2.5 and 6.0.15)
  in which case the master will delete the key and propagate the command, and the replica will fail that command (silently).

This realization means that not only that we must always respect commands propagated from masters / AOF, we might also have to be aware which version of redis was the source of that command.
i.e. if a command can be executed in two different ways we need to know how the master executed it and be "bug" compatible with that specific version.

@redis/core-team FYI.

2 more interesting cases:

• EXPIRE, EXPIREAT, SETEX, GETEX: Return error when expire time overflows #8287 (released in 6.2.0)
  Will be executed on an unfixed master, and fail on the fixed replica.
• Add tests to cover EXPIRE overflow detection fix #9839 (part of 7.0)
  same