Skip to content

Fix case sensitivity in KV.get#53

Merged
zerowidth merged 5 commits intomasterfrom
fix-case-sensitivity
Apr 7, 2021
Merged

Fix case sensitivity in KV.get#53
zerowidth merged 5 commits intomasterfrom
fix-case-sensitivity

Conversation

@zerowidth
Copy link
Contributor

@zerowidth zerowidth commented Mar 22, 2021

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.

@zerowidth zerowidth mentioned this pull request Mar 22, 2021
Open
jshorty
jshorty approved these changes Apr 7, 2021
View reviewed changes
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
Contributor Author

zerowidth commented Apr 7, 2021

Released as 0.5.2.

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

Reviewers

@jshorty jshorty jshorty approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@zerowidth @jshorty