-
Notifications
You must be signed in to change notification settings - Fork 570
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
FSET transforms field names to lowercase #741
Comments
Good catch. Can confirm. 👍 |
Thanks for looking into this @iwpnd. After looking around a bit more, I am now certain that this issue has deeper roots than I originally anticipated. Apart from tile38/internal/server/search.go Line 174 in 51e6862
To reproduce:
The This is related to issue #704. The fixes for both |
Can also confirm. Let's see whats going on with update: Can I address this @tidwall? It doesn't appear to be intended, considering you can set a field in just about any case. Also this would align the behaviour of |
@iwpnd yes please take a look. I don’t recall this being the intended behavior. |
Ok, I'll review asap. |
Fixed and merged! |
Describe the bug
The command
FSET
transforms any given field name to lowercase.SET
ing a key with fields that have mixed or uppercase names works correctly.To Reproduce
Set an arbitrary key with an uppercase field
SET user 1 FIELD TEST 123 POINT 90 90
Get the key
GET user 1 WITHFIELDS
Set the field to a new value
FSET user 1 TEST 987
Get the key again
GET user 1 WITHFIELDS
Observe that we now have a new field named "test", instead of the original "TEST" field being updated.
Expected behavior
FSET
should retain the field name's letter casing.Additional context
I don't have experience in Go, but my initial assumption is that the culprit logic is likely this:
tile38/internal/server/crud.go
Lines 843 to 860 in 51e6862
Instead of calling ToLower on 844, it should be called directly on the switched value at 845, similar to how
SET
works.The text was updated successfully, but these errors were encountered: