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

Fix case sensitivity in KV.get #53

Merged
merged 5 commits into from
Apr 7, 2021
Merged

Fix case sensitivity in KV.get #53

merged 5 commits into from
Apr 7, 2021

Conversation

zerowidth
Copy link
Member

This PR fixes a bug reported internally:

[1] pry(main)> GitHub.kv.set("foo", "bar")
=> nil
[2] pry(main)> GitHub.kv.set("Foo", "baz")
=> nil
[3] pry(main)> GitHub.kv.get("foo")
=> #<GitHub::Result:0xd41c value: "baz">

The expected behavior was that foo and Foo would be treated as different keys. However, the MySQL default utf8 collation is case insensitive. This changes the fetch code to be case-insensitive to match the default MySQL behavior:

Before:

set "foo", "lower"
get "foo" # => "lower"
get "FOO" # => nil
set "FOO", "upper" # overwrites existing 'foo' key
get "foo" # => "upper", matching based on original key name
get "FOO" # => nil, since ruby "FOO" string doesn't match the "foo" key returned from the database

After:

set "foo", "lower"
get "foo" # => "lower"
get "FOO" # => "lower"
set "FOO", "upper"
get "foo" # => "upper"
get "FOO" # => "upper"

This doesn't help if you're expecting case sensitivity, but it makes the set/get behavior consistent.

Copy link

@jshorty jshorty left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! New to this codebase but given the default MySQL behavior, coercing to all-downcase keys outside of the query makes sense to me here.

@zerowidth zerowidth merged commit 25c64dc into master Apr 7, 2021
@zerowidth zerowidth deleted the fix-case-sensitivity branch April 7, 2021 22:07
@zerowidth
Copy link
Member Author

Released as 0.5.2.

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.

None yet

2 participants