From 9e2177761a572ad172a1090c173efa35d27ecf58 Mon Sep 17 00:00:00 2001 From: Shray Arora Date: Tue, 22 Sep 2026 01:55:33 -0700 Subject: [PATCH 1/4] chain/storage: fix gaps in composite-key storage Four gaps in the composite-key storage layer: - LevelDB GetRange returned index entries as if they were user data. GetAllItems and GetKeyRange already skipped them; GetRange was missed. - DuckDB's composite-key methods returned 0 (success) while doing nothing. They now return -1. - EncodeCompositeKeyPrefix with no attributes added an extra \0, so a query for a whole index matched nothing. - DecodeCompositeKey didn't check for the \0 after "ck", so "ckXidx..." decoded as a valid entry. Each fix has a test. Nothing changes for keys that don't start with "ck\0". --- chain/storage/composite_key_codec.cpp | 10 +++++++--- chain/storage/composite_key_codec_test.cpp | 16 ++++++++++++++++ chain/storage/duckdb.h | 11 +++++------ chain/storage/kv_storage_test.cpp | 14 ++++++++++++++ chain/storage/leveldb.cpp | 8 ++++++++ 5 files changed, 50 insertions(+), 9 deletions(-) diff --git a/chain/storage/composite_key_codec.cpp b/chain/storage/composite_key_codec.cpp index 781fa9954..3df108941 100644 --- a/chain/storage/composite_key_codec.cpp +++ b/chain/storage/composite_key_codec.cpp @@ -75,8 +75,11 @@ std::string EncodeCompositeKeyPrefix( result.append(attribute_prefix[i]); } - // Trailing delimiter makes this a strict byte prefix of the full key. - result.push_back(kCompositeKeyDelim); + // End on a delimiter so "Davis" can't match "Davisville". With no + // attributes, the delimiter after index_name already does this. + if (!attribute_prefix.empty()) { + result.push_back(kCompositeKeyDelim); + } return result; } @@ -90,7 +93,8 @@ bool DecodeCompositeKey(const std::string& encoded, size_t ns_len = std::strlen(kCompositeKeyNamespace); if (encoded.size() < ns_len + 1 || - encoded.compare(0, ns_len, kCompositeKeyNamespace) != 0) { + encoded.compare(0, ns_len, kCompositeKeyNamespace) != 0 || + encoded[ns_len] != kCompositeKeyDelim) { return false; } diff --git a/chain/storage/composite_key_codec_test.cpp b/chain/storage/composite_key_codec_test.cpp index 30509f5f9..306dc2144 100644 --- a/chain/storage/composite_key_codec_test.cpp +++ b/chain/storage/composite_key_codec_test.cpp @@ -82,3 +82,19 @@ TEST(CompositeKeyCodecTest, EmptyAttributes) { EXPECT_EQ(attrs.size(), 0u); EXPECT_EQ(pk, "user:1"); } + +TEST(CompositeKeyCodecTest, PrefixWithNoAttributesMatchesWholeIndex) { + std::string prefix = EncodeCompositeKeyPrefix("byCity", {}); + EXPECT_EQ(EncodeCompositeKey("byCity", {"Davis"}, "user:1").find(prefix), 0u); + EXPECT_EQ(EncodeCompositeKey("byCity", {}, "user:2").find(prefix), 0u); + // Must not reach into another index whose name starts the same way. + EXPECT_NE(EncodeCompositeKey("byCity2", {"Davis"}, "user:3").find(prefix), + 0u); +} + +TEST(CompositeKeyCodecTest, DecodeRejectsMissingDelimiterAfterNamespace) { + std::string idx, pk; + std::vector attrs; + std::string bad = std::string("ckX") + "idx" + kCompositeKeyDelim + "pk"; + EXPECT_FALSE(DecodeCompositeKey(bad, &idx, &attrs, &pk)); +} diff --git a/chain/storage/duckdb.h b/chain/storage/duckdb.h index 153c19c61..8573669c2 100644 --- a/chain/storage/duckdb.h +++ b/chain/storage/duckdb.h @@ -101,12 +101,11 @@ class DuckDB : public Storage { bool Flush(bool should_sync = false) override { return true; } - // No-op overrides so DuckDB stays concrete: Storage declares these - // composite-key methods pure virtual, and without them DuckDB would be - // abstract and fail to compile. The SQL backend doesn't use them. - int CreateCompositeKey(const std::string&) override { return 0; } + // Composite keys aren't supported on DuckDB. Return -1 (failure) so callers + // don't think an index entry was written. + int CreateCompositeKey(const std::string&) override { return -1; } - int DeleteCompositeKey(const std::string&) override { return 0; } + int DeleteCompositeKey(const std::string&) override { return -1; } std::vector GetByCompositeKeyPrefix( const std::string&) override { @@ -114,7 +113,7 @@ class DuckDB : public Storage { } int UpdateCompositeKey(const std::string&, const std::string&) override { - return 0; + return -1; } private: diff --git a/chain/storage/kv_storage_test.cpp b/chain/storage/kv_storage_test.cpp index b6719072c..f3ebbac87 100644 --- a/chain/storage/kv_storage_test.cpp +++ b/chain/storage/kv_storage_test.cpp @@ -418,6 +418,20 @@ TEST_P(KVStorageTest, GetAllItemsExcludesCompositeKeys) { EXPECT_EQ(davis.size(), 1u); } +// GetRange must not return index entries either. "ck\0..." sorts between +// "apple" and "dog", so an unfiltered scan would include it. +TEST_P(KVStorageTest, GetRangeExcludesCompositeKeys) { + EXPECT_EQ(storage->SetValue("apple", "1"), 0); + EXPECT_EQ(storage->SetValue("dog", "2"), 0); + EXPECT_EQ(storage->CreateCompositeKey( + EncodeCompositeKey("byCity", {"Davis"}, "apple")), + 0); + + // MemoryDB's GetRange is unordered, so accept either order. + std::string range = storage->GetRange("a", "z"); + EXPECT_TRUE(range == "[1,2]" || range == "[2,1]") << range; +} + INSTANTIATE_TEST_CASE_P(KVStorageTest, KVStorageTest, ::testing::Values(MEM, LEVELDB, LEVELDB_WITH_BLOCK_CACHE)); diff --git a/chain/storage/leveldb.cpp b/chain/storage/leveldb.cpp index 15b1a8511..71c253dfa 100644 --- a/chain/storage/leveldb.cpp +++ b/chain/storage/leveldb.cpp @@ -203,10 +203,18 @@ std::string ResLevelDB::GetValue(const std::string& key) { std::string ResLevelDB::GetRange(const std::string& min_key, const std::string& max_key) { std::string values = "["; + // Hide composite-key index entries from user-facing scans. + const std::string ck_prefix = + std::string(kCompositeKeyNamespace) + kCompositeKeyDelim; + leveldb::Iterator* it = db_->NewIterator(leveldb::ReadOptions()); bool first_iteration = true; for (it->Seek(min_key); it->Valid() && it->key().ToString() <= max_key; it->Next()) { + if (it->key().size() >= ck_prefix.size() && + memcmp(it->key().data(), ck_prefix.data(), ck_prefix.size()) == 0) { + continue; + } if (!first_iteration) values.append(","); first_iteration = false; values.append(it->value().ToString()); From 261d6a4a74d0a759ab921fd225376202dd5b804d Mon Sep 17 00:00:00 2001 From: Shray Arora Date: Tue, 22 Sep 2026 01:55:41 -0700 Subject: [PATCH 2/4] executor/kv: add secondary index commands for composite keys Adds CREATE_INDEX_ENTRY, DELETE_INDEX_ENTRY, UPDATE_INDEX_ENTRY and QUERY_BY_INDEX so clients can use the composite keys added in the storage layer. The client sends the parts (index name, attributes, primary key) and the executor builds the stored key, so a client can't write any key outside the "ck\0" range. SET and SET_WITH_VERSION now refuse keys in that range. - Create fails if the record doesn't exist. - Delete and update fail if the entry doesn't exist. - Query returns each matching primary key once, sorted. - ExecuteRequest and ExecuteData both call one shared helper. Tests: kv_executor_index_test runs 21 cases on MemoryDB and LevelDB, through both ExecuteData and ExecuteRequest. --- executor/kv/BUILD | 12 + executor/kv/kv_executor.cpp | 170 ++++++++++++ executor/kv/kv_executor.h | 29 +++ executor/kv/kv_executor_index_test.cpp | 348 +++++++++++++++++++++++++ proto/kv/kv.proto | 13 + 5 files changed, 572 insertions(+) create mode 100644 executor/kv/kv_executor_index_test.cpp diff --git a/executor/kv/BUILD b/executor/kv/BUILD index 89bb67c7f..5790e016f 100644 --- a/executor/kv/BUILD +++ b/executor/kv/BUILD @@ -26,6 +26,7 @@ cc_library( hdrs = ["kv_executor.h"], deps = [ "//chain/storage", + "//chain/storage:composite_key_codec", "//common:comm", "//executor/common:transaction_manager", "//executor/contract/executor:contract_executor", @@ -34,6 +35,17 @@ cc_library( ], ) +cc_test( + name = "kv_executor_index_test", + srcs = ["kv_executor_index_test.cpp"], + deps = [ + ":kv_executor", + "//chain/storage:leveldb", + "//chain/storage:memory_db", + "//common/test:test_main", + ], +) + cc_test( name = "kv_executor_test", srcs = ["kv_executor_test.cpp"], diff --git a/executor/kv/kv_executor.cpp b/executor/kv/kv_executor.cpp index 9751e6453..54b84a4f7 100644 --- a/executor/kv/kv_executor.cpp +++ b/executor/kv/kv_executor.cpp @@ -21,9 +21,23 @@ #include +#include + +#include "chain/storage/composite_key_codec.h" #include "executor/contract/executor/contract_executor.h" namespace resdb { +namespace { + +// Index entry keys start with "ck\0". SET must not write them, or a client +// could plant fake index entries. +bool IsCompositeKeyRange(const std::string& key) { + static const std::string kPrefix = std::string( + storage::kCompositeKeyNamespace) + storage::kCompositeKeyDelim; + return key.compare(0, kPrefix.size(), kPrefix) == 0; +} + +} // namespace KVExecutor::KVExecutor(std::unique_ptr storage) { storage_ = std::move(storage); @@ -75,6 +89,11 @@ std::unique_ptr KVExecutor::ExecuteRequest( std::string result = ExecuteSQL(kv_request.sql_query()); kv_response.set_sql_response(result); kv_response.set_value(result); // keep legacy field populated + } else if (kv_request.cmd() == KVRequest::CREATE_INDEX_ENTRY || + kv_request.cmd() == KVRequest::DELETE_INDEX_ENTRY || + kv_request.cmd() == KVRequest::UPDATE_INDEX_ENTRY || + kv_request.cmd() == KVRequest::QUERY_BY_INDEX) { + ExecuteIndexCommand(kv_request, &kv_response); } else if(!kv_request.smart_contract_request().empty()){ std::unique_ptr resp = contract_manager_->ExecuteData(kv_request.smart_contract_request()); @@ -129,6 +148,11 @@ std::unique_ptr KVExecutor::ExecuteData( std::string result = ExecuteSQL(kv_request.sql_query()); kv_response.set_sql_response(result); kv_response.set_value(result); // keep legacy field populated + } else if (kv_request.cmd() == KVRequest::CREATE_INDEX_ENTRY || + kv_request.cmd() == KVRequest::DELETE_INDEX_ENTRY || + kv_request.cmd() == KVRequest::UPDATE_INDEX_ENTRY || + kv_request.cmd() == KVRequest::QUERY_BY_INDEX) { + ExecuteIndexCommand(kv_request, &kv_response); } else if(!kv_request.smart_contract_request().empty()){ std::unique_ptr resp = contract_manager_->ExecuteData(kv_request.smart_contract_request()); @@ -145,6 +169,11 @@ std::unique_ptr KVExecutor::ExecuteData( } void KVExecutor::Set(const std::string& key, const std::string& value) { + if (IsCompositeKeyRange(key)) { + LOG(ERROR) << "SET rejected: keys starting with \"ck\\0\" are reserved " + "for secondary indexes"; + return; + } storage_->SetValueWithSeq(key, value, seq_); } @@ -162,6 +191,11 @@ std::string KVExecutor::GetRange(const std::string& min_key, void KVExecutor::SetWithVersion(const std::string& key, const std::string& value, int version) { + if (IsCompositeKeyRange(key)) { + LOG(ERROR) << "SET_WITH_VERSION rejected: keys starting with \"ck\\0\" " + "are reserved for secondary indexes"; + return; + } storage_->SetValueWithVersion(key, value, version); } @@ -220,5 +254,141 @@ std::string KVExecutor::ExecuteSQL(const std::string& sql_query) { return storage_->ExecuteSQL(sql_query); } +void KVExecutor::ExecuteIndexCommand(const KVRequest& request, + KVResponse* response) { + std::vector attributes(request.attributes().begin(), + request.attributes().end()); + switch (request.cmd()) { + case KVRequest::CREATE_INDEX_ENTRY: + response->set_success(CreateIndexEntry(request.index_name(), attributes, + request.primary_key())); + break; + case KVRequest::DELETE_INDEX_ENTRY: + response->set_success(DeleteIndexEntry(request.index_name(), attributes, + request.primary_key())); + break; + case KVRequest::UPDATE_INDEX_ENTRY: { + std::vector new_attributes(request.new_attributes().begin(), + request.new_attributes().end()); + response->set_success(UpdateIndexEntry(request.index_name(), attributes, + new_attributes, + request.primary_key())); + break; + } + case KVRequest::QUERY_BY_INDEX: + QueryByIndex(request.index_name(), attributes, response->mutable_items()); + break; + default: + break; + } +} + +// MemoryDB stores SET and SET_WITH_VERSION records separately, so check both. +bool KVExecutor::PrimaryKeyExists(const std::string& key) { + return !Get(key).empty() || + !storage_->GetValueWithVersion(key, 0).first.empty(); +} + +// Exact match: a prefix search for "user:1" also returns "user:10". +bool KVExecutor::IndexEntryExists(const std::string& composite_key) { + for (const std::string& key : + storage_->GetByCompositeKeyPrefix(composite_key)) { + if (key == composite_key) { + return true; + } + } + return false; +} + +bool KVExecutor::CreateIndexEntry(const std::string& index_name, + const std::vector& attributes, + const std::string& primary_key) { + if (index_name.empty() || primary_key.empty()) { + LOG(ERROR) << "CreateIndexEntry: index_name and primary_key are required"; + return false; + } + if (!PrimaryKeyExists(primary_key)) { + LOG(ERROR) << "CreateIndexEntry: primary key not found: " << primary_key; + return false; + } + std::string composite_key = + storage::EncodeCompositeKey(index_name, attributes, primary_key); + if (composite_key.empty()) { + LOG(ERROR) << "CreateIndexEntry: input contains a \\0 byte"; + return false; + } + return storage_->CreateCompositeKey(composite_key) == 0; +} + +bool KVExecutor::DeleteIndexEntry(const std::string& index_name, + const std::vector& attributes, + const std::string& primary_key) { + if (index_name.empty() || primary_key.empty()) { + LOG(ERROR) << "DeleteIndexEntry: index_name and primary_key are required"; + return false; + } + std::string composite_key = + storage::EncodeCompositeKey(index_name, attributes, primary_key); + if (composite_key.empty() || !IndexEntryExists(composite_key)) { + LOG(ERROR) << "DeleteIndexEntry: no such index entry"; + return false; + } + return storage_->DeleteCompositeKey(composite_key) == 0; +} + +bool KVExecutor::UpdateIndexEntry( + const std::string& index_name, + const std::vector& old_attributes, + const std::vector& new_attributes, + const std::string& primary_key) { + if (index_name.empty() || primary_key.empty()) { + LOG(ERROR) << "UpdateIndexEntry: index_name and primary_key are required"; + return false; + } + std::string old_key = + storage::EncodeCompositeKey(index_name, old_attributes, primary_key); + std::string new_key = + storage::EncodeCompositeKey(index_name, new_attributes, primary_key); + if (old_key.empty() || new_key.empty()) { + LOG(ERROR) << "UpdateIndexEntry: input contains a \\0 byte"; + return false; + } + if (!IndexEntryExists(old_key)) { + LOG(ERROR) << "UpdateIndexEntry: no existing entry to update"; + return false; + } + return storage_->UpdateCompositeKey(old_key, new_key) == 0; +} + +void KVExecutor::QueryByIndex(const std::string& index_name, + const std::vector& attribute_prefix, + Items* items) { + if (index_name.empty()) { + LOG(ERROR) << "QueryByIndex: index_name is required"; + return; + } + std::string prefix = + storage::EncodeCompositeKeyPrefix(index_name, attribute_prefix); + if (prefix.empty()) { + LOG(ERROR) << "QueryByIndex: input contains a \\0 byte"; + return; + } + // A record can have several matching entries (e.g. {"Davis", "active"} and + // {"Davis", "inactive"}), so collect into a set: each key once, sorted. + std::set primary_keys; + for (const std::string& composite_key : + storage_->GetByCompositeKeyPrefix(prefix)) { + std::string decoded_index; + std::vector decoded_attributes; + std::string primary_key; + if (storage::DecodeCompositeKey(composite_key, &decoded_index, + &decoded_attributes, &primary_key)) { + primary_keys.insert(primary_key); + } + } + for (const std::string& primary_key : primary_keys) { + items->add_item()->set_key(primary_key); + } +} } // namespace resdb diff --git a/executor/kv/kv_executor.h b/executor/kv/kv_executor.h index f0199b21f..b0b78089e 100644 --- a/executor/kv/kv_executor.h +++ b/executor/kv/kv_executor.h @@ -21,7 +21,9 @@ #include #include +#include #include +#include #include "chain/storage/storage.h" #include "executor/common/transaction_manager.h" @@ -58,7 +60,34 @@ class KVExecutor : public TransactionManager { void GetTopHistory(const std::string& key, int top_number, Items* items); std::string ExecuteSQL(const std::string& sql_query); + // Secondary indexes (issue #180). An index entry says which attributes a + // record has, e.g. ("by_city", {"Davis"}) -> "user:1", and is stored as the + // key "ck\0by_city\0Davis\0user:1". Only these functions build such keys. + + // Fails if the record (primary_key) doesn't exist. + bool CreateIndexEntry(const std::string& index_name, + const std::vector& attributes, + const std::string& primary_key); + // Fails if the entry doesn't exist. + bool DeleteIndexEntry(const std::string& index_name, + const std::vector& attributes, + const std::string& primary_key); + // Moves an entry to new attributes in one write. Fails if it doesn't exist. + bool UpdateIndexEntry(const std::string& index_name, + const std::vector& old_attributes, + const std::vector& new_attributes, + const std::string& primary_key); + // Adds the primary keys of matching entries to `items`, each once, sorted. + void QueryByIndex(const std::string& index_name, + const std::vector& attribute_prefix, + Items* items); + private: + // Shared by ExecuteRequest and ExecuteData. + void ExecuteIndexCommand(const KVRequest& request, KVResponse* response); + bool PrimaryKeyExists(const std::string& key); + bool IndexEntryExists(const std::string& composite_key); + std::unique_ptr contract_manager_; }; diff --git a/executor/kv/kv_executor_index_test.cpp b/executor/kv/kv_executor_index_test.cpp new file mode 100644 index 000000000..568fbe9db --- /dev/null +++ b/executor/kv/kv_executor_index_test.cpp @@ -0,0 +1,348 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include +#include + +#include +#include +#include +#include + +#include "chain/storage/leveldb.h" +#include "chain/storage/memory_db.h" +#include "executor/kv/kv_executor.h" +#include "proto/kv/kv.pb.h" + +namespace resdb { +namespace { + +using ::testing::Combine; +using ::testing::ElementsAre; +using ::testing::IsEmpty; +using ::testing::Values; + +enum Backend { MEMORY_DB, LEVEL_DB }; +// ExecuteData and ExecuteRequest each dispatch commands; test both. +enum EntryPoint { EXECUTE_DATA, EXECUTE_REQUEST }; + +class KVExecutorIndexTest + : public ::testing::TestWithParam> { + protected: + KVExecutorIndexTest() { + std::unique_ptr storage; + if (std::get<0>(GetParam()) == LEVEL_DB) { + db_path_ = ::testing::TempDir() + "kv_executor_index_test_db"; + std::filesystem::remove_all(db_path_); + storage = storage::NewResLevelDB(db_path_); + } else { + storage = std::make_unique(); + } + executor_ = std::make_unique(std::move(storage)); + } + + ~KVExecutorIndexTest() override { + executor_.reset(); // close LevelDB before deleting its folder + if (!db_path_.empty()) { + std::filesystem::remove_all(db_path_); + } + } + + KVResponse Execute(const KVRequest& request) { + std::unique_ptr raw; + if (std::get<1>(GetParam()) == EXECUTE_DATA) { + std::string data; + request.SerializeToString(&data); + raw = executor_->ExecuteData(data); + } else { + raw = executor_->ExecuteRequest(request); + } + KVResponse response; + if (raw != nullptr) { + response.ParseFromString(*raw); + } + return response; + } + + void Set(const std::string& key, const std::string& value) { + KVRequest request; + request.set_cmd(KVRequest::SET); + request.set_key(key); + request.set_value(value); + Execute(request); + } + + void SetWithVersion(const std::string& key, const std::string& value) { + KVRequest request; + request.set_cmd(KVRequest::SET_WITH_VERSION); + request.set_key(key); + request.set_value(value); + request.set_version(0); + Execute(request); + } + + std::string Get(const std::string& key) { + KVRequest request; + request.set_cmd(KVRequest::GET); + request.set_key(key); + return Execute(request).value(); + } + + bool IndexCommand(KVRequest::CMD cmd, const std::string& index, + const std::vector& attributes, + const std::string& primary_key, + const std::vector& new_attributes = {}) { + KVRequest request; + request.set_cmd(cmd); + request.set_index_name(index); + for (const auto& a : attributes) request.add_attributes(a); + for (const auto& a : new_attributes) request.add_new_attributes(a); + request.set_primary_key(primary_key); + return Execute(request).success(); + } + + bool Create(const std::string& index, const std::vector& attrs, + const std::string& pk) { + return IndexCommand(KVRequest::CREATE_INDEX_ENTRY, index, attrs, pk); + } + + bool Delete(const std::string& index, const std::vector& attrs, + const std::string& pk) { + return IndexCommand(KVRequest::DELETE_INDEX_ENTRY, index, attrs, pk); + } + + bool Update(const std::string& index, const std::vector& from, + const std::vector& to, const std::string& pk) { + return IndexCommand(KVRequest::UPDATE_INDEX_ENTRY, index, from, pk, to); + } + + std::vector Query(const std::string& index, + const std::vector& attrs) { + KVRequest request; + request.set_cmd(KVRequest::QUERY_BY_INDEX); + request.set_index_name(index); + for (const auto& a : attrs) request.add_attributes(a); + // Keep the response in a variable: looping over Execute(...).items() + // directly would read from a temporary that's already been destroyed. + KVResponse response = Execute(request); + std::vector keys; + for (const Item& item : response.items().item()) { + keys.push_back(item.key()); + } + return keys; + } + + std::string db_path_; + std::unique_ptr executor_; +}; + +// A string holding a real zero byte (a plain "a\0b" literal stops at the \0). +std::string WithNul(const std::string& a, const std::string& b) { + return a + std::string(1, '\0') + b; +} + +TEST_P(KVExecutorIndexTest, CreateThenQuery) { + Set("user:1", "Ana"); + Set("user:2", "Bo"); + Set("user:3", "Cy"); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:1")); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:2")); + EXPECT_TRUE(Create("by_city", {"Sacramento"}, "user:3")); + + EXPECT_THAT(Query("by_city", {"Davis"}), ElementsAre("user:1", "user:2")); + EXPECT_THAT(Query("by_city", {"Sacramento"}), ElementsAre("user:3")); + EXPECT_THAT(Query("by_city", {"Chico"}), IsEmpty()); +} + +TEST_P(KVExecutorIndexTest, CreateFailsWhenRecordDoesNotExist) { + EXPECT_FALSE(Create("by_city", {"Davis"}, "user:99")); + EXPECT_THAT(Query("by_city", {"Davis"}), IsEmpty()); +} + +// MemoryDB keeps SET_WITH_VERSION records in a separate map from SET records. +TEST_P(KVExecutorIndexTest, CreateWorksForRecordWrittenWithVersion) { + SetWithVersion("user:5", "Eve"); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:5")); + EXPECT_THAT(Query("by_city", {"Davis"}), ElementsAre("user:5")); +} + +TEST_P(KVExecutorIndexTest, CreateRejectsBadInput) { + Set("user:1", "Ana"); + EXPECT_FALSE(Create("", {"Davis"}, "user:1")); + EXPECT_FALSE(Create("by_city", {"Davis"}, "")); + EXPECT_FALSE(Create("by_city", {WithNul("Da", "vis")}, "user:1")); + EXPECT_FALSE(Create(WithNul("by", "city"), {"Davis"}, "user:1")); + EXPECT_THAT(Query("by_city", {"Davis"}), IsEmpty()); +} + +TEST_P(KVExecutorIndexTest, CreatingTwiceKeepsOneEntry) { + Set("user:1", "Ana"); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:1")); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:1")); + EXPECT_THAT(Query("by_city", {"Davis"}), ElementsAre("user:1")); +} + +// "Davis" must not match "Davisville". +TEST_P(KVExecutorIndexTest, QueryMatchesWholeAttribute) { + Set("user:1", "Ana"); + Set("user:2", "Bo"); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:1")); + EXPECT_TRUE(Create("by_city", {"Davisville"}, "user:2")); + EXPECT_THAT(Query("by_city", {"Davis"}), ElementsAre("user:1")); +} + +// Same attribute, different index: queries must not mix them. +TEST_P(KVExecutorIndexTest, QueryOnlyReadsItsOwnIndex) { + Set("user:1", "Ana"); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:1")); + EXPECT_THAT(Query("by_birthplace", {"Davis"}), IsEmpty()); + EXPECT_THAT(Query("by_cit", {"Davis"}), IsEmpty()); +} + +TEST_P(KVExecutorIndexTest, QueryWithLeadingAttributes) { + Set("user:1", "Ana"); + Set("user:2", "Bo"); + EXPECT_TRUE(Create("by_city_status", {"Davis", "active"}, "user:1")); + EXPECT_TRUE(Create("by_city_status", {"Davis", "inactive"}, "user:2")); + EXPECT_THAT(Query("by_city_status", {"Davis"}), + ElementsAre("user:1", "user:2")); + EXPECT_THAT(Query("by_city_status", {"Davis", "active"}), + ElementsAre("user:1")); +} + +// No attributes = every entry in the index. +TEST_P(KVExecutorIndexTest, QueryWithNoAttributesReturnsWholeIndex) { + Set("user:1", "Ana"); + Set("user:2", "Bo"); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:1")); + EXPECT_TRUE(Create("by_city", {"Sacramento"}, "user:2")); + EXPECT_THAT(Query("by_city", {}), ElementsAre("user:1", "user:2")); +} + +TEST_P(KVExecutorIndexTest, QueryRejectsBadInput) { + Set("user:1", "Ana"); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:1")); + EXPECT_THAT(Query("", {"Davis"}), IsEmpty()); + EXPECT_THAT(Query("by_city", {WithNul("Da", "vis")}), IsEmpty()); +} + +TEST_P(KVExecutorIndexTest, DeleteRemovesOnlyThatEntry) { + Set("user:1", "Ana"); + Set("user:10", "Jo"); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:1")); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:10")); + + EXPECT_TRUE(Delete("by_city", {"Davis"}, "user:1")); + EXPECT_THAT(Query("by_city", {"Davis"}), ElementsAre("user:10")); + // user:1's entry is gone; "user:10" must not count as a match for it. + EXPECT_FALSE(Delete("by_city", {"Davis"}, "user:1")); +} + +TEST_P(KVExecutorIndexTest, DeleteFailsWhenEntryDoesNotExist) { + Set("user:1", "Ana"); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:1")); + EXPECT_FALSE(Delete("by_city", {"Sacramento"}, "user:1")); + EXPECT_THAT(Query("by_city", {"Davis"}), ElementsAre("user:1")); +} + +TEST_P(KVExecutorIndexTest, UpdateMovesEntry) { + Set("user:1", "Ana"); + Set("user:2", "Bo"); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:1")); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:2")); + + EXPECT_TRUE(Update("by_city", {"Davis"}, {"Sacramento"}, "user:2")); + EXPECT_THAT(Query("by_city", {"Davis"}), ElementsAre("user:1")); + EXPECT_THAT(Query("by_city", {"Sacramento"}), ElementsAre("user:2")); +} + +// A wrong "from" must change nothing, and must not create the new entry. +TEST_P(KVExecutorIndexTest, UpdateFailsWhenEntryDoesNotExist) { + Set("user:1", "Ana"); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:1")); + EXPECT_FALSE(Update("by_city", {"Chico"}, {"Sacramento"}, "user:1")); + EXPECT_THAT(Query("by_city", {"Davis"}), ElementsAre("user:1")); + EXPECT_THAT(Query("by_city", {"Sacramento"}), IsEmpty()); +} + +TEST_P(KVExecutorIndexTest, UpdateRejectsBadInput) { + Set("user:1", "Ana"); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:1")); + EXPECT_FALSE(Update("by_city", {"Davis"}, {WithNul("Sac", "ramento")}, + "user:1")); + EXPECT_THAT(Query("by_city", {"Davis"}), ElementsAre("user:1")); +} + +// SET and SET_WITH_VERSION must not be able to plant a fake index entry. +TEST_P(KVExecutorIndexTest, SetCannotWriteIndexEntries) { + const std::string forged = std::string("ck") + '\0' + "by_city" + '\0' + + "Davis" + '\0' + "user:666"; + Set(forged, "x"); + SetWithVersion(forged, "x"); + EXPECT_THAT(Query("by_city", {"Davis"}), IsEmpty()); + EXPECT_EQ(Get(forged), ""); +} + +// Keys that merely start with "ck" (no \0) are ordinary user keys. +TEST_P(KVExecutorIndexTest, SetStillAllowsKeysStartingWithCk) { + Set("ck_balance", "100"); + EXPECT_EQ(Get("ck_balance"), "100"); +} + +// user:1 has two matching entries; it must come back once. +TEST_P(KVExecutorIndexTest, QueryReturnsEachRecordOnce) { + Set("user:1", "Ana"); + EXPECT_TRUE(Create("by_city_status", {"Davis", "active"}, "user:1")); + EXPECT_TRUE(Create("by_city_status", {"Davis", "inactive"}, "user:1")); + EXPECT_THAT(Query("by_city_status", {"Davis"}), ElementsAre("user:1")); +} + +// Stored order is by attributes ("active" < "inactive"), so user:2 comes +// first internally; results must still be sorted by primary key. +TEST_P(KVExecutorIndexTest, QueryResultsAreSortedByPrimaryKey) { + Set("user:1", "Ana"); + Set("user:2", "Bo"); + EXPECT_TRUE(Create("by_city_status", {"Davis", "inactive"}, "user:1")); + EXPECT_TRUE(Create("by_city_status", {"Davis", "active"}, "user:2")); + EXPECT_THAT(Query("by_city_status", {"Davis"}), + ElementsAre("user:1", "user:2")); +} + +TEST_P(KVExecutorIndexTest, UpdateToSameAttributesKeepsEntry) { + Set("user:1", "Ana"); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:1")); + EXPECT_TRUE(Update("by_city", {"Davis"}, {"Davis"}, "user:1")); + EXPECT_THAT(Query("by_city", {"Davis"}), ElementsAre("user:1")); +} + +TEST_P(KVExecutorIndexTest, UpdateOntoExistingEntry) { + Set("user:1", "Ana"); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:1")); + EXPECT_TRUE(Create("by_city", {"Sacramento"}, "user:1")); + EXPECT_TRUE(Update("by_city", {"Davis"}, {"Sacramento"}, "user:1")); + EXPECT_THAT(Query("by_city", {"Davis"}), IsEmpty()); + EXPECT_THAT(Query("by_city", {"Sacramento"}), ElementsAre("user:1")); +} + +INSTANTIATE_TEST_SUITE_P(AllBackendsAndEntryPoints, KVExecutorIndexTest, + Combine(Values(MEMORY_DB, LEVEL_DB), + Values(EXECUTE_DATA, EXECUTE_REQUEST))); + +} // namespace +} // namespace resdb diff --git a/proto/kv/kv.proto b/proto/kv/kv.proto index 613403048..b4afc8559 100644 --- a/proto/kv/kv.proto +++ b/proto/kv/kv.proto @@ -36,6 +36,11 @@ message KVRequest { GET_TOP = 10; // NEW: SQL command type SQL = 11; + // Secondary index (composite key) commands, see issue #180. + CREATE_INDEX_ENTRY = 12; + DELETE_INDEX_ENTRY = 13; + UPDATE_INDEX_ENTRY = 14; + QUERY_BY_INDEX = 15; } CMD cmd = 1; string key = 2; @@ -52,6 +57,12 @@ message KVRequest { bytes smart_contract_request = 10; // NEW: raw SQL text for SQL commands (cmd == SQL) string sql_query = 11; + // NEW: For index commands. The server builds the stored key from these. + string index_name = 12; + repeated string attributes = 13; + string primary_key = 14; + // For UPDATE_INDEX_ENTRY: the attributes to move the entry to. + repeated string new_attributes = 15; } message ValueInfo { @@ -76,4 +87,6 @@ message KVResponse { bytes smart_contract_response = 10; // NEW: response payload for SQL commands string sql_response = 11; + // Whether CREATE/DELETE/UPDATE_INDEX_ENTRY made the change. + bool success = 12; } From 0004d3a8925c86c1e0424b0f1d8acd588f535c3f Mon Sep 17 00:00:00 2001 From: Shray Arora Date: Tue, 22 Sep 2026 13:45:12 -0700 Subject: [PATCH 3/4] executor/kv: test indexing records with binary and empty values Values are opaque bytes, so a record can hold an image or any other binary data. Adds a test that stores PNG-like bytes (including zero bytes), indexes the record and queries it back. Also documents the one limit: a record whose value is empty can't be indexed, because it looks the same as a missing record. --- executor/kv/kv_executor_index_test.cpp | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/executor/kv/kv_executor_index_test.cpp b/executor/kv/kv_executor_index_test.cpp index 568fbe9db..3ae59558f 100644 --- a/executor/kv/kv_executor_index_test.cpp +++ b/executor/kv/kv_executor_index_test.cpp @@ -289,6 +289,24 @@ TEST_P(KVExecutorIndexTest, UpdateRejectsBadInput) { EXPECT_THAT(Query("by_city", {"Davis"}), ElementsAre("user:1")); } +// Values are opaque bytes, so a record holding an image indexes like any other. +TEST_P(KVExecutorIndexTest, IndexRecordWithBinaryValue) { + std::string image("\x89PNG\r\n\x1a\n", 8); // PNG signature + image += std::string(3, '\0'); // zero bytes + image += std::string("\xff\xfe\xfd", 3); // not valid text + + Set("img:1", image); + EXPECT_EQ(Get("img:1"), image); + EXPECT_TRUE(Create("by_type", {"png"}, "img:1")); + EXPECT_THAT(Query("by_type", {"png"}), ElementsAre("img:1")); +} + +// A record with an empty value can't be told apart from a missing one. +TEST_P(KVExecutorIndexTest, CannotIndexRecordWithEmptyValue) { + Set("user:1", ""); + EXPECT_FALSE(Create("by_city", {"Davis"}, "user:1")); +} + // SET and SET_WITH_VERSION must not be able to plant a fake index entry. TEST_P(KVExecutorIndexTest, SetCannotWriteIndexEntries) { const std::string forged = std::string("ck") + '\0' + "by_city" + '\0' + From f44f7be5ba0773a340024477c2a8b6a195e15eea Mon Sep 17 00:00:00 2001 From: Shray Arora Date: Thu, 24 Sep 2026 14:42:39 -0700 Subject: [PATCH 4/4] executor/kv: return record values with query_by_index A query returns the primary keys of the matching records, so an application that wants the records has to fetch them one by one, and each of those is its own request through consensus. With with_values set, the executor reads the matching records itself and returns them in the same reply, so a query costs one request whatever the match count. Off by default, so a query still returns keys only unless it is asked for. --- executor/kv/kv_executor.cpp | 16 ++++++-- executor/kv/kv_executor.h | 3 +- executor/kv/kv_executor_index_test.cpp | 55 ++++++++++++++++++++++++++ proto/kv/kv.proto | 3 ++ 4 files changed, 73 insertions(+), 4 deletions(-) diff --git a/executor/kv/kv_executor.cpp b/executor/kv/kv_executor.cpp index 54b84a4f7..19f53600e 100644 --- a/executor/kv/kv_executor.cpp +++ b/executor/kv/kv_executor.cpp @@ -276,7 +276,8 @@ void KVExecutor::ExecuteIndexCommand(const KVRequest& request, break; } case KVRequest::QUERY_BY_INDEX: - QueryByIndex(request.index_name(), attributes, response->mutable_items()); + QueryByIndex(request.index_name(), attributes, request.with_values(), + response->mutable_items()); break; default: break; @@ -362,7 +363,7 @@ bool KVExecutor::UpdateIndexEntry( void KVExecutor::QueryByIndex(const std::string& index_name, const std::vector& attribute_prefix, - Items* items) { + bool with_values, Items* items) { if (index_name.empty()) { LOG(ERROR) << "QueryByIndex: index_name is required"; return; @@ -387,7 +388,16 @@ void KVExecutor::QueryByIndex(const std::string& index_name, } } for (const std::string& primary_key : primary_keys) { - items->add_item()->set_key(primary_key); + Item* item = items->add_item(); + item->set_key(primary_key); + if (with_values) { + // Read here, so the caller doesn't send one request per match. + std::string value = Get(primary_key); + if (value.empty()) { + value = storage_->GetValueWithVersion(primary_key, 0).first; + } + item->mutable_value_info()->set_value(value); + } } } diff --git a/executor/kv/kv_executor.h b/executor/kv/kv_executor.h index b0b78089e..93e70ed69 100644 --- a/executor/kv/kv_executor.h +++ b/executor/kv/kv_executor.h @@ -78,9 +78,10 @@ class KVExecutor : public TransactionManager { const std::vector& new_attributes, const std::string& primary_key); // Adds the primary keys of matching entries to `items`, each once, sorted. + // With `with_values`, each record's value is read and returned too. void QueryByIndex(const std::string& index_name, const std::vector& attribute_prefix, - Items* items); + bool with_values, Items* items); private: // Shared by ExecuteRequest and ExecuteData. diff --git a/executor/kv/kv_executor_index_test.cpp b/executor/kv/kv_executor_index_test.cpp index 3ae59558f..88fd7cff1 100644 --- a/executor/kv/kv_executor_index_test.cpp +++ b/executor/kv/kv_executor_index_test.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include "chain/storage/leveldb.h" @@ -132,6 +133,21 @@ class KVExecutorIndexTest return IndexCommand(KVRequest::UPDATE_INDEX_ENTRY, index, from, pk, to); } + std::vector> QueryWithValues( + const std::string& index, const std::vector& attrs) { + KVRequest request; + request.set_cmd(KVRequest::QUERY_BY_INDEX); + request.set_index_name(index); + for (const auto& a : attrs) request.add_attributes(a); + request.set_with_values(true); + KVResponse response = Execute(request); + std::vector> records; + for (const Item& item : response.items().item()) { + records.push_back({item.key(), item.value_info().value()}); + } + return records; + } + std::vector Query(const std::string& index, const std::vector& attrs) { KVRequest request; @@ -358,6 +374,45 @@ TEST_P(KVExecutorIndexTest, UpdateOntoExistingEntry) { EXPECT_THAT(Query("by_city", {"Sacramento"}), ElementsAre("user:1")); } +TEST_P(KVExecutorIndexTest, QueryWithValuesReturnsRecords) { + Set("user:1", "Ana"); + Set("user:2", "Bo"); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:1")); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:2")); + + EXPECT_THAT(QueryWithValues("by_city", {"Davis"}), + ElementsAre(std::make_pair("user:1", "Ana"), + std::make_pair("user:2", "Bo"))); +} + +// Without the flag the values must stay out of the reply. +TEST_P(KVExecutorIndexTest, QueryWithoutValuesReturnsKeysOnly) { + Set("user:1", "Ana"); + EXPECT_TRUE(Create("by_city", {"Davis"}, "user:1")); + + KVRequest request; + request.set_cmd(KVRequest::QUERY_BY_INDEX); + request.set_index_name("by_city"); + request.add_attributes("Davis"); + KVResponse response = Execute(request); + ASSERT_EQ(response.items().item_size(), 1); + EXPECT_EQ(response.items().item(0).key(), "user:1"); + EXPECT_EQ(response.items().item(0).value_info().value(), ""); +} + +TEST_P(KVExecutorIndexTest, QueryWithValuesHandlesBinaryAndVersionedRecords) { + std::string image("\x89PNG\r\n\x1a\n", 8); + image += std::string(3, '\0'); + Set("img:1", image); + SetWithVersion("user:5", "Eve"); + EXPECT_TRUE(Create("by_type", {"png"}, "img:1")); + EXPECT_TRUE(Create("by_type", {"png"}, "user:5")); + + EXPECT_THAT(QueryWithValues("by_type", {"png"}), + ElementsAre(std::make_pair("img:1", image), + std::make_pair("user:5", "Eve"))); +} + INSTANTIATE_TEST_SUITE_P(AllBackendsAndEntryPoints, KVExecutorIndexTest, Combine(Values(MEMORY_DB, LEVEL_DB), Values(EXECUTE_DATA, EXECUTE_REQUEST))); diff --git a/proto/kv/kv.proto b/proto/kv/kv.proto index b4afc8559..e1a4f2d81 100644 --- a/proto/kv/kv.proto +++ b/proto/kv/kv.proto @@ -63,6 +63,9 @@ message KVRequest { string primary_key = 14; // For UPDATE_INDEX_ENTRY: the attributes to move the entry to. repeated string new_attributes = 15; + // For QUERY_BY_INDEX: also return each record's value, saving the caller + // one request per match. + bool with_values = 16; } message ValueInfo {