tile38 icon indicating copy to clipboard operation
tile38 copied to clipboard

FSET transforms field names to lowercase

Open unendingblue opened this issue 1 year ago • 1 comments

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: https://github.com/tidwall/tile38/blob/51e686279795c608f94f8e8f57443713701694db/internal/server/crud.go#L843-L860

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

unendingblue avatar May 24 '24 16:05 unendingblue

Good catch. Can confirm. 👍

iwpnd avatar May 24 '24 16:05 iwpnd

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: https://github.com/tidwall/tile38/blob/51e686279795c608f94f8e8f57443713701694db/internal/server/search.go#L174

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.

unendingblue avatar May 26 '24 16:05 unendingblue

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

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.

iwpnd avatar May 26 '24 18:05 iwpnd

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

tidwall avatar May 26 '24 21:05 tidwall

#742 and #743 should resolve this now @tidwall

iwpnd avatar Jun 02 '24 13:06 iwpnd

Ok, I'll review asap.

tidwall avatar Jun 03 '24 19:06 tidwall

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

tidwall avatar Jun 04 '24 03:06 tidwall