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

FSET transforms field names to lowercase #741

Closed
unendingblue opened this issue May 24, 2024 · 7 comments · Fixed by #742 or #743
Closed

FSET transforms field names to lowercase #741

unendingblue opened this issue May 24, 2024 · 7 comments · Fixed by #742 or #743

Comments

@unendingblue
Copy link

Describe the bug
The command FSET transforms any given field name to lowercase.
SETing 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

Response
{"ok":true,"object":{"type":"Point","coordinates":[90,90]},"fields":{"TEST":123},"elapsed":"19.744µs"}

Set the field to a new value
FSET user 1 TEST 987

Get the key again
GET user 1 WITHFIELDS

Response
{"ok":true,"object":{"type":"Point","coordinates":[90,90]},"fields":{"TEST":123,"test":987},"elapsed":"16.374µs"}

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:

for i := 3; i < len(args); i++ {
arg := strings.ToLower(args[i])
switch arg {
case "xx":
xx = true
default:
fkey := arg
i++
if i == len(args) {
return retwerr(errInvalidNumberOfArguments)
}
if isReservedFieldName(fkey) {
return retwerr(errInvalidArgument(fkey))
}
fval := args[i]
fields = append(fields, field.Make(fkey, fval))
}
}

Instead of calling ToLower on 844, it should be called directly on the switched value at 845, similar to how SET works.

@iwpnd
Copy link
Contributor

iwpnd commented May 24, 2024

Good catch. Can confirm. 👍

iwpnd added a commit to iwpnd/tile38 that referenced this issue May 25, 2024
@unendingblue
Copy link
Author

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 FSET, this issue is also present in NEARBY's WHEREIN. I assume the reason is this function's use of ToLower:

func (s *Server) cmdSearchArgs(

To reproduce:
SET user 1 FIELD TEST 123 POINT 90 90
NEARBY user POINT 90 90 -> Observe the key is found
NEARBY user WHEREIN TEST 1 123 POINT 90 90 -> Observe the key is not found

SET user 2 FIELD test 123 POINT 90 90
NEARBY user WHEREIN TEST 1 123 POINT 90 90 -> Observe the key is found, even though the search term is uppercase while the key's field name is lowercase

The WHERE search option does work correctly. To confirm:
NEARBY user WHERE TEST 123 123 POINT 90 90 -> Observe the correct key (user 1) is found
NEARBY user WHERE test 123 123 POINT 90 90 -> Observe the correct key (user 2) is found

This is related to issue #704. The fixes for both FSET and WHEREIN may be a breaking change for systems that have not noticed this bug, though I still do believe ironing it out is the way to go as the current behavior is inconsistent.

@iwpnd
Copy link
Contributor

iwpnd commented May 26, 2024

Can also confirm. Let's see whats going on with wherein 👀

update:
yes, whereins are appended with strings.ToLower(name) too, here.

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 WHEREIN and WHERE.

@tidwall
Copy link
Owner

tidwall commented May 26, 2024

@iwpnd yes please take a look. I don’t recall this being the intended behavior.

@iwpnd
Copy link
Contributor

iwpnd commented Jun 2, 2024

#742 and #743 should resolve this now @tidwall

@tidwall
Copy link
Owner

tidwall commented Jun 3, 2024

Ok, I'll review asap.

@tidwall
Copy link
Owner

tidwall commented Jun 4, 2024

Fixed and merged!
@unendingblue Thanks for reporting!
@iwpnd Thanks for fixing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants