Fix profile search prefix matching#117
Conversation
ndb_search_profile() uses MDB_SET_RANGE to position the cursor at keys >= query, but ndb_search_profile_next() was iterating through ALL subsequent keys without checking if they still matched the query prefix. This caused searches like "jack" to return unrelated profiles starting with "j" (e.g., "jb55", "jen", etc.) because LMDB would continue past the query prefix into subsequent entries. Fix: - Add query[24] field to struct ndb_search to store the original query - Store lowercased query in ndb_search_profile() for later comparison - Validate first result matches prefix before returning - Check prefix match in ndb_search_profile_next() and stop when results no longer match Add test_profile_search_prefix() to verify all returned results match the query prefix, preventing regression. Fixes: damus-io/damus#3504 Closes damus-io#115 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded@alltheseas has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 9 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change adds prefix matching validation to profile searches in nostrdb. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
The old test expected ndb_search_profile_next() to return profiles that don't match the query prefix (broken behavior). Updated to verify that all results correctly match the "jean" prefix.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test.c (1)
882-912: Fix the old test to handle new prefix matching behavior.The pipeline failure at line 897 is caused by
test_profile_search()expecting the old buggy behavior wherendb_search_profile_next()would continue iterating past the "jean" prefix (e.g., returning profiles starting with "jb", "je"). With the prefix matching fix,ndb_search_profile_next()correctly returnsfalsewhen the next result doesn't match the "jean" prefix, causing the assertion to fail.The test needs to be updated to either:
- Remove the assertion at line 897 and check the return value conditionally
- Use a different search query that's guaranteed to have multiple matching results in the test data
- Remove this test entirely since
test_profile_search_prefix()now provides comprehensive coverage🔎 Suggested fix: Make the second result check conditional
assert(!strncmp(name, "jean", 4)); - assert(ndb_search_profile_next(&search)); - //print_search(&txn, &search); - profile = lookup_profile(&txn, search.profile_key); - name = NdbProfile_name_get(profile); - //assert(strncmp(name, "jean", 4)); - - for (i = 0; i < 3; i++) { - ndb_search_profile_next(&search); - //print_search(&txn, &search); - } - - //assert(!strcmp(name, "jb55")); + // With prefix matching, we may not have additional results starting with "jean" + if (ndb_search_profile_next(&search)) { + profile = lookup_profile(&txn, search.profile_key); + name = NdbProfile_name_get(profile); + // Verify any additional results also match the prefix + assert(!strncasecmp(name, "jean", 4)); + } ndb_search_profile_end(&search); ndb_end_query(&txn);
🧹 Nitpick comments (3)
src/nostrdb.h (1)
179-184: Newquery[24]field is consistent with index key size; clarify semanticsThe added buffer size matches
struct ndb_search_key::search[24], which is good for keeping prefix logic consistent. Two small refinements you might consider:
- Make the comment explicit that this holds a lowercased, truncated query used for prefix checks, not the raw user string.
- Optionally tie the two sizes together (e.g., a shared
#define NDB_PROFILE_SEARCH_STR_LEN 24or aSTATIC_ASSERT(sizeof(((struct ndb_search_key *)0)->search) == sizeof(((struct ndb_search *)0)->query), search_query_size_mismatch)) to avoid silent divergence if one changes later.src/nostrdb.c (2)
3623-3668: Prefix validation inndb_search_profilelooks correct; document truncation/empty‑query behaviorThe new logic to:
- store a lowercased query into
search->queryand- immediately reject the first hit when
search->key->searchno longer matches that prefixcorrectly fixes the “SET_RANGE then walk unrelated keys” bug and aligns with how
ndb_search_key.searchis constructed.A couple of behavioral details worth making explicit in comments or docs:
- Queries are effectively limited to
sizeof(search->query) - 1bytes; longer inputs are truncated before prefix comparison, but the index keys are truncated in the same way, so the behavior is consistent.- An empty query (
query_len == 0) still returns all profiles becausestrncmp(..., 0)is always 0. That matches prior behavior; if you ever want empty queries to return no results instead, this is the place to special‑case it.Optionally, you could also zero
search->key/search->profile_keyin thecleanuppath to avoid leaving stale data around after a failed search, though callers should already be checking the return code.
3679-3712:ndb_search_profile_nextcorrectly enforces the prefix boundary; consider small robustness tweaksThe added prefix check:
- computes
query_len = strlen(search->query)and- returns 0 as soon as
search->key->searchno longer starts with that prefix,ensures iteration stops at the end of the matching range, which is what you want on top of
MDB_SET_RANGE. Duplicate‑pubkey skipping is still preserved viainit_idandretry.A few low‑impact improvements you might consider:
- Cache the query length in
struct ndb_search(e.g.,size_t query_len;) when starting the search, and reuse it here instead of recomputingstrlenon each_next. This also makes behavior independent of any accidental mutation ofsearch->query.- For defensive programming, you could early‑return 0 if
search->cursor == NULLin_nextto guard against misuse (calling_nextafter an initial 0 result), rather than relying on callers to follow the contract.These don’t affect correctness of the prefix‑matching fix, which looks solid.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/nostrdb.csrc/nostrdb.htest.c
🧰 Additional context used
🪛 GitHub Actions: C/C++ CI
test.c
[error] 897-897: make check failed. test_profile_search assertion failed in test.c:897. Assertion `ndb_search_profile_next(&search)' failed.
🔇 Additional comments (4)
test.c (4)
21-21: LGTM!The
ctype.hinclude is necessary for thetolower()function used in the new prefix matching test.
914-964: Well-structured regression test for prefix matching.The test correctly validates that:
- The first result matches the query prefix
- All subsequent results match the prefix (case-insensitive)
- The search terminates when results no longer match the prefix
The safety limit at line 955 is a good defensive measure for test environments.
1050-1050: LGTM!Appropriately adds the new prefix matching test to the profile loading test suite.
1095-1095: LGTM!Appropriately adds the new prefix matching test to the migration test suite.
2 participants
Summary
ndb_search_profile()andndb_search_profile_next()to properly validate query prefix matchingquery[24]field tostruct ndb_searchto store the original search querytest_profile_search_prefix()to verify all results match the query prefixProblem
ndb_search_profile()uses LMDB'sMDB_SET_RANGEto position the cursor at keys >= query, butndb_search_profile_next()was iterating through ALL subsequent keys without checking if they still matched the query prefix.This caused searches like "jack" to return unrelated profiles starting with "j" (e.g., "jb55", "jen", etc.) because LMDB would continue past the query prefix into subsequent entries.
Test plan
test_profile_search_prefix()to verify all returned results match the query prefixCloses #115
Fixes: damus-io/damus#3504
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.