.Net: fix(Redis): RedisJsonMapper excludes unannotated POCO properties from JSON payload#14023
Conversation
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 91%
✓ Correctness
The PR correctly fixes the issue of unannotated POCO properties leaking into Redis JSON payloads. The implementation selectively builds the JSON object from model.DataProperties and model.VectorProperties, matching the approach in RedisJsonDynamicMapper. Key extraction now uses GetValueAsObject directly (avoiding full serialization), and null embedding handling is improved. The logic is sound and consistent with the existing infrastructure.
✓ Security Reliability
The change correctly addresses data leakage by building the JSON payload selectively from annotated properties only, rather than serializing the full POCO. The null-handling for Embedding properties is improved (old code used null-forgiving
!that would NRE on null embedings; new code gracefully handles null). Key extraction provides a clear error for null keys. The approach aligns with the existing RedisJsonDynamicMapper pattern. No security or reliability issues found.
✓ Test Coverage
The test updates in RedisJsonCollectionTests correctly verify the new behavior (unannotated properties excluded from JSON payloads) via exact JSON string matching. However, the dedicated mapper unit test (RedisJsonMapperTests.MapsAllFieldsFromDataToStorageModel) sets NotAnnotated to a non-null value but never asserts its absence from the output — meaning a regression at the mapper level would not be caught by that test. Additionally, new code paths for null vectors and Guid keys lack direct test coverage.
✗ Design Approach
I found one design issue in the new selective Redis POCO serialization path: it now hard-codes
nullfields into the JSON payload, which bypasses caller-providedJsonSerializerOptionsnull-handling semantics that this connector advertises and that other POCO mappers in the repo preserve.
Suggestions
- Honor
JsonSerializerOptionswhen deciding whether a null POCO property should be emitted, instead of always writingstorageName: null;WeaviateMapper's POCO path shows the existing pattern of only materializing properties that actually have values (dotnet/src/VectorData/Weaviate/WeaviateMapper.cs:52-58).
Automated review by nileshpatil6's agents
Fixes #14021
RedisJsonMapper.MapFromDataToStorageModelwas serializing the full POCO object viaJsonSerializer.SerializeToNode, then removing only the key property. This caused unannotated public properties to be persisted into Redis JSON payloads.The fix builds the JSON object selectively from
model.DataPropertiesandmodel.VectorPropertiesonly, matching the approach already used inRedisJsonDynamicMapper.Updated unit tests to remove
NotAnnotatedfrom expected payloads and deleted the existing// TODO: Fix issue where NotAnnotated is being included in the JSON.comments.