.Net: Fix RedisJsonCollection upsert persisting unannotated POCO properties#14030
.Net: Fix RedisJsonCollection upsert persisting unannotated POCO properties#14030jluocsa wants to merge 3 commits into
Conversation
…erties (microsoft#14021) RedisJsonMapper.MapFromDataToStorageModel previously serialized the entire record via JsonSerializer.SerializeToNode and only stripped the key, so any public POCO property without a [VectorStoreKey]/[VectorStoreData]/[VectorStoreVector] attribute (and not declared in the collection definition) was persisted to Redis. Build the JSON payload explicitly from model.DataProperties and model.VectorProperties, mirroring RedisJsonDynamicMapper. Storage keys still come from property.StorageName (already populated by CollectionJsonModelBuilder from JsonPropertyName / PropertyNamingPolicy / the .NET property name), so the produced JSON for schema-annotated properties matches the previous behavior. Updates RedisJsonCollectionTests upsert expectations (drops NotAnnotated:null fragments + TODOs) and adds an explicit assertion in RedisJsonMapperTests that unannotated properties are absent from the serialized JSON.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates Redis JSON mapping to persist only schema-annotated fields (excluding unannotated POCO properties), and adjusts unit tests to validate the new JSON payload shape.
Changes:
- Build Redis JSON payload from
DataProperties+VectorPropertiesrather than serializing the whole model. - Ensure unannotated properties (e.g.,
NotAnnotated) are not written to Redis JSON. - Update unit tests’ expected JSON and add assertions that unannotated fields are absent.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| dotnet/src/VectorData/Redis/RedisJsonMapper.cs | Reworks JSON construction to include only schema-defined data/vector properties and exclude unannotated POCO properties. |
| dotnet/test/VectorData/Redis.UnitTests/RedisJsonMapperTests.cs | Adds assertions ensuring unannotated fields are not present in the mapped JSON. |
| dotnet/test/VectorData/Redis.UnitTests/RedisJsonCollectionTests.cs | Updates expected JSON payloads to no longer include NotAnnotated and removes obsolete TODOs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
The three failing CI legs ( I opened #14031 as a single-line bump to |
Per Copilot's review on microsoft#14030: replace the null-forgiving operator on the mapped JsonObject with an explicit Assert.NotNull, then drop the '?' / '!' operators on subsequent index access and ContainsKey. This keeps test failures diagnostic (clean assertion failure instead of NullReferenceException) and reads as a single contract: 'the mapper must return a JsonObject; here is what it should contain'.
Per Copilot's review on microsoft#14030: collapse the two near-identical switch-expression + foreach-Span blocks for float and double vector payloads into a single type-switch that funnels every supported shape (ReadOnlyMemory<T>, Embedding<T>, T[]) through a shared AppendVector<T>(JsonArray, ReadOnlySpan<T>) helper. Behavior is unchanged: same set of accepted vector shapes, same ordering, same UnreachableException on an unsupported runtime type.
|
@jluocsa, thanks for the contribution. This looks like a good improvement. CC @roji |
Motivation and Context
Fixes #14021.
RedisJsonCollection<TKey, TRecord>.UpsertAsyncwas persisting POCO properties that are not part of the vector-store schema (no[VectorStoreKey]/[VectorStoreData]/[VectorStoreVector]attribute and not declared in the collection definition).Root cause:
RedisJsonMapper<TConsumerDataModel>.MapFromDataToStorageModelserialized the entire user record withJsonSerializer.SerializeToNode(dataModel, jsonSerializerOptions)and only removed the key field afterwards. Anything else on the type — including unannotated public properties — flowed straight into the Redis JSON payload.RedisJsonDynamicMapper(theDictionary<string, object?>path) already does the right thing: it iteratesmodel.DataProperties/model.VectorPropertiesand only emits those.Description
Refactor
RedisJsonMapper<TConsumerDataModel>.MapFromDataToStorageModelto build the storageJsonObjectexplicitly from theCollectionModel, mirroring the dynamic mapper:model.KeyProperty.GetValueAsObject(dataModel)and validate it is a non-null string (unchanged semantics — Redis JSON keys are still string-typed and the constructor still validates that up front).model.DataPropertiesentry, keyed bydataProperty.StorageName(whichCollectionJsonModelBuilder.Customize()already populates from[JsonPropertyName]/ the configuredPropertyNamingPolicy/ the .NET property name — exactly what the previous whole-object serialization produced for annotated properties). Each value is serialized throughJsonSerializer.SerializeToNode(value, dataProperty.Type, jsonSerializerOptions)so all of the user'sJsonSerializerOptionscontinue to apply.model.VectorPropertiesentry, keyed byproperty.StorageName, with the existing precedence: generated embedding for this slot, otherwise the value pulled from the POCO. Vector values continue to be written as a flatJsonArrayoffloat/double. The sameReadOnlyMemory<T> | Embedding<T> | T[]switch as inRedisJsonDynamicMapperis used, so bothfloatanddoublevector types and their nullable variants are handled.Properties without a vector-store attribute and not declared in the collection definition are now simply not serialized — same behavior as the dynamic mapper.
Tests:
RedisJsonCollectionTests.CanUpsertRecordAsync/CanUpsertManyRecordsAsync: dropped the"NotAnnotated":null/"notAnnotated":nullfragments from the expected JSON payloads and removed the matching// TODO: Fix issue where NotAnnotated is being included in the JSON.comments.RedisJsonMapperTests.MapsAllFieldsFromDataToStorageModel{,WithCustomSerializerOptions}: added a positiveAssert.False(jsonObject.ContainsKey("NotAnnotated"))(and thenotAnnotatedcamelCase variant) to lock in the fix.Other tests in
Redis.UnitTestswere not touched.Verification
Result:
Total tests: 109. Passed: 109. Failed: 0. Skipped: 0.The targeted reproducer from the issue body:
passes on all four
[InlineData]rows.Contribution Checklist
Redis.UnitTests)Closes #14021