.Net: Share generic data model validations and use everywhere. (#8923)

### Motivation and Context If someone uses the generic data model, they need to also supply a record definition, otherwise we don't know how to map the data from storage to the generic data model. Also, if someone uses the generic data model and they use a key type other than the ones we support out of the box, they need to provide a custom mapper to do that key mapping. ### Description - This PR adds checks for this with helpful messaging to a shared class. - Adds the validation calls to all connectors that support the generic data model. ### Contribution Checklist  - [x] The code builds clean without any errors or warnings - [x] The PR follows the [SK Contribution Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [x] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone 😄
microsoft · Sep 23, 2024 · 0b5c769 · 0b5c769
1 parent a5570b1
commit 0b5c769
Show file tree

Hide file tree

Showing 6 changed files with 99 additions and 9 deletions.
diff --git a/...rc/Connectors/Connectors.Memory.AzureAISearch/AzureAISearchVectorStoreRecordCollection.cs b/...rc/Connectors/Connectors.Memory.AzureAISearch/AzureAISearchVectorStoreRecordCollection.cs
@@ -105,15 +105,8 @@ public AzureAISearchVectorStoreRecordCollection(SearchIndexClient searchIndexCli
         // Verify.
         Verify.NotNull(searchIndexClient);
         Verify.NotNullOrWhiteSpace(collectionName);
-        Verify.True(
-            !(typeof(TRecord).IsGenericType &&
-                typeof(TRecord).GetGenericTypeDefinition() == typeof(VectorStoreGenericDataModel<>) &&
-                typeof(TRecord).GetGenericArguments()[0] != typeof(string) &&
-                options?.JsonObjectCustomMapper is null),
-            "A data model of VectorStoreGenericDataModel with a different key type than string, is not supported by the default mappers. Please provide your own mapper to map to your chosen key type.");
-        Verify.True(
-            !(typeof(TRecord) == typeof(VectorStoreGenericDataModel<string>) && options?.VectorStoreRecordDefinition is null),
-            $"A {nameof(VectorStoreRecordDefinition)} must be provided when using {nameof(VectorStoreGenericDataModel<string>)}.");
+        VectorStoreRecordPropertyReader.VerifyGenericDataModelKeyType(typeof(TRecord), options?.JsonObjectCustomMapper is not null, s_supportedKeyTypes);
+        VectorStoreRecordPropertyReader.VerifyGenericDataModelDefinitionSupplied(typeof(TRecord), options?.VectorStoreRecordDefinition is not null);
 
         // Assign.
         this._searchIndexClient = searchIndexClient;

diff --git a/dotnet/src/Connectors/Connectors.Memory.Qdrant/QdrantVectorStoreRecordCollection.cs b/dotnet/src/Connectors/Connectors.Memory.Qdrant/QdrantVectorStoreRecordCollection.cs
@@ -82,6 +82,8 @@ internal QdrantVectorStoreRecordCollection(MockableQdrantClient qdrantClient, st
         // Verify.
         Verify.NotNull(qdrantClient);
         Verify.NotNullOrWhiteSpace(collectionName);
+        VectorStoreRecordPropertyReader.VerifyGenericDataModelKeyType(typeof(TRecord), options?.PointStructCustomMapper is not null, s_supportedKeyTypes);
+        VectorStoreRecordPropertyReader.VerifyGenericDataModelDefinitionSupplied(typeof(TRecord), options?.VectorStoreRecordDefinition is not null);
 
         // Assign.
         this._qdrantClient = qdrantClient;

diff --git a/dotnet/src/Connectors/Connectors.Memory.Redis/RedisHashSetVectorStoreRecordCollection.cs b/dotnet/src/Connectors/Connectors.Memory.Redis/RedisHashSetVectorStoreRecordCollection.cs
@@ -94,6 +94,8 @@ public RedisHashSetVectorStoreRecordCollection(IDatabase database, string collec
         // Verify.
         Verify.NotNull(database);
         Verify.NotNullOrWhiteSpace(collectionName);
+        VectorStoreRecordPropertyReader.VerifyGenericDataModelKeyType(typeof(TRecord), options?.HashEntriesCustomMapper is not null, s_supportedKeyTypes);
+        VectorStoreRecordPropertyReader.VerifyGenericDataModelDefinitionSupplied(typeof(TRecord), options?.VectorStoreRecordDefinition is not null);
 
         // Assign.
         this._database = database;

diff --git a/dotnet/src/Connectors/Connectors.Memory.Redis/RedisJsonVectorStoreRecordCollection.cs b/dotnet/src/Connectors/Connectors.Memory.Redis/RedisJsonVectorStoreRecordCollection.cs
@@ -80,6 +80,8 @@ public RedisJsonVectorStoreRecordCollection(IDatabase database, string collectio
         // Verify.
         Verify.NotNull(database);
         Verify.NotNullOrWhiteSpace(collectionName);
+        VectorStoreRecordPropertyReader.VerifyGenericDataModelKeyType(typeof(TRecord), options?.JsonNodeCustomMapper is not null, s_supportedKeyTypes);
+        VectorStoreRecordPropertyReader.VerifyGenericDataModelDefinitionSupplied(typeof(TRecord), options?.VectorStoreRecordDefinition is not null);
 
         // Assign.
         this._database = database;

diff --git a/dotnet/src/InternalUtilities/src/Data/VectorStoreRecordPropertyReader.cs b/dotnet/src/InternalUtilities/src/Data/VectorStoreRecordPropertyReader.cs
@@ -397,6 +397,62 @@ var interfaceType when interfaceType.GetInterfaces().FirstOrDefault(i => i.IsGen
         }
     }
 
+    /// <summary>
+    /// Checks that if the provided <paramref name="recordType"/> is a <see cref="VectorStoreGenericDataModel{T}"/> that the key type is supported by the default mappers.
+    /// If not supported, a custom mapper must be supplied, otherwise an exception is thrown.
+    /// </summary>
+    /// <param name="recordType">The type of the record data model used by the connector.</param>
+    /// <param name="customMapperSupplied">A value indicating whether a custom mapper was supplied to the connector</param>
+    /// <param name="allowedKeyTypes">The list of key types supported by the default mappers.</param>
+    /// <exception cref="ArgumentException">Thrown if the key type of the <see cref="VectorStoreGenericDataModel{T}"/> is not supported by the default mappers and a custom mapper was not supplied.</exception>
+    public static void VerifyGenericDataModelKeyType(Type recordType, bool customMapperSupplied, IEnumerable<Type> allowedKeyTypes)
+    {
+        // If we are not dealing with a generic data model, no need to check anything else.
+        if (!recordType.IsGenericType || recordType.GetGenericTypeDefinition() != typeof(VectorStoreGenericDataModel<>))
+        {
+            return;
+        }
+
+        // If the key type is supported, we are good.
+        var keyType = recordType.GetGenericArguments()[0];
+        if (allowedKeyTypes.Contains(keyType))
+        {
+            return;
+        }
+
+        // If the key type is not supported out of the box, but a custom mapper was supplied, we are good.
+        if (customMapperSupplied)
+        {
+            return;
+        }
+
+        throw new ArgumentException($"The key type '{keyType.FullName}' of data model '{nameof(VectorStoreGenericDataModel<string>)}' is not supported by the default mappers. " +
+            $"Only the following key types are supported: {string.Join(", ", allowedKeyTypes)}. Please provide your own mapper to map to your chosen key type.");
+    }
+
+    /// <summary>
+    /// Checks that if the provided <paramref name="recordType"/> is a <see cref="VectorStoreGenericDataModel{T}"/> that a <see cref="VectorStoreRecordDefinition"/> is also provided.
+    /// </summary>
+    /// <param name="recordType">The type of the record data model used by the connector.</param>
+    /// <param name="recordDefinitionSupplied">A value indicating whether a record definition was supplied to the connector.</param>
+    /// <exception cref="ArgumentException">Thrown if a <see cref="VectorStoreRecordDefinition"/> is not provided when using <see cref="VectorStoreGenericDataModel{T}"/>.</exception>
+    public static void VerifyGenericDataModelDefinitionSupplied(Type recordType, bool recordDefinitionSupplied)
+    {
+        // If we are not dealing with a generic data model, no need to check anything else.
+        if (!recordType.IsGenericType || recordType.GetGenericTypeDefinition() != typeof(VectorStoreGenericDataModel<>))
+        {
+            return;
+        }
+
+        // If we are dealing with a generic data model, and a record definition was supplied, we are good.
+        if (recordDefinitionSupplied)
+        {
+            return;
+        }
+
+        throw new ArgumentException($"A {nameof(VectorStoreRecordDefinition)} must be provided when using '{nameof(VectorStoreGenericDataModel<string>)}'.");
+    }
+
     /// <summary>
     /// Get the JSON property name of a property by using the <see cref="JsonPropertyNameAttribute"/> if available, otherwise
     /// using the <see cref="JsonNamingPolicy"/> if available, otherwise falling back to the property name.

diff --git a/dotnet/src/SemanticKernel.UnitTests/Data/VectorStoreRecordPropertyReaderTests.cs b/dotnet/src/SemanticKernel.UnitTests/Data/VectorStoreRecordPropertyReaderTests.cs
@@ -254,6 +254,41 @@ public void VerifyPropertyTypesFailsForDisallowedTypes()
         Assert.Equal("Data properties must be one of the supported types: System.Int32, System.Single. Type of the property 'Data' is System.String.", ex2.Message);
     }
 
+    [Theory]
+    [InlineData(typeof(SinglePropsModel), false, new Type[] { typeof(string) }, false)]
+    [InlineData(typeof(VectorStoreGenericDataModel<string>), false, new Type[] { typeof(string), typeof(ulong) }, false)]
+    [InlineData(typeof(VectorStoreGenericDataModel<int>), true, new Type[] { typeof(string), typeof(ulong) }, false)]
+    [InlineData(typeof(VectorStoreGenericDataModel<int>), false, new Type[] { typeof(string), typeof(ulong) }, true)]
+    public void VerifyGenericDataModelKeyTypeThrowsOnlyForUnsupportedKeyTypeWithoutCustomMapper(Type recordType, bool customMapperSupplied, IEnumerable<Type> allowedKeyTypes, bool shouldThrow)
+    {
+        if (shouldThrow)
+        {
+            var ex = Assert.Throws<ArgumentException>(() => VectorStoreRecordPropertyReader.VerifyGenericDataModelKeyType(recordType, customMapperSupplied, allowedKeyTypes));
+            Assert.Equal("The key type 'System.Int32' of data model 'VectorStoreGenericDataModel' is not supported by the default mappers. Only the following key types are supported: System.String, System.UInt64. Please provide your own mapper to map to your chosen key type.", ex.Message);
+        }
+        else
+        {
+            VectorStoreRecordPropertyReader.VerifyGenericDataModelKeyType(recordType, customMapperSupplied, allowedKeyTypes);
+        }
+    }
+
+    [Theory]
+    [InlineData(typeof(SinglePropsModel), false, false)]
+    [InlineData(typeof(VectorStoreGenericDataModel<string>), true, false)]
+    [InlineData(typeof(VectorStoreGenericDataModel<string>), false, true)]
+    public void VerifyGenericDataModelDefinitionSuppliedThrowsOnlyForMissingDefinition(Type recordType, bool definitionSupplied, bool shouldThrow)
+    {
+        if (shouldThrow)
+        {
+            var ex = Assert.Throws<ArgumentException>(() => VectorStoreRecordPropertyReader.VerifyGenericDataModelDefinitionSupplied(recordType, definitionSupplied));
+            Assert.Equal("A VectorStoreRecordDefinition must be provided when using 'VectorStoreGenericDataModel'.", ex.Message);
+        }
+        else
+        {
+            VectorStoreRecordPropertyReader.VerifyGenericDataModelDefinitionSupplied(recordType, definitionSupplied);
+        }
+    }
+
     [Fact]
     public void VerifyStoragePropertyNameMapChecksStorageNameAndFallsBackToPropertyName()
     {