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

Add type check before metadata decode #2015

Merged
merged 11 commits into from
Jan 17, 2024
Merged

Conversation

jihuayu
Copy link
Member

@jihuayu jihuayu commented Jan 14, 2024

In this PR I add type check before metadata decode.
I also add some test cases about operating with the wrong type
It fixes #2012

Comment on lines +38 to +47
message := "ERR Invalid argument: WRONGTYPE Operation against a key holding the wrong kind of value"
require.NoError(t, rdb.Set(ctx, "a", "hello", 0).Err())
require.EqualError(t, rdb.Do(ctx, "XADD", "a", "*", "a", "test").Err(), message)
require.EqualError(t, rdb.Do(ctx, "LPUSH", "a", 1).Err(), message)
require.EqualError(t, rdb.Do(ctx, "HSET", "a", "1", "2").Err(), message)
require.EqualError(t, rdb.Do(ctx, "SADD", "a", "1", "2").Err(), message)
require.EqualError(t, rdb.Do(ctx, "ZADD", "a", "1", "2").Err(), message)
require.EqualError(t, rdb.Do(ctx, "JSON.SET", "a", "$", "{}").Err(), message)
require.EqualError(t, rdb.Do(ctx, "BF.ADD", "a", "test").Err(), message)
require.EqualError(t, rdb.Do(ctx, "SADD", "a", 100).Err(), message)
Copy link
Member

@PragmaTwice PragmaTwice Jan 14, 2024

Choose a reason for hiding this comment

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

I think the test case can pass without these changes, regardless of error messages.

Copy link
Member Author

Choose a reason for hiding this comment

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

This test case is designed to ensure that the error message is consistent with Redis.
Based on my experience, it's difficult for me to associate the problem with inconsistent key types when I see other error messages.
So I wrote this test case to prevent future users from being confused by meaningless error messages.

Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

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

See comments : )

@PragmaTwice
Copy link
Member

Instead, if you want to fix #2012, I think a better way is to change Metadata::ParseMetadata.

Copy link

sonarcloud bot commented Jan 16, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.6% Duplication on New Code

See analysis details on SonarCloud

@jihuayu
Copy link
Member Author

jihuayu commented Jan 16, 2024

@PragmaTwice
I delay InvalidArgument error check after type match check in Metadata::ParseMetadata.

For the tests, I think it is necessary.

@git-hulk git-hulk requested a review from PragmaTwice January 16, 2024 09:34
Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

sonarcloud bot commented Jan 17, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

7 New issues
0 Security Hotspots
No data about Coverage
0.4% Duplication on New Code

See analysis details on SonarCloud

@git-hulk git-hulk merged commit 0723ae1 into apache:unstable Jan 17, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XADD to other type key returns wrong error message
4 participants