Skip to content

Commit 42e0b2e

Browse files
authored
.Net: Net: Address roji/westey review feedback round 2 on text search connectors (#10456) (#13611)
Addresses second round of review feedback from [@roji](#13384) and @westey-m on PR #13384 (`feature-text-search-linq`). This round focuses on code quality improvements: adding `[Obsolete]` attributes to legacy types, consolidating expression processing with pattern matching, and removing dead code.
1 parent 6d91511 commit 42e0b2e

22 files changed

Lines changed: 269 additions & 439 deletions

File tree

dotnet/samples/Concepts/RAG/Bing_RagWithTextSearch.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
// Copyright (c) Microsoft. All rights reserved.
2+
23
using Microsoft.SemanticKernel;
34
using Microsoft.SemanticKernel.Data;
45
using Microsoft.SemanticKernel.Plugins.Web.Bing;
@@ -133,6 +134,7 @@ Include citations to and the date of the relevant information where it is refere
133134
));
134135
}
135136

137+
#pragma warning disable CS0618 // Suppress obsolete warnings for legacy TextSearchOptions/TextSearchFilter usage
136138
/// <summary>
137139
/// Show how to create a default <see cref="KernelPlugin"/> from an <see cref="ITextSearch"/> and use it to
138140
/// add grounding context to a Handlebars prompt that include full web pages.
@@ -183,4 +185,5 @@ Include citations to the relevant information where it is referenced in the resp
183185
promptTemplateFactory: promptTemplateFactory
184186
));
185187
}
188+
#pragma warning restore CS0618
186189
}

dotnet/samples/Concepts/Search/Bing_FunctionCallingWithTextSearch.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ public async Task FunctionCallingWithBingTextSearchIncludingCitationsAsync()
6666
Console.WriteLine(await kernel.InvokePromptAsync("What is the Semantic Kernel? Include citations to the relevant information where it is referenced in the response.", arguments));
6767
}
6868

69+
#pragma warning disable CS0618 // Suppress obsolete warnings for legacy TextSearchOptions/TextSearchFilter usage
6970
/// <summary>
7071
/// Show how to create a default <see cref="KernelPlugin"/> from an <see cref="BingTextSearch"/> and use it with
7172
/// function calling to have the LLM include grounding context from the Microsoft Dev Blogs site in it's response.
@@ -143,4 +144,5 @@ private static KernelFunction CreateSearchBySite(BingTextSearch textSearch, Text
143144

144145
return textSearch.CreateSearch(options);
145146
}
147+
#pragma warning restore CS0618
146148
}

dotnet/samples/Concepts/Search/Bing_TextSearch.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ namespace Search;
1111
/// </summary>
1212
public class Bing_TextSearch(ITestOutputHelper output) : BaseTest(output)
1313
{
14+
#pragma warning disable CS0618 // Suppress obsolete warnings for legacy TextSearchOptions/TextSearchFilter usage
1415
/// <summary>
1516
/// Show how to create a <see cref="BingTextSearch"/> and use it to perform a text search.
1617
/// </summary>
@@ -118,6 +119,7 @@ public async Task UsingBingTextSearchWithASiteFilterAsync()
118119
WriteHorizontalRule();
119120
}
120121
}
122+
#pragma warning restore CS0618
121123

122124
/// <summary>
123125
/// Show how to use enhanced LINQ filtering with BingTextSearch for type-safe searches.

dotnet/samples/Concepts/Search/Google_TextSearch.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ namespace Search;
1212
/// </summary>
1313
public class Google_TextSearch(ITestOutputHelper output) : BaseTest(output)
1414
{
15+
#pragma warning disable CS0618 // Suppress obsolete warnings for legacy TextSearchOptions/TextSearchFilter usage
1516
/// <summary>
1617
/// Show how to create a <see cref="GoogleTextSearch"/> and use it to perform a text search.
1718
/// </summary>
@@ -106,6 +107,7 @@ public async Task UsingGoogleTextSearchWithASiteSearchFilterAsync()
106107
Console.WriteLine(new string('-', HorizontalRuleLength));
107108
}
108109
}
110+
#pragma warning restore CS0618
109111

110112
/// <summary>
111113
/// Show how to use enhanced LINQ filtering with GoogleTextSearch including Contains, NOT, FileType, and compound AND expressions.

dotnet/samples/Concepts/Search/Tavily_TextSearch.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ namespace Search;
1111
/// </summary>
1212
public class Tavily_TextSearch(ITestOutputHelper output) : BaseTest(output)
1313
{
14+
#pragma warning disable CS0618 // Suppress obsolete warnings for legacy TextSearchOptions/TextSearchFilter usage
1415
/// <summary>
1516
/// Show how to create a <see cref="TavilyTextSearch"/> and use it to perform a text search.
1617
/// </summary>
@@ -181,6 +182,7 @@ public async Task UsingTavilyTextSearchWithAnIncludeDomainFilterAsync()
181182
WriteHorizontalRule();
182183
}
183184
}
185+
#pragma warning restore CS0618
184186

185187
/// <summary>
186188
/// Show how to use enhanced LINQ filtering with TavilyTextSearch for type-safe searches with Title.Contains() support.

dotnet/samples/GettingStartedWithTextSearch/Step3_Search_With_FunctionCalling.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
// Copyright (c) Microsoft. All rights reserved.
2+
23
using System.Text.Json;
34
using Microsoft.Extensions.DependencyInjection;
45
using Microsoft.SemanticKernel;
@@ -69,6 +70,7 @@ public async Task FunctionCallingWithBingTextSearchIncludingCitationsAsync()
6970
Console.WriteLine(await kernel.InvokePromptAsync("What is the Semantic Kernel? Include citations to the relevant information where it is referenced in the response.", arguments));
7071
}
7172

73+
#pragma warning disable CS0618 // Suppress obsolete warnings for legacy TextSearchOptions/TextSearchFilter usage
7274
/// <summary>
7375
/// Show how to create a default <see cref="KernelPlugin"/> from an <see cref="BingTextSearch"/> and use it with
7476
/// function calling to have the LLM include grounding context from the Microsoft Dev Blogs site in it's response.
@@ -159,5 +161,6 @@ private static KernelFunction CreateSearchBySite(BingTextSearch textSearch, Text
159161

160162
return textSearch.CreateSearch(options);
161163
}
164+
#pragma warning restore CS0618
162165
#endregion
163166
}

dotnet/src/IntegrationTests/Agents/CommonInterfaceConformance/AgentWithTextSearchProviderConformance/AgentWithTextSearchProvider.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
// Copyright (c) Microsoft. All rights reserved.
22

3+
#pragma warning disable CS0618 // Obsolete TextSearchOptions/TextSearchFilter
4+
35
using System;
46
using System.Linq;
57
using System.Threading;
@@ -41,9 +43,7 @@ public abstract class AgentWithTextSearchProvider<TFixture>(Func<TFixture> creat
4143
public async Task TextSearchBehaviorStateIsUsedByAgentInternalAsync(string question, string expectedResult, params string[] ragResults)
4244
{
4345
// Arrange
44-
#pragma warning disable CS0618 // ITextSearch is obsolete - Testing legacy interface
4546
var mockTextSearch = new Mock<ITextSearch>();
46-
#pragma warning restore CS0618
4747
mockTextSearch.Setup(x => x.GetTextSearchResultsAsync(
4848
It.IsAny<string>(),
4949
It.IsAny<TextSearchOptions>(),

dotnet/src/Plugins/Plugins.Web/Bing/BingTextSearch.cs

Lines changed: 36 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
// Copyright (c) Microsoft. All rights reserved.
22

3-
#pragma warning disable CS0618 // Non-generic ITextSearch is obsolete - provides backward compatibility during Phase 2 LINQ migration
4-
53
using System;
64
using System.Collections.Generic;
75
using System.Linq;
@@ -23,7 +21,7 @@ namespace Microsoft.SemanticKernel.Plugins.Web.Bing;
2321
/// <summary>
2422
/// A Bing Text Search implementation that can be used to perform searches using the Bing Web Search API.
2523
/// </summary>
26-
#pragma warning disable CS0618 // ITextSearch is obsolete - this class provides backward compatibility
24+
#pragma warning disable CS0618 // ITextSearch is obsolete
2725
public sealed class BingTextSearch : ITextSearch, ITextSearch<BingWebPage>
2826
#pragma warning restore CS0618
2927
{
@@ -47,6 +45,10 @@ public BingTextSearch(string apiKey, BingTextSearchOptions? options = null)
4745
this._options = options;
4846
}
4947

48+
#pragma warning disable CS0618 // Obsolete ITextSearch, TextSearchOptions, TextSearchFilter, FilterClause
49+
50+
#region Legacy ITextSearch Implementation
51+
5052
/// <inheritdoc/>
5153
public async Task<KernelSearchResults<string>> SearchAsync(string query, TextSearchOptions? searchOptions = null, CancellationToken cancellationToken = default)
5254
{
@@ -83,6 +85,10 @@ public async Task<KernelSearchResults<object>> GetSearchResultsAsync(string quer
8385
return new KernelSearchResults<object>(this.GetResultsAsWebPageAsync(searchResponse, cancellationToken), totalCount, GetResultsMetadata(searchResponse));
8486
}
8587

88+
#endregion
89+
90+
#pragma warning restore CS0618
91+
8692
/// <inheritdoc/>
8793
async Task<KernelSearchResults<string>> ITextSearch<BingWebPage>.SearchAsync(string query, TextSearchOptions<BingWebPage>? searchOptions, CancellationToken cancellationToken)
8894
{
@@ -172,67 +178,48 @@ private static void ProcessExpression(Expression expression, List<(string FieldN
172178
{
173179
switch (expression)
174180
{
175-
case BinaryExpression binaryExpr when binaryExpr.NodeType == ExpressionType.AndAlso:
181+
case BinaryExpression { NodeType: ExpressionType.AndAlso } andExpr:
176182
// Handle AND: page => page.Language == "en" && page.Name.Contains("AI")
177-
ProcessExpression(binaryExpr.Left, filters);
178-
ProcessExpression(binaryExpr.Right, filters);
183+
ProcessExpression(andExpr.Left, filters);
184+
ProcessExpression(andExpr.Right, filters);
179185
break;
180186

181-
case BinaryExpression binaryExpr when binaryExpr.NodeType == ExpressionType.OrElse:
187+
case BinaryExpression { NodeType: ExpressionType.OrElse }:
182188
// Handle OR: Currently not directly supported by TextSearchFilter
183189
// Bing API supports OR via multiple queries, but TextSearchFilter doesn't expose this
184190
throw new NotSupportedException(
185191
"Logical OR (||) is not supported by Bing Text Search filters. " +
186192
"Consider splitting into multiple search queries.");
187193

188-
case UnaryExpression unaryExpr when unaryExpr.NodeType == ExpressionType.Not:
194+
case UnaryExpression { NodeType: ExpressionType.Not }:
189195
// Handle NOT: page => !page.Language.Equals("en")
190196
throw new NotSupportedException(
191197
"Logical NOT (!) is not directly supported by Bing Text Search advanced operators. " +
192198
"Consider restructuring your filter to use positive conditions.");
193199

194-
case BinaryExpression binaryExpr when binaryExpr.NodeType == ExpressionType.Equal:
200+
case BinaryExpression { NodeType: ExpressionType.Equal } binaryExpr:
195201
// Handle equality: page => page.Language == "en"
196202
ProcessEqualityExpression(binaryExpr, filters, isNegated: false);
197203
break;
198204

199-
case BinaryExpression binaryExpr when binaryExpr.NodeType == ExpressionType.NotEqual:
205+
case BinaryExpression { NodeType: ExpressionType.NotEqual } binaryExpr:
200206
// Handle inequality: page => page.Language != "en"
201207
// Implemented via Bing's negation syntax (e.g., -language:en)
202208
ProcessEqualityExpression(binaryExpr, filters, isNegated: true);
203209
break;
204210

205-
case MethodCallExpression methodExpr when methodExpr.Method.Name == "Contains":
206-
// Distinguish between instance method (String.Contains) and static method (Enumerable/MemoryExtensions.Contains)
207-
if (methodExpr.Object is MemberExpression)
208-
{
209-
// Instance method: page.Name.Contains("value") - SUPPORTED
210-
ProcessContainsExpression(methodExpr, filters);
211-
}
212-
else if (methodExpr.Object == null)
213-
{
214-
// Static method: could be Enumerable.Contains (C# 13-) or MemoryExtensions.Contains (C# 14+)
215-
// Bing API doesn't support OR logic, so collection Contains patterns are not supported
216-
if (methodExpr.Method.DeclaringType == typeof(Enumerable) ||
217-
(methodExpr.Method.DeclaringType == typeof(MemoryExtensions) && IsMemoryExtensionsContains(methodExpr)))
218-
{
219-
throw new NotSupportedException(
220-
"Collection Contains filters (e.g., array.Contains(page.Property)) are not supported by Bing Search API. " +
221-
"Bing's advanced search operators do not support OR logic across multiple values. " +
222-
"Supported pattern: Property.Contains(\"value\") for string properties like Name, Snippet, or Url. " +
223-
"For multiple value matching, consider alternative approaches or use a different search provider.");
224-
}
225-
226-
throw new NotSupportedException(
227-
$"Contains() method from {methodExpr.Method.DeclaringType?.Name} is not supported.");
228-
}
229-
else
230-
{
231-
throw new NotSupportedException(
232-
"Contains() must be called on a property (e.g., page.Name.Contains(\"value\")).");
233-
}
211+
case MethodCallExpression { Method.Name: "Contains", Object: MemberExpression } methodExpr:
212+
// Instance method: page.Name.Contains("value") - SUPPORTED
213+
ProcessContainsExpression(methodExpr, filters);
234214
break;
235215

216+
case MethodCallExpression { Method.Name: "Contains", Object: null }:
217+
// Static method: collection.Contains(page.Property) - NOT SUPPORTED
218+
throw new NotSupportedException(
219+
"Collection Contains filters (e.g., collection.Contains(page.Property)) are not supported by Bing Search API. " +
220+
"Bing API doesn't support OR logic across multiple values for a single field. " +
221+
"Consider using multiple separate queries instead.");
222+
236223
default:
237224
throw new NotSupportedException(
238225
$"Expression type '{expression.NodeType}' is not supported for Bing API filters. " +
@@ -360,32 +347,13 @@ private static void ProcessContainsExpression(MethodCallExpression methodExpr, L
360347
}
361348
}
362349

363-
/// <summary>
364-
/// Determines if a MethodCallExpression is a MemoryExtensions.Contains call (C# 14 "first-class spans" feature).
365-
/// </summary>
366-
/// <param name="methodExpr">The method call expression to check.</param>
367-
/// <returns>True if this is a MemoryExtensions.Contains call with supported parameters; otherwise false.</returns>
368-
private static bool IsMemoryExtensionsContains(MethodCallExpression methodExpr)
369-
{
370-
// MemoryExtensions.Contains has 2-3 parameters:
371-
// - Contains<T>(ReadOnlySpan<T> span, T value)
372-
// - Contains<T>(ReadOnlySpan<T> span, T value, IEqualityComparer<T>? comparer)
373-
// We only support when comparer is null or omitted
374-
return methodExpr.Method.Name == nameof(MemoryExtensions.Contains) &&
375-
methodExpr.Arguments.Count >= 2 &&
376-
methodExpr.Arguments.Count <= 3 &&
377-
(methodExpr.Arguments.Count == 2 ||
378-
(methodExpr.Arguments.Count == 3 && methodExpr.Arguments[2] is ConstantExpression { Value: null }));
379-
}
380-
381350
/// <summary>
382351
/// Maps BingWebPage property names to Bing API filter field names for equality operations.
383352
/// </summary>
384353
/// <param name="propertyName">The BingWebPage property name.</param>
385354
/// <returns>The corresponding Bing API filter name, or null if not mappable.</returns>
386-
private static string? MapPropertyToBingFilter(string propertyName)
387-
{
388-
return propertyName.ToUpperInvariant() switch
355+
private static string? MapPropertyToBingFilter(string propertyName) =>
356+
propertyName.ToUpperInvariant() switch
389357
{
390358
// Map BingWebPage properties to Bing API equivalents
391359
"LANGUAGE" => "language", // Maps to advanced search
@@ -401,16 +369,14 @@ private static bool IsMemoryExtensionsContains(MethodCallExpression methodExpr)
401369

402370
_ => null // Property not mappable to Bing filters
403371
};
404-
}
405372

406373
/// <summary>
407374
/// Maps BingWebPage property names to Bing API advanced search operators for Contains operations.
408375
/// </summary>
409376
/// <param name="propertyName">The BingWebPage property name.</param>
410377
/// <returns>The corresponding Bing advanced search operator, or null if not mappable.</returns>
411-
private static string? MapPropertyToContainsFilter(string propertyName)
412-
{
413-
return propertyName.ToUpperInvariant() switch
378+
private static string? MapPropertyToContainsFilter(string propertyName) =>
379+
propertyName.ToUpperInvariant() switch
414380
{
415381
// Map properties to Bing's contains-style operators
416382
"NAME" => "intitle", // intitle:"search term" - title contains
@@ -420,7 +386,6 @@ private static bool IsMemoryExtensionsContains(MethodCallExpression methodExpr)
420386

421387
_ => null // Property not mappable to Contains-style filters
422388
};
423-
}
424389

425390
/// <summary>
426391
/// Execute a Bing search query and return the results.
@@ -594,12 +559,12 @@ public TextSearchResult MapFromResultToTextSearchResult(object result)
594559
}
595560
}
596561

597-
#pragma warning disable CS0618 // FilterClause is obsolete - backward compatibility shim for legacy ITextSearch
598562
/// <summary>
599563
/// Extracts filter key-value pairs from a legacy <see cref="TextSearchFilter"/>.
600564
/// This shim converts the obsolete FilterClause-based format to the internal (FieldName, Value) list.
601565
/// It will be removed when the legacy ITextSearch interface is retired.
602566
/// </summary>
567+
#pragma warning disable CS0618 // Obsolete TextSearchFilter, FilterClause
603568
private static List<(string FieldName, object Value)> ExtractFiltersFromLegacy(TextSearchFilter? filter)
604569
{
605570
var filters = new List<(string FieldName, object Value)>();
@@ -611,6 +576,11 @@ public TextSearchResult MapFromResultToTextSearchResult(object result)
611576
{
612577
filters.Add((eq.FieldName, eq.Value));
613578
}
579+
else
580+
{
581+
throw new NotSupportedException(
582+
$"Filter clause type '{clause.GetType().Name}' is not supported by Bing Text Search. Only EqualToFilterClause is supported.");
583+
}
614584
}
615585
}
616586
return filters;

0 commit comments

Comments
 (0)