diff --git a/docs/migration-guide.md b/docs/migration-guide.md index ed2abbbd..b7d55026 100644 --- a/docs/migration-guide.md +++ b/docs/migration-guide.md @@ -11,7 +11,7 @@ The general actions needed are: All `libvalkey` headers are now found under `include/valkey/`. * Update used build options, e.g. `USE_TLS` replaces `USE_SSL`. -## Migrating from `hiredis` v1.2.0 +## Migrating from `hiredis` v1.2.0 or v1.3.0 The type `sds` is removed from the public API. @@ -92,6 +92,14 @@ The type `sds` is removed from the public API. * `HIRCLUSTER_FLAG_ADD_SLAVE` removed, flag can be replaced with an option, see `VALKEY_OPT_USE_REPLICAS`. * `HIRCLUSTER_FLAG_ROUTE_USE_SLOTS` removed, the use of `CLUSTER SLOTS` is enabled by default. +### Changed API function signatures + +* `valkeyClusterGetSlotByKey` now requires a `keylen` parameter of type `size_t`. + Previously the key length was determined using `strlen()`, which gave incorrect + results for keys containing embedded null characters. +* `valkeyClusterGetNodeByKey` now requires a `keylen` parameter of type `size_t`, + for the same reason as above. + ### Removed support for splitting multi-key commands per slot Since old days (from `hiredis-vip`) there has been support for sending some commands with multiple keys that covers multiple slots. diff --git a/include/valkey/cluster.h b/include/valkey/cluster.h index 61d1e442..f88849f9 100644 --- a/include/valkey/cluster.h +++ b/include/valkey/cluster.h @@ -336,9 +336,9 @@ LIBVALKEY_API void valkeyClusterInitNodeIterator(valkeyClusterNodeIterator *iter LIBVALKEY_API valkeyClusterNode *valkeyClusterNodeNext(valkeyClusterNodeIterator *iter); /* Helper functions */ -LIBVALKEY_API unsigned int valkeyClusterGetSlotByKey(char *key); +LIBVALKEY_API unsigned int valkeyClusterGetSlotByKey(char *key, size_t keylen); LIBVALKEY_API valkeyClusterNode *valkeyClusterGetNodeByKey(valkeyClusterContext *cc, - char *key); + char *key, size_t keylen); #ifdef __cplusplus } diff --git a/src/cluster.c b/src/cluster.c index 7a20240a..ba13057a 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -3426,12 +3426,12 @@ valkeyClusterNode *valkeyClusterNodeNext(valkeyClusterNodeIterator *iter) { } /* Get hash slot for given key string, which can include hash tags */ -unsigned int valkeyClusterGetSlotByKey(char *key) { - return keyHashSlot(key, strlen(key)); +unsigned int valkeyClusterGetSlotByKey(char *key, size_t keylen) { + return keyHashSlot(key, (int)keylen); } /* Get node that handles given key string, which can include hash tags */ valkeyClusterNode *valkeyClusterGetNodeByKey(valkeyClusterContext *cc, - char *key) { - return node_get_by_table(cc, keyHashSlot(key, strlen(key))); + char *key, size_t keylen) { + return node_get_by_table(cc, keyHashSlot(key, (int)keylen)); } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 15d82f57..a44c056c 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -97,6 +97,10 @@ target_include_directories(ut_slotmap_update PRIVATE "${PROJECT_SOURCE_DIR}/src" target_link_libraries(ut_slotmap_update valkey_unittest) add_test(NAME ut_slotmap_update COMMAND "$") +add_executable(ut_slot_by_key ut_slot_by_key.c) +target_link_libraries(ut_slot_by_key valkey_unittest) +add_test(NAME ut_slot_by_key COMMAND "$") + # Cluster tests add_executable(ct_commands ct_commands.c test_utils.c) target_link_libraries(ct_commands valkey ${TLS_LIBRARY}) diff --git a/tests/ct_out_of_memory_handling.c b/tests/ct_out_of_memory_handling.c index a7131819..1bd910cb 100644 --- a/tests/ct_out_of_memory_handling.c +++ b/tests/ct_out_of_memory_handling.c @@ -158,7 +158,7 @@ void test_alloc_failure_handling(void) { valkeyReply *reply; const char *cmd = "SET key value"; - valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"key"); + valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"key", 3); assert(node); int n; @@ -217,7 +217,7 @@ void test_alloc_failure_handling(void) { valkeyReply *reply; const char *cmd = "SET foo one"; - valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"foo"); + valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"foo", 3); assert(node); /* Discover allocations needed for a successful append to node. */ @@ -259,8 +259,8 @@ void test_alloc_failure_handling(void) { prepare_allocation_test(cc, 1000); /* Get the source information for the migration. */ - unsigned int slot = valkeyClusterGetSlotByKey((char *)"foo"); - valkeyClusterNode *srcNode = valkeyClusterGetNodeByKey(cc, (char *)"foo"); + unsigned int slot = valkeyClusterGetSlotByKey((char *)"foo", 3); + valkeyClusterNode *srcNode = valkeyClusterGetNodeByKey(cc, (char *)"foo", 3); int srcPort = srcNode->port; /* Get a destination node to migrate the slot to. */ @@ -318,7 +318,7 @@ void test_alloc_failure_handling(void) { * allowing a high number of allocations. */ prepare_allocation_test(cc, 1000); /* Fetch the nodes again, in case the slotmap has been reloaded. */ - srcNode = valkeyClusterGetNodeByKey(cc, (char *)"foo"); + srcNode = valkeyClusterGetNodeByKey(cc, (char *)"foo", 3); dstNode = getNodeByPort(cc, dstPort); reply = valkeyClusterCommandToNode( cc, srcNode, "CLUSTER SETSLOT %d NODE %s", slot, replyDstId->str); diff --git a/tests/ct_specific_nodes.c b/tests/ct_specific_nodes.c index 7eedcbb3..11b4803c 100644 --- a/tests/ct_specific_nodes.c +++ b/tests/ct_specific_nodes.c @@ -43,7 +43,7 @@ void test_command_to_all_nodes(valkeyClusterContext *cc) { void test_transaction(valkeyClusterContext *cc) { - valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"foo"); + valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"foo", 3); assert(node); valkeyReply *reply; @@ -71,7 +71,7 @@ void test_streams(valkeyClusterContext *cc) { char *id; /* Get the node that handles given stream */ - valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"mystream"); + valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"mystream", 8); assert(node); /* Preparation: remove old stream/key */ @@ -80,7 +80,7 @@ void test_streams(valkeyClusterContext *cc) { freeReplyObject(reply); /* Query wrong node */ - valkeyClusterNode *wrongNode = valkeyClusterGetNodeByKey(cc, (char *)"otherstream"); + valkeyClusterNode *wrongNode = valkeyClusterGetNodeByKey(cc, (char *)"otherstream", 11); assert(node != wrongNode); reply = valkeyClusterCommandToNode(cc, wrongNode, "XLEN mystream"); CHECK_REPLY_ERROR(cc, reply, "MOVED"); @@ -230,7 +230,7 @@ void test_pipeline_transaction(valkeyClusterContext *cc) { int status; valkeyReply *reply; - valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"foo"); + valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"foo", 3); assert(node); status = valkeyClusterAppendCommandToNode(cc, node, "MULTI"); @@ -466,7 +466,7 @@ void test_async_transaction(void) { valkeyClusterAsyncContext *acc = valkeyClusterAsyncConnectWithOptions(&options); ASSERT_MSG(acc && acc->err == 0, acc ? acc->errstr : "OOM"); - valkeyClusterNode *node = valkeyClusterGetNodeByKey(&acc->cc, (char *)"foo"); + valkeyClusterNode *node = valkeyClusterGetNodeByKey(&acc->cc, (char *)"foo", 3); assert(node); int status; @@ -498,6 +498,22 @@ void test_async_transaction(void) { event_base_free(base); } +void test_get_node_by_key(valkeyClusterContext *cc) { + /* A key with embedded null should use the full length for slot + * calculation, not stop at the null byte. */ + valkeyClusterNode *node_full = valkeyClusterGetNodeByKey(cc, (char *)"ke\0y", 4); + valkeyClusterNode *node_truncated = valkeyClusterGetNodeByKey(cc, (char *)"ke", 2); + assert(node_full != NULL); + assert(node_truncated != NULL); + assert(node_full != node_truncated); +} + +void test_get_node_by_key_empty(valkeyClusterContext *cc) { + /* An empty string is a valid key. */ + valkeyClusterNode *node = valkeyClusterGetNodeByKey(cc, (char *)"", 0); + assert(node != NULL); +} + int main(void) { valkeyClusterOptions options = {0}; options.initial_nodes = CLUSTER_NODE; @@ -512,6 +528,8 @@ int main(void) { test_command_to_all_nodes(cc); test_transaction(cc); test_streams(cc); + test_get_node_by_key(cc); + test_get_node_by_key_empty(cc); // Pipeline API test_pipeline_to_single_node(cc); diff --git a/tests/ut_slot_by_key.c b/tests/ut_slot_by_key.c new file mode 100644 index 00000000..08111906 --- /dev/null +++ b/tests/ut_slot_by_key.c @@ -0,0 +1,41 @@ +/* Unit tests for valkeyClusterGetSlotByKey. */ + +#include "cluster.h" +#include "test_utils.h" + +#include + +void test_slot_by_key_basic(void) { + /* A simple key should produce a consistent slot. */ + unsigned int slot = valkeyClusterGetSlotByKey((char *)"foo", 3); + assert(slot == valkeyClusterGetSlotByKey((char *)"foo", 3)); +} + +void test_slot_by_key_with_hashtag(void) { + /* Keys with the same hash tag should map to the same slot. */ + unsigned int slot1 = valkeyClusterGetSlotByKey((char *)"{tag}key1", 9); + unsigned int slot2 = valkeyClusterGetSlotByKey((char *)"{tag}key2", 9); + assert(slot1 == slot2); +} + +void test_slot_by_key_binary_safe(void) { + /* Key with embedded null: "ke\0y" (length 4) should produce a + * different slot than "ke" (length 2, what strlen would give). */ + unsigned int slot_full = valkeyClusterGetSlotByKey((char *)"ke\0y", 4); + unsigned int slot_truncated = valkeyClusterGetSlotByKey((char *)"ke", 2); + assert(slot_full != slot_truncated); +} + +void test_slot_by_key_empty(void) { + /* An empty string is a valid key. */ + unsigned int slot = valkeyClusterGetSlotByKey((char *)"", 0); + assert(slot == 0); +} + +int main(void) { + test_slot_by_key_basic(); + test_slot_by_key_with_hashtag(); + test_slot_by_key_binary_safe(); + test_slot_by_key_empty(); + return 0; +}