Conversation
Find(string) walks every property of every item and hands each element of an
enumerable property to a recursive helper typed as dynamic. A Dictionary
property satisfies IsEnumerable, so each element arrived as a KeyValuePair
struct, and the helper's "current == null" guard made the runtime binder throw
because that struct has no equality operator. Any model with a populated
dictionary was unsearchable:
RuntimeBinderException: Operator '==' cannot be applied to operands of type
'System.Collections.Generic.KeyValuePair<string,string>' and '<null>'
An empty dictionary never enters the loop, which is why this was not caught
before.
Replace the guard with "current is null". Pattern matching is not dynamically
bound, so the check no longer depends on the runtime type having an equality
operator. This also covers lists of any other struct without operator ==, which
failed the same way.
Handle IDictionary before the enumerable branch, matching how HandleTyped and
HandleExpando already treat dictionaries, and search keys and values as
separate values. Without this the search only worked by comparing against the
"[key, value]" text a KeyValuePair stringifies to, making the brackets and
separator searchable, so Find("[plan") and Find(", gold") were false positives.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kn184u1sK2zuFMTc4PNdrR
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughFull-text search now recursively checks dictionary keys and values, safely handles dynamic nulls, and adds collection tests for matching, empty dictionaries, non-string values, formatting characters, interface-typed dictionaries, and no-match cases. ChangesDictionary full-text search
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@JsonFlatFileDataStore/ObjectExtensions.cs`:
- Around line 103-114: Broaden IsDictionary and the dictionary handling in
AnyPropertyHasValue to recognize generic IDictionary<TKey,TValue> and
IReadOnlyDictionary<TKey,TValue> declarations, while continuing to enumerate
entries as key/value pairs before the enumerable path. Add a regression test
using an interface-typed dictionary property that verifies nested key or value
matches are found without searching KeyValuePair string representations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b39b0162-ce06-49f9-8025-457440dc19cd
📒 Files selected for processing (2)
JsonFlatFileDataStore.Test/CollectionQueryTests.csJsonFlatFileDataStore/ObjectExtensions.cs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
JsonFlatFileDataStore/ObjectExtensions.cs (1)
103-118: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftHandle generic dictionary interfaces, not only
IDictionary.
propValue is IDictionarymisses runtime values that implement onlyIDictionary<TKey,TValue>orIReadOnlyDictionary<TKey,TValue>, causing them to enter the enumerable branch and be searched throughKeyValuePair.ToString(). This can miss key/value matches and reintroduce false positives from entry formatting. Neither generic interface requires the non-genericIDictionarycontract. (learn.microsoft.com)The new test assigns concrete
Dictionary<string, string>instances to both interface properties, so it does not cover this runtime case. Detect generic dictionary interfaces and extract each entry’sKeyandValuebefore the enumerable fallback.🔎 Verify runtime coverage
#!/usr/bin/env bash set -euo pipefail rg -n 'IReadOnlyDictionary|IDictionary|ReadOnlyDictionary|ImmutableDictionary|new Dictionary' \ JsonFlatFileDataStore.Test JsonFlatFileDataStoreAdd a regression test using a dictionary implementation that does not also implement
System.Collections.IDictionary.🤖 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 `@JsonFlatFileDataStore/ObjectExtensions.cs` around lines 103 - 118, Update the property-value handling around the IDictionary branch to detect runtime implementations of generic IDictionary<TKey,TValue> and IReadOnlyDictionary<TKey,TValue>, including implementations that do not implement non-generic IDictionary. Extract and recursively search each entry’s Key and Value before the enumerable fallback; preserve the existing ExpandoObject handling and add a regression test using a generic dictionary implementation without System.Collections.IDictionary.Source: MCP tools
🤖 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.
Inline comments:
In `@CHANGELOG.md`:
- Line 8: Update the CHANGELOG entry to use the established hyphenated spelling
“full-text search” consistently.
---
Outside diff comments:
In `@JsonFlatFileDataStore/ObjectExtensions.cs`:
- Around line 103-118: Update the property-value handling around the IDictionary
branch to detect runtime implementations of generic IDictionary<TKey,TValue> and
IReadOnlyDictionary<TKey,TValue>, including implementations that do not
implement non-generic IDictionary. Extract and recursively search each entry’s
Key and Value before the enumerable fallback; preserve the existing
ExpandoObject handling and add a regression test using a generic dictionary
implementation without System.Collections.IDictionary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b9f88630-6226-44e9-9e64-9dc2f02abb91
📒 Files selected for processing (4)
CHANGELOG.mdJsonFlatFileDataStore.Test/CollectionQueryTests.csJsonFlatFileDataStore.Test/TestModels.csJsonFlatFileDataStore/ObjectExtensions.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- JsonFlatFileDataStore.Test/CollectionQueryTests.cs
| * FIXED: Retry JSON parse on read to tolerate concurrent partial-file writes | ||
| * FIXED: A failing commit action no longer hangs other callers in the same batch | ||
| * FIXED: Collection key in file not matching configured case is now matched case-insensitively, instead of reading empty and duplicating the key on save | ||
| * FIXED: Full text search no longer throws on models with a dictionary property, and searches dictionary keys and values |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hyphenate “full-text search”.
Use the established spelling for this compound term.
Suggested wording
- * FIXED: Full text search no longer throws on models with a dictionary property, and searches dictionary keys and values
+ * FIXED: Full-text search no longer throws on models with a dictionary property, and searches dictionary keys and values📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * FIXED: Full text search no longer throws on models with a dictionary property, and searches dictionary keys and values | |
| * FIXED: Full-text search no longer throws on models with a dictionary property, and searches dictionary keys and values |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~8-~8: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...nd duplicating the key on save * FIXED: Full text search no longer throws on models with ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🤖 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 `@CHANGELOG.md` at line 8, Update the CHANGELOG entry to use the established
hyphenated spelling “full-text search” consistently.
Source: Linters/SAST tools
The dictionary branch of FullTextSearch tested the declared property type via
IsDictionary, which only recognises the non-generic IDictionary. A property
declared as IDictionary<,> or IReadOnlyDictionary<,> therefore fell through to
the enumerable branch and matched the "[key, value]" text a KeyValuePair
stringifies to, so Find("[plan") and Find(", gold") were false positives.
Test the runtime value with "propValue is IDictionary" instead. A concrete
Dictionary implements the non-generic IDictionary regardless of how the property
is declared, while ExpandoObject does not, so it is still recursed as an object.
This keeps the shared IsDictionary helper untouched, avoiding changes to the
CopyProperties merge paths.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kn184u1sK2zuFMTc4PNdrR
e87983e to
4a73d31
Compare
1 participant
Problem
Find(string)throws on any model with a populated dictionary property, making full-text search unusable for those models:FullTextSearchwalks every property of every item. ADictionary<,>satisfiesIsEnumerable, so each element reaches the recursive helper as aKeyValuePair<,>struct typeddynamic, and the helper's first statement isif (current == null). The runtime binder cannot apply==to a struct with no equality operator.An empty dictionary never enters the loop, which is why the existing
FullTextSearch_Typed/FullTextSearch_Dynamictests pass — they only exercise models without a populated dictionary.Fix
Two parts:
current == null→current is null. Pattern matching is not dynamically bound, so the null check no longer depends on the runtime type having an equality operator. This is the root cause, and it also covers lists of any other struct withoutoperator ==, which failed identically. (List<int>was fine only becauseinthas a lifted==.)Handle
IDictionarybefore the enumerable branch, matching howHandleTypedandHandleExpandoin the same file already treat dictionaries, and search keys and values as separate values.Part 2 is what makes the behaviour intentional rather than incidental. With only part 1 the search "works", but by comparing against the
[key, value]text aKeyValuePairstringifies to — soFind("[plan")andFind(", gold")return false positives. Both are now pinned by a test.Tests
Seven tests added to
CollectionQueryTests, written before the fix and each watched failing first:Dictionary<int, int>)"[plan",", gold")Full suite: 245 passed, 0 failed (238 before).
Independently confirmed by the stress harness that found the bug: distinct failures went 8 → 7, with a result-JSON diff showing exactly one resolved and zero new.
Note
The same defect exists on
newtonsoft-to-system-text-json— the fix should carry over directly.🤖 Generated with Claude Code
https://claude.ai/code/session_01Kn184u1sK2zuFMTc4PNdrR
Summary by CodeRabbit
New Features
Bug Fixes