Skip to content

Make BITFIELD_RO reject OVERFLOW subcommand #9930

Closed
huangzhw wants to merge 2 commits into
redis:unstablefrom
huangzhw:bitfield_ro
Closed

Make BITFIELD_RO reject OVERFLOW subcommand #9930
huangzhw wants to merge 2 commits into
redis:unstablefrom
huangzhw:bitfield_ro

Conversation

@huangzhw

Copy link
Copy Markdown
Contributor

make BITFIELD with only GET subcommand reject OVERFLOW subcommand.

@oranagra oranagra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@huangzhw can you please add tests to cover these checks?

@huangzhw

Copy link
Copy Markdown
Contributor Author

Added.

@oranagra oranagra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@redis/core-team please approve or share your thoughts.
note that this is a breaking-change, since an argument that used to be ignored will now error.

@oranagra oranagra added approval-needed Waiting for core team approval to be merged breaking-change This change can potentially break existing application release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus labels Dec 12, 2021

@yossigo yossigo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, I'm not sure such a breaking change is generally justified but as this command was only introduced in 6.2 it makes more sense.

@oranagra

Copy link
Copy Markdown
Member

@yossigo the BITFIELD_RO was introduced in 6.2, but this PR also affects plain BITFIELD.
i'm also on the fence here thinking that maybe it's sometimes ok go silently skip args that are meaningless (to avoid breakage).

the real exception is obviously when security is a concern (Like silently skipping an ACL arg, which can lead to user thinking something was done when it wasn't).

bottom line, if we wanna "fix" this, 7.0 is our chance. but since this is harmless, i'm ok with not fixing this.

@yossigo

yossigo commented Dec 19, 2021

Copy link
Copy Markdown
Collaborator

@oranagra If everyone agrees the original problem is indeed harmless, I lean towards not breaking anything to fix it.

@oranagra

Copy link
Copy Markdown
Member

that's what i'm leaning towards. let's seek a 3rd person opinion before closing.
@itamarhaber WDYT? (this is a breaking change to fix a harmless problem?)

@madolson madolson self-requested a review December 21, 2021 06:52
@madolson

Copy link
Copy Markdown
Contributor

I would generally vote to not break backwards compatibility, this doesn't seem like a very important change.

@oranagra

Copy link
Copy Markdown
Member

great. so we all agree it's a good fix, but also that it doesn't worth breaking compatibility.
i'll close it then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval-needed Waiting for core team approval to be merged breaking-change This change can potentially break existing application release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants