fix: guard against falsy-value traps in psycopg2, chromadb, weaviate mocks#780
fix: guard against falsy-value traps in psycopg2, chromadb, weaviate mocks#780koushik1359 wants to merge 1 commit intorocketride-org:developfrom
Conversation
…mocks Three test mocks mishandled falsy values, causing mock tests to pass while real-service tests would fail: psycopg2: bool is a subclass of int in Python, so isinstance(False, int) returns True. SQL WHERE params like isDeleted=False were incorrectly matched as LIMIT values. Fixed by adding not isinstance(p, bool) guards in _handle_semantic_search and _extract_limit. chromadb: 'if limit and ...' treated limit=0 as falsy, returning all rows instead of zero rows. Fixed with explicit 'limit is not None' check. weaviate: sort key 'distance if distance else 1.0' collapsed 0.0 (exact match, best possible distance) to 1.0, ranking perfect matches last. Fixed with 'distance if distance is not None else 1.0'. Resolves rocketride-org#748
|
No description provided. |
📝 WalkthroughWalkthroughThis PR corrects three Python truthiness and ChangesTest Mock Truthiness and isinstance Fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nodes/test/mocks/psycopg2/__init__.py (1)
155-158:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame falsy-value trap remains in
_handle_select_query—if limit:should beif limit is not None:.
_extract_limitreturnsOptional[int], so a non-boolean integer0in the params list would be returned as a valid limit but then silently skipped here becauseif limit:isif 0:→False. This is the identical class of bug being fixed across the other three mocks in this PR, and it lives in the same file.🐛 Proposed fix
- if limit: + if limit is not None: results = results[:limit]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/test/mocks/psycopg2/__init__.py` around lines 155 - 158, In _handle_select_query, the LIMIT check uses a falsy test that skips valid 0 limits; change the conditional that currently reads "if limit:" to explicitly check for None (i.e., "if limit is not None:") so a returned integer 0 from _extract_limit is applied; locate the _handle_select_query function and the _extract_limit helper and update the limit application accordingly.nodes/test/mocks/chromadb/__init__.py (1)
174-186:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
limit=0still yields 1 result — check must precede the append.The item is appended at line 174 before the guard at line 185. When
limit=0, the check evaluates0 is not None and len(results['ids']) >= 0→Trueon the very first iteration — but only after the first item has already been added. The PR's stated goal of "return 0 rows forlimit=0" is not achieved.🐛 Proposed fix — move the limit guard before the append
+ # Apply limit (guard before append so limit=0 correctly returns empty) + if limit is not None and len(results['ids']) >= limit: + break + results['ids'].append(id) if 'metadatas' in include: results['metadatas'].append(self._serialize_metadata(metadata)) @@ -182,9 +182,6 @@ if 'embeddings' in include: results['embeddings'].append(data.get('embedding')) count += 1 - # Apply limit - if limit is not None and len(results['ids']) >= limit: - break🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/test/mocks/chromadb/__init__.py` around lines 174 - 186, The loop appends an item to results['ids'] (and optionally metadatas/documents/embeddings) before checking the limit, so limit=0 still returns one row; move the limit guard that checks "if limit is not None and len(results['ids']) >= limit" to the top of the iteration (before appending) so no items are added when the current length already meets the limit; update the block around results['ids'].append(id) / results['metadatas'].append(self._serialize_metadata(metadata)) / results['documents'].append(document) / results['embeddings'].append(data.get('embedding')) to perform the limit check first and then append, keeping the existing count increment and break behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@nodes/test/mocks/chromadb/__init__.py`:
- Around line 174-186: The loop appends an item to results['ids'] (and
optionally metadatas/documents/embeddings) before checking the limit, so limit=0
still returns one row; move the limit guard that checks "if limit is not None
and len(results['ids']) >= limit" to the top of the iteration (before appending)
so no items are added when the current length already meets the limit; update
the block around results['ids'].append(id) /
results['metadatas'].append(self._serialize_metadata(metadata)) /
results['documents'].append(document) /
results['embeddings'].append(data.get('embedding')) to perform the limit check
first and then append, keeping the existing count increment and break behavior.
In `@nodes/test/mocks/psycopg2/__init__.py`:
- Around line 155-158: In _handle_select_query, the LIMIT check uses a falsy
test that skips valid 0 limits; change the conditional that currently reads "if
limit:" to explicitly check for None (i.e., "if limit is not None:") so a
returned integer 0 from _extract_limit is applied; locate the
_handle_select_query function and the _extract_limit helper and update the limit
application accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e1eabc41-818e-4c12-a216-cdd1c555cc5f
📒 Files selected for processing (3)
nodes/test/mocks/chromadb/__init__.pynodes/test/mocks/psycopg2/__init__.pynodes/test/mocks/weaviate/__init__.py
Problem
Three test mocks mishandled falsy values (0, False, 0.0), causing tests
to pass against mocks while failing against real services in production.
psycopg2 —
boolsubclassesintin Python, soisinstance(False, int)returns
True. SQL WHERE params likeisDeleted=Falsewere being matchedas LIMIT values in
_handle_semantic_searchand_extract_limit.chromadb —
if limit and ...treatedlimit=0as falsy, returningall rows instead of zero rows when
get(limit=0)was called.weaviate — Sort key
distance if distance else 1.0collapsed0.0(exact match, the best possible cosine distance) to
1.0, rankingperfect matches last instead of first.
Fix
not isinstance(p, bool)guard at both int-detection sitesif limitwithif limit is not Noneif distancewithif distance is not NoneResolves #748
Summary by CodeRabbit