Fix incorrect error code for eval scripts and fix test error checking#10575
Fix incorrect error code for eval scripts and fix test error checking#10575oranagra merged 5 commits intoredis:unstablefrom
Conversation
| lappend rv [string match "ERR*not*float*" $smallerr] | ||
| lappend rv [string match "ERR*not*float*" $bigerr] | ||
| lappend rv [string match "ERR *not*float*" $smallerr] | ||
| lappend rv [string match "ERR *not*float*" $bigerr] |
There was a problem hiding this comment.
I'm not certain I would bother to change all the places that checked the error without space.
It is indeed slightly better, but adds unnecessary changes to many places.
Maybe the main advantage is that if these lines will get copied, people will get the right thing.
What's bothering me more, is why don't we see some similar fix in scripting.tcl. That's the one that missed the issue you're fixing.
There was a problem hiding this comment.
It's because I forgot to commit the second half of the changes. I only captured the "ERR* and not {ERR*.
I'm going to posit it's still worth doing, since people are going to copy from these. There are no other mistakes, but I think it's good hygiene here.
oranagra
left a comment
There was a problem hiding this comment.
ok, so the logic is that anything that just matches "ERR*" and no other text in the error message, can remain without a space, and everything that matches additional text after the error code, needs a space.
there are probably plenty of others matching specific error codes that are not ERR, like OOM, WRONGTYPE, but for these, it's usually enough to match just the error code, and i suppose no one bothered to match the error message.
i.e. we need to match the error message, just in cases where the error code is generic.
is all of that right?
|
@oranagra That is my logic yes. |
MeirShpilraien
left a comment
There was a problem hiding this comment.
👍 thanks @madolson nice catch.
…redis#10575) By the convention of errors, there is supposed to be a space between the code and the name. While looking at some lua stuff I noticed that interpreter errors were not adding the space, so some clients will try to map the detailed error message into the error. We have tests that hit this condition, but they were just checking that the string "starts" with ERR. I updated some other tests with similar incorrect string checking. This isn't complete though, as there are other ways we check for ERR I didn't fix. Produces some fun output like: ``` # Errorstats errorstat_ERR:count=1 errorstat_ERRuser_script_1_:count=1 ```
By the convention of errors, there is supposed to be a space between the code and the name. While looking at some lua stuff I noticed that interpreter errors were not adding the space, so some clients will try to map the detailed error message into the error.
We have tests that hit this condition, but they were just checking that the string "starts" with ERR. I updated some other tests with similar incorrect string checking. This isn't complete though, as there are other ways we check for ERR I didn't fix.
Produces some fun output like: