Skip to content

fix hincrbyfloat not to create a key if the new value is invalid#11149

Merged
oranagra merged 3 commits intoredis:unstablefrom
chendq8:fixhincr
Aug 28, 2022
Merged

fix hincrbyfloat not to create a key if the new value is invalid#11149
oranagra merged 3 commits intoredis:unstablefrom
chendq8:fixhincr

Conversation

@chendq8
Copy link
Contributor

@chendq8 chendq8 commented Aug 19, 2022 •
edited
Loading

Check the validity of the value before performing the create operation, prevents new data from being generated even if the request fails to execute.

enjoy-binbin
enjoy-binbin reviewed Aug 19, 2022
View reviewed changes
Copy link
Contributor

@enjoy-binbin enjoy-binbin left a comment

Choose a reason for hiding this comment

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

can you add some top comment to describe the problem?
didn't bother to read through the code

@oranagra
Copy link
Member

oranagra commented Aug 21, 2022 •
edited
Loading

indeed. a detailed comment would help. @chendq8 please update.
looking at the test, i realize you found a bug in which a command can fail (return an error), but it could happen after already performing a medication to the database.

however, i don't think i like the way to fixed it (changing hashTypeLookupWriteOrCreate to it's components).
how about just copying the isnan and isinf to the top?
i.e. if the input isn't Inf or NaN, then adding 0 won't change it to be one of them.
or in other words, the first check will check the input, and the second one will only be relevant if the key already exists.

it does mean that we'll affect the error, e.g. cases that in the past would result with WRONGTYPE, will now be rejected with the NaN/Inf error, but i don't think that's an issue.

@chendq8
Copy link
Contributor Author

chendq8 commented Aug 22, 2022

indeed. a detailed comment would help. @chendq8 please update. looking at the test, i realize you found a bug in which a command can fail (return an error), but it could happen after already performing a medication to the database.

however, i don't think i like the way to fixed it (changing hashTypeLookupWriteOrCreate to it's components). how about just copying the isnan and isinf to the top? i.e. if the input isn't Inf or NaN, then adding 0 won't change it to be one of them. or in other words, the first check will check the input, and the second one will only be relevant if the key already exists.

it does mean that we'll affect the error, e.g. cases that in the past would result with WRONGTYPE, will now be rejected with the NaN/Inf error, but i don't think that's an issue.

The implementation mode has been modified.

enjoy-binbin
enjoy-binbin reviewed Aug 22, 2022
View reviewed changes
zuiderkwast
zuiderkwast approved these changes Aug 22, 2022
View reviewed changes
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

LGTM

@oranagra oranagra changed the title fix hincrbyfloat fix hincrbyfloat not to create a key if the new value is invalid Aug 28, 2022
@oranagra oranagra merged commit bc7fe41 into redis:unstable Aug 28, 2022
@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Aug 28, 2022
oranagra added a commit that referenced this pull request Apr 17, 2023
@chendq8 @oranagra @enjoy-binbin
fix hincrbyfloat not to create a key if the new value is invalid (#11149
e030e35 
)

Check the validity of the value before performing the create operation,
prevents new data from being generated even if the request fails to execute.

Co-authored-by: Oran Agra <[email protected]>
Co-authored-by: chendianqiang <[email protected]>
Co-authored-by: Binbin <[email protected]>
(cherry picked from commit bc7fe41)
(cherry picked from commit 606a385)
oranagra added a commit that referenced this pull request Apr 17, 2023
@chendq8 @oranagra @enjoy-binbin
fix hincrbyfloat not to create a key if the new value is invalid (#11149
1c1bd61 
)

Check the validity of the value before performing the create operation,
prevents new data from being generated even if the request fails to execute.

Co-authored-by: Oran Agra <[email protected]>
Co-authored-by: chendianqiang <[email protected]>
Co-authored-by: Binbin <[email protected]>
(cherry picked from commit bc7fe41)
oranagra added a commit that referenced this pull request Apr 17, 2023
@chendq8 @oranagra @enjoy-binbin
fix hincrbyfloat not to create a key if the new value is invalid (#11149
c924ac3 
)

Check the validity of the value before performing the create operation,
prevents new data from being generated even if the request fails to execute.

Co-authored-by: Oran Agra <[email protected]>
Co-authored-by: chendianqiang <[email protected]>
Co-authored-by: Binbin <[email protected]>
(cherry picked from commit bc7fe41)
(cherry picked from commit 606a385)
(cherry picked from commit 7df23a5)
@msotheeswaran-sc msotheeswaran-sc mentioned this pull request Apr 18, 2023
Merged
madolson pushed a commit to madolson/redis that referenced this pull request Apr 19, 2023
@chendq8 @oranagra
@enjoy-binbin @madolson
fix hincrbyfloat not to create a key if the new value is invalid (red…
a39684b 
…is#11149)

Check the validity of the value before performing the create operation,
prevents new data from being generated even if the request fails to execute.

Co-authored-by: Oran Agra <[email protected]>
Co-authored-by: chendianqiang <[email protected]>
Co-authored-by: Binbin <[email protected]>
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
@chendq8 @oranagra @enjoy-binbin
fix hincrbyfloat not to create a key if the new value is invalid (red…
25870d9 
…is#11149)

Check the validity of the value before performing the create operation,
prevents new data from being generated even if the request fails to execute.

Co-authored-by: Oran Agra <[email protected]>
Co-authored-by: chendianqiang <[email protected]>
Co-authored-by: Binbin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oranagra oranagra oranagra approved these changes

+2 more reviewers

@enjoy-binbin enjoy-binbin enjoy-binbin left review comments

@zuiderkwast zuiderkwast zuiderkwast approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release-notes indication that this issue needs to be mentioned in the release notes

Projects

Redis 6.2 Backport
Status: Done
Redis 7.0 Backport
Status: Done
Redis Triage
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chendq8 @oranagra @zuiderkwast @enjoy-binbin