Skip to content

Commit 837cf0e

Browse files
committed
Address PR #13179 reviewer feedback - VectorStoreTextSearch translation layer improvements
Refactored TextSearchFilter to LINQ translation logic in VectorStoreTextSearch per reviewer feedback: ## Exception Handling Modernization - Eliminated exception swallowing anti-pattern (try-catch returning null) - Replaced with explicit ArgumentException/NotSupportedException with descriptive messages - Improved debugging experience with proper error bubbling ## Code Quality & Architecture - Consolidated 5 duplicate methods into unified CreateClauseExpression using switch expressions - Applied modern C# patterns (expression bodies, pattern matching) - Removed VectorSearchFilter.OldFilter legacy fallback mechanism - Reduced codebase complexity (net -203 lines: +100 insertions, -303 deletions) ## AOT Compatibility - Fixed IL2091 in TextSearchKernelBuilderExtensions DynamicallyAccessedMembers annotation - Applied RequiresDynamicCode surgically to reflection-using methods only - Maintained AOT-friendly public surface while isolating dynamic code requirements ## Validation Results - All 20 VectorStoreTextSearch tests passing with updated exception expectations - Build validation passed across all projects with --warnaserror flag - ArgumentException now thrown instead of InvalidOperationException for better error clarity ## Files Modified - VectorStoreTextSearch.cs: Core refactoring - removed technical debt, unified expression handling - TextSearchKernelBuilderExtensions.cs: Fixed AOT annotation mismatch (IL2091) - TextSearchServiceCollectionExtensions.cs: Related AOT annotation updates - VectorStoreTextSearchTests.cs: Updated test expectations for improved error handling
1 parent 30bd2c3 commit 837cf0e

File tree

4 files changed

+100
-303
lines changed

4 files changed

+100
-303
lines changed

dotnet/src/SemanticKernel.Core/Data/TextSearch/TextSearchKernelBuilderExtensions.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ public static class TextSearchKernelBuilderExtensions
1818
/// <param name="resultMapper"><see cref="ITextSearchResultMapper" /> instance that can map a TRecord to a <see cref="TextSearchResult"/></param>
1919
/// <param name="options">Options used to construct an instance of <see cref="VectorStoreTextSearch{TRecord}"/></param>
2020
/// <param name="serviceId">An optional service id to use as the service key.</param>
21-
public static IKernelBuilder AddVectorStoreTextSearch<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] TRecord>(
21+
public static IKernelBuilder AddVectorStoreTextSearch<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.PublicMethods)] TRecord>(
2222
this IKernelBuilder builder,
2323
ITextSearchStringMapper? stringMapper = null,
2424
ITextSearchResultMapper? resultMapper = null,

dotnet/src/SemanticKernel.Core/Data/TextSearch/TextSearchServiceCollectionExtensions.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ public static class TextSearchServiceCollectionExtensions
2222
/// <param name="resultMapper"><see cref="ITextSearchResultMapper" /> instance that can map a TRecord to a <see cref="TextSearchResult"/></param>
2323
/// <param name="options">Options used to construct an instance of <see cref="VectorStoreTextSearch{TRecord}"/></param>
2424
/// <param name="serviceId">An optional service id to use as the service key.</param>
25-
public static IServiceCollection AddVectorStoreTextSearch<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] TRecord>(
25+
public static IServiceCollection AddVectorStoreTextSearch<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.PublicMethods)] TRecord>(
2626
this IServiceCollection services,
2727
ITextSearchStringMapper? stringMapper = null,
2828
ITextSearchResultMapper? resultMapper = null,
@@ -58,7 +58,7 @@ public static class TextSearchServiceCollectionExtensions
5858
/// <param name="resultMapper"><see cref="ITextSearchResultMapper" /> instance that can map a TRecord to a <see cref="TextSearchResult"/></param>
5959
/// <param name="options">Options used to construct an instance of <see cref="VectorStoreTextSearch{TRecord}"/></param>
6060
/// <param name="serviceId">An optional service id to use as the service key.</param>
61-
public static IServiceCollection AddVectorStoreTextSearch<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] TRecord>(
61+
public static IServiceCollection AddVectorStoreTextSearch<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.PublicMethods)] TRecord>(
6262
this IServiceCollection services,
6363
string vectorSearchableServiceId,
6464
ITextSearchStringMapper? stringMapper = null,
@@ -104,7 +104,7 @@ public static class TextSearchServiceCollectionExtensions
104104
/// <param name="options">Options used to construct an instance of <see cref="VectorStoreTextSearch{TRecord}"/></param>
105105
/// <param name="serviceId">An optional service id to use as the service key.</param>
106106
[Obsolete("Use the overload which doesn't accept a textEmbeddingGenerationServiceId, and configure an IEmbeddingGenerator instead with the collection represented by vectorSearchServiceId.")]
107-
public static IServiceCollection AddVectorStoreTextSearch<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] TRecord>(
107+
public static IServiceCollection AddVectorStoreTextSearch<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.PublicMethods)] TRecord>(
108108
this IServiceCollection services,
109109
string vectorSearchServiceId,
110110
string textEmbeddingGenerationServiceId,

0 commit comments

Comments
 (0)