-
Notifications
You must be signed in to change notification settings - Fork 4.4k
.Net: Net: feat: Modernize TavilyTextSearch and BraveTextSearch connectors with ITextSearch<TRecord> interface (microsoft#10456) #13191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
westey-m
merged 14 commits into
microsoft:feature-text-search-linq
from
alzarei:feature-text-search-linq-pr5
Nov 21, 2025
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
c7a2b06 to
caa67ec
Compare
edeb8bf to
d771436
Compare
…rchResults<TRecord> - Change interface return type from KernelSearchResults<object> to KernelSearchResults<TRecord> - Update VectorStoreTextSearch implementation with new GetResultsAsTRecordAsync helper - Keep GetResultsAsRecordAsync for legacy ITextSearch interface backward compatibility - Update 3 unit tests to use strongly-typed DataModel instead of object Benefits: - Improved type safety - no more casting required - Better IntelliSense and developer experience - Zero breaking changes to legacy ITextSearch interface - All 19 unit tests pass This is Part 2.1 of the Issue microsoft#10456 multi-PR chain, refining the API .
[Ifeat: Modernize TavilyTextSearch and BraveTextSearch with ITextSearch<TRecord> interface - Add TavilyWebPage and BraveWebPage model classes for type-safe LINQ filtering - Implement dual interface support (ITextSearch + ITextSearch<TRecord>) in both connectors - Add LINQ-to-API conversion logic for Tavily and Brave search parameters - Maintain 100% backward compatibility with existing ITextSearch usage - Enable compile-time type safety and IntelliSense support for web search filters Addresses microsoft#10456
…rror messages - Added support for OrElse (||) operators in LINQ expressions - Added NotEqual (!=) operator support with appropriate error handling - Added UnaryExpression (NOT) support with proper limitations - Enhanced Contains method support for array.Contains(property) patterns - Improved error messages with usage examples - Added constants for API parameter names for better maintainability - Added TODO comments for potential code sharing opportunities Changes apply to both BraveTextSearch and TavilyTextSearch implementations.
- Changed Url property from string? to Uri? in TavilyWebPage - Changed Url property from string? to Uri? in BraveWebPage - Updated constructors to accept Uri? parameters - Added null-safe string-to-Uri conversions in factory methods - Fixed image URL handling in TavilyTextSearch Resolves all 31 CA1056 violations (URI properties should not be strings).
…h connectors - Implement IsMemoryExtensionsContains() detection in TavilyTextSearch and BraveTextSearch - Handle C# 14+ MemoryExtensions.Contains method resolution (static calls with 2-3 parameters) - Provide clear error messages explaining API limitations and suggesting alternatives - Maintain backward compatibility with C# 13- Enumerable.Contains patterns Validation: Build (0 warnings), Tests (1574 passed), AOT compatible Fixes microsoft#10456
…on handling - Added comprehensive generic interface tests for both Tavily and Brave connectors - TavilyTextSearchTests: 3 new tests for ITextSearch<TavilyWebPage> interface - BraveTextSearchTests: 3 new tests for ITextSearch<BraveWebPage> interface - Tests cover SearchAsync, GetSearchResultsAsync, and GetTextSearchResultsAsync methods - Added proper mocking with MultipleHttpMessageHandlerStub and test data - Fixed collection Contains exception handling to prevent graceful degradation - Modified ConvertToLegacyOptions in both connectors to re-throw critical exceptions - Collection Contains patterns now properly throw NotSupportedException instead of degrading - Maintains graceful degradation for other unsupported LINQ patterns - Fixed previously failing CollectionContainsFilterThrowsNotSupportedExceptionAsync test - Enhanced C# 14 MemoryExtensions.Contains compatibility - Improved error messages to cover both C# 13- (Enumerable.Contains) and C# 14+ (MemoryExtensions.Contains) - All collection Contains patterns now properly handled regardless of C# language version Test Results: - Tavily: 30 tests passed (including 3 new generic interface tests) - Brave: 24 tests passed (including 3 new generic interface tests) - All collection Contains exception tests now pass correctly
- Update BraveTextSearch and TavilyTextSearch to properly implement ITextSearch<TRecord> - Fix unit tests to use strongly-typed generic interfaces - Ensure backward compatibility with legacy ITextSearch interface - All 105 text search tests passing, full validation completed Resolves API type safety issues described in How-to-Fix-API-Type-Change-to-TRecord-PR5.md
d771436 to
4caa829
Compare
7 tasks
…ame Generic→Linq - Revert GetSearchResultsReturnsSuccessfullyAsync to use non-generic interface (var textSearch, KernelSearchResults<object>) - This test should test backward compatibility with legacy ITextSearch interface - Only generic interface tests should use ITextSearch<TRecord> and KernelSearchResults<TRecord> - Rename 'Generic' test methods to 'Linq' to match naming convention from other PRs: - GenericSearchAsyncReturnsResultsSuccessfullyAsync → LinqSearchAsyncReturnsResultsSuccessfullyAsync - GenericGetSearchResultsAsyncReturnsResultsSuccessfullyAsync → LinqGetSearchResultsAsyncReturnsResultsSuccessfullyAsync - GenericGetTextSearchResultsAsyncReturnsResultsSuccessfullyAsync → LinqGetTextSearchResultsAsyncReturnsResultsSuccessfullyAsync - Applied to both BraveTextSearchTests.cs and TavilyTextSearchTests.cs - All 105 text search tests still passing Fix maintains proper test separation: - Non-generic tests: Test legacy ITextSearch interface compatibility - LINQ tests: Test new ITextSearch<TRecord> strongly-typed interface
alzarei
commented
Nov 6, 2025
dotnet/src/Plugins/Plugins.UnitTests/Web/Brave/BraveTextSearchTests.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Plugins/Plugins.UnitTests/Web/Tavily/TavilyTextSearchTests.cs
Outdated
Show resolved
Hide resolved
…arch
- Add SearchQueryFilterClause for query enhancement pattern
- Implement Title.Contains() → query modification for Brave and Tavily APIs
- Support LINQ expressions: results.Where(r => r.Title.Contains('AI'))
- Query enhancement: 'base query' + ' AI' for improved relevance
- Maintains backward compatibility with legacy ITextSearch interface
- All 130 web plugin tests passing
Technical approach:
- SearchQueryFilterClause: New filter clause type for search term addition
- ConvertToLegacyOptionsWithQuery: Extract and append search terms to query
- Different from Bing (direct operators) and Google (API parameters)
- Optimized for APIs without field-specific search capabilities
- Fix RCS1037 trailing whitespace errors on lines 12, 16, 18 - Add missing final newline to prevent FINALNEWLINE error - Fix file encoding issues (BOM removal) - Addresses GitHub CI/CD build failures for net8.0, net462, netstandard2.0 targets All formatting issues resolved to match GitHub Actions requirements.
Changed SearchQueryFilterClause from public to internal in VectorData.Abstractions to reduce public API surface area per reviewer feedback. Since FilterClause has an internal constructor, SearchQueryFilterClause cannot be moved to Plugins.Web. Using InternalsVisibleTo pattern (already used 20+ times in codebase) to grant Plugins.Web access to this implementation detail. - Changed 'public sealed class' to 'internal sealed class' in SearchQueryFilterClause.cs - Added InternalsVisibleTo for Microsoft.SemanticKernel.Plugins.Web in VectorData.Abstractions.csproj - Verified: 0 warnings, 0 errors, 54 web plugin tests passing
Reverting the internal + InternalsVisibleTo approach per reviewer feedback. Making SearchQueryFilterClause public is the cleanest solution: - Legitimate, reusable abstraction for text search scenarios - Respects FilterClause architecture (stays in VectorData.Abstractions) - No InternalsVisibleTo complexity or CS0436 warnings - Precedent: Other FilterClause types are public (EqualToFilterClause, etc.) - Other search connectors can benefit from this pattern Trade-off: Adds one class to VectorData.Abstractions public API surface, but it's a well-scoped, documented filter clause pattern with clear use cases. Addresses reviewer question about SearchQueryFilterClause visibility.
alzarei
commented
Nov 12, 2025
dotnet/src/VectorData/VectorData.Abstractions/FilterClauses/SearchQueryFilterClause.cs
Outdated
Show resolved
Hide resolved
…structor public Per reviewer feedback, moving SearchQueryFilterClause from Microsoft.Extensions.VectorData to Microsoft.SemanticKernel.Plugins.Web namespace as internal class, where it's actually used (Brave/Tavily connectors). Changes: - FilterClause.cs: Changed constructor from 'internal' to 'public' to allow external inheritance - SearchQueryFilterClause.cs: Moved to Plugins.Web/FilterClauses/ as internal sealed class - VectorData.Abstractions.csproj: Added CA1012 suppression (abstract types with public constructors) Trade-off: Opening FilterClause to external inheritance is accepted to minimize MEVD public API surface.
alzarei
added a commit
to alzarei/semantic-kernel
that referenced
this pull request
Nov 16, 2025
…terClause decision in ADR-0065 Documents the architectural role of FilterClause as the common translation layer and updates the SearchQueryFilterClause visibility decision to reflect the implemented approach. Changes: - Added FilterClause architecture section with diagrams showing dual path convergence - Updated SearchQueryFilterClause decision: Made FilterClause constructor public and moved SearchQueryFilterClause to Plugins.Web (internal) - Clarified SearchQueryFilterClause is LINQ-only (never created by users) - Documented trade-off: Opening FilterClause to external inheritance to minimize MEVD API surface - Explained why FilterClause stays after legacy removal (LINQ translator needs it) - Distinguished user-facing APIs (removed in Phase 3) from internal translation layer (stays) - Added cross-reference between architecture explanation and decision rationale sections This captures the architectural discussion and final decision from PR microsoft#13191 review with @westey-m.
westey-m
reviewed
Nov 19, 2025
dotnet/src/VectorData/VectorData.Abstractions/VectorData.Abstractions.csproj
Outdated
Show resolved
Hide resolved
westey-m
approved these changes
Nov 19, 2025
a5ba264
into
microsoft:feature-text-search-linq
10 checks passed
This was referenced Nov 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
kernel.core
kernel
Issues or pull requests impacting the core kernel
.NET
Issue or Pull requests regarding .NET code
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Modernize TavilyTextSearch and BraveTextSearch connectors with ITextSearch interface
Problem Statement
The TavilyTextSearch and BraveTextSearch connectors currently implement only the legacy ITextSearch interface, forcing users to use clause-based TextSearchFilter instead of modern type-safe LINQ expressions. Additionally, the existing LINQ support is limited to basic expressions (equality, AND operations).
Technical Approach
This PR modernizes both connectors with generic interface implementation and extends LINQ filtering to support OR operations, negation, and inequality operators. The implementation adds type-safe model classes and enhanced expression tree analysis capabilities.
Implementation Details
Core Changes
Enhanced LINQ Expression Support
Code Examples
Before (Legacy Interface)
After (Generic Interface)
Implementation Benefits
Interface Modernization
Enhanced Filtering Capabilities
Developer Experience
Validation Results
Build Verification
dotnet build --configuration Release --interactiveTest Results
Full Test Suite:
Core Unit Tests:
dotnet test src\SemanticKernel.UnitTests\SemanticKernel.UnitTests.csproj --configuration ReleaseTest Failure Analysis
The 1,361 test failures are infrastructure/configuration issues, not code defects:
These failures are expected in development environments without external API configurations.
Code Quality
dotnet format SK-dotnet.slnxFiles Modified
Breaking Changes
None. All existing LINQ expressions continue to work unchanged with enhanced error message generation.
Multi-PR Context
This is PR 5 of 6 in the structured implementation approach for Issue #10456. This PR completes the modernization of remaining text search connectors with enhanced LINQ expression capabilities while maintaining full backward compatibility.