mirror of
https://github.com/microsoft/agent-framework.git
synced 2026-06-16 21:04:09 +08:00
.NET: Fix filter combine logic for ChatHistoryMemoryProvider (#4501)
* Fix filter combine logic for ChatHistoryMemoryProvider * Replace var with explicit types in filter building code and test Address PR review nit: use explicit types instead of var for better readability in the filter-building logic and the new combined filter compilation test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix style issues --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
d8d6ac1c59
commit
f7e4143c61
@@ -350,36 +350,38 @@ public sealed class ChatHistoryMemoryProvider : MessageAIContextProvider, IDispo
|
||||
string? userId = searchScope.UserId;
|
||||
string? sessionId = searchScope.SessionId;
|
||||
|
||||
Expression<Func<Dictionary<string, object?>, bool>>? filter = null;
|
||||
// Build a combined filter using a single shared parameter to avoid expression tree
|
||||
// scoping issues when multiple filters are combined with AndAlso.
|
||||
ParameterExpression parameter = Expression.Parameter(typeof(Dictionary<string, object?>), "x");
|
||||
Expression? filterBody = null;
|
||||
|
||||
if (applicationId != null)
|
||||
{
|
||||
filter = x => (string?)x[ApplicationIdField] == applicationId;
|
||||
filterBody = RebindFilterBody(x => (string?)x[ApplicationIdField] == applicationId, parameter);
|
||||
}
|
||||
|
||||
if (agentId != null)
|
||||
{
|
||||
Expression<Func<Dictionary<string, object?>, bool>> agentIdFilter = x => (string?)x[AgentIdField] == agentId;
|
||||
filter = filter == null ? agentIdFilter : Expression.Lambda<Func<Dictionary<string, object?>, bool>>(
|
||||
Expression.AndAlso(filter.Body, agentIdFilter.Body),
|
||||
filter.Parameters);
|
||||
Expression body = RebindFilterBody(x => (string?)x[AgentIdField] == agentId, parameter);
|
||||
filterBody = filterBody == null ? body : Expression.AndAlso(filterBody, body);
|
||||
}
|
||||
|
||||
if (userId != null)
|
||||
{
|
||||
Expression<Func<Dictionary<string, object?>, bool>> userIdFilter = x => (string?)x[UserIdField] == userId;
|
||||
filter = filter == null ? userIdFilter : Expression.Lambda<Func<Dictionary<string, object?>, bool>>(
|
||||
Expression.AndAlso(filter.Body, userIdFilter.Body),
|
||||
filter.Parameters);
|
||||
Expression body = RebindFilterBody(x => (string?)x[UserIdField] == userId, parameter);
|
||||
filterBody = filterBody == null ? body : Expression.AndAlso(filterBody, body);
|
||||
}
|
||||
|
||||
if (sessionId != null)
|
||||
{
|
||||
Expression<Func<Dictionary<string, object?>, bool>> sessionIdFilter = x => (string?)x[SessionIdField] == sessionId;
|
||||
filter = filter == null ? sessionIdFilter : Expression.Lambda<Func<Dictionary<string, object?>, bool>>(
|
||||
Expression.AndAlso(filter.Body, sessionIdFilter.Body),
|
||||
filter.Parameters);
|
||||
Expression body = RebindFilterBody(x => (string?)x[SessionIdField] == sessionId, parameter);
|
||||
filterBody = filterBody == null ? body : Expression.AndAlso(filterBody, body);
|
||||
}
|
||||
|
||||
Expression<Func<Dictionary<string, object?>, bool>>? filter = filterBody != null
|
||||
? Expression.Lambda<Func<Dictionary<string, object?>, bool>>(filterBody, parameter)
|
||||
: null;
|
||||
|
||||
// Use search to find relevant messages
|
||||
var searchResults = collection.SearchAsync(
|
||||
queryText,
|
||||
@@ -467,6 +469,27 @@ public sealed class ChatHistoryMemoryProvider : MessageAIContextProvider, IDispo
|
||||
|
||||
private string? SanitizeLogData(string? data) => this._enableSensitiveTelemetryData ? data : "<redacted>";
|
||||
|
||||
/// <summary>
|
||||
/// Rebinds a filter expression's body to use the specified shared parameter,
|
||||
/// replacing the original lambda parameter so that multiple filters can be safely
|
||||
/// combined with <see cref="Expression.AndAlso(Expression, Expression)"/>.
|
||||
/// </summary>
|
||||
private static Expression RebindFilterBody(
|
||||
Expression<Func<Dictionary<string, object?>, bool>> filter,
|
||||
ParameterExpression sharedParameter)
|
||||
{
|
||||
return new ParameterReplacer(filter.Parameters[0], sharedParameter).Visit(filter.Body);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// An <see cref="ExpressionVisitor"/> that replaces one <see cref="ParameterExpression"/> with another.
|
||||
/// </summary>
|
||||
private sealed class ParameterReplacer(ParameterExpression original, ParameterExpression replacement) : ExpressionVisitor
|
||||
{
|
||||
protected override Expression VisitParameter(ParameterExpression node)
|
||||
=> node == original ? replacement : base.VisitParameter(node);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Represents the state of a <see cref="ChatHistoryMemoryProvider"/> stored in the <see cref="AgentSession.StateBag"/>.
|
||||
/// </summary>
|
||||
|
||||
@@ -454,6 +454,77 @@ public class ChatHistoryMemoryProviderTests
|
||||
Times.Once);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task InvokedAsync_CombinedFilterCanBeCompiled_WhenMultipleScopeFiltersProvidedAsync()
|
||||
{
|
||||
// Arrange
|
||||
// This test reproduces a bug where combining multiple scope filters
|
||||
// (e.g. userId + sessionId) produces an expression tree with dangling
|
||||
// ParameterExpression references that fails at compile time.
|
||||
ChatHistoryMemoryProviderOptions providerOptions = new()
|
||||
{
|
||||
SearchTime = ChatHistoryMemoryProviderOptions.SearchBehavior.BeforeAIInvoke,
|
||||
MaxResults = 2,
|
||||
ContextPrompt = "Here is the relevant chat history:\n"
|
||||
};
|
||||
|
||||
ChatHistoryMemoryProviderScope searchScope = new()
|
||||
{
|
||||
ApplicationId = "app1",
|
||||
AgentId = "agent1",
|
||||
SessionId = "session1",
|
||||
UserId = "user1"
|
||||
};
|
||||
|
||||
System.Linq.Expressions.Expression<Func<Dictionary<string, object?>, bool>>? capturedFilter = null;
|
||||
|
||||
this._vectorStoreCollectionMock
|
||||
.Setup(c => c.SearchAsync(
|
||||
It.IsAny<string>(),
|
||||
It.IsAny<int>(),
|
||||
It.IsAny<VectorSearchOptions<Dictionary<string, object?>>>(),
|
||||
It.IsAny<CancellationToken>()))
|
||||
.Callback((string query, int maxResults, VectorSearchOptions<Dictionary<string, object?>> options, CancellationToken ct) =>
|
||||
capturedFilter = options.Filter)
|
||||
.Returns(ToAsyncEnumerableAsync(new List<VectorSearchResult<Dictionary<string, object?>>>()));
|
||||
|
||||
ChatHistoryMemoryProvider provider = new(
|
||||
this._vectorStoreMock.Object,
|
||||
TestCollectionName,
|
||||
1,
|
||||
_ => new ChatHistoryMemoryProvider.State(searchScope, searchScope),
|
||||
options: providerOptions);
|
||||
|
||||
ChatMessage requestMsg = new(ChatRole.User, "requesting relevant history");
|
||||
AIContextProvider.InvokingContext invokingContext = new(s_mockAgent, new TestAgentSession(), new AIContext { Messages = new List<ChatMessage> { requestMsg } });
|
||||
|
||||
// Act
|
||||
await provider.InvokingAsync(invokingContext, CancellationToken.None);
|
||||
|
||||
// Assert - The filter must be compilable and executable without expression tree scoping errors
|
||||
Assert.NotNull(capturedFilter);
|
||||
Func<Dictionary<string, object?>, bool> compiledFilter = capturedFilter!.Compile();
|
||||
|
||||
Dictionary<string, object?> matchingRecord = new()
|
||||
{
|
||||
["ApplicationId"] = "app1",
|
||||
["AgentId"] = "agent1",
|
||||
["SessionId"] = "session1",
|
||||
["UserId"] = "user1"
|
||||
};
|
||||
|
||||
Dictionary<string, object?> nonMatchingRecord = new()
|
||||
{
|
||||
["ApplicationId"] = "app1",
|
||||
["AgentId"] = "agent1",
|
||||
["SessionId"] = "other-session",
|
||||
["UserId"] = "user1"
|
||||
};
|
||||
|
||||
Assert.True(compiledFilter(matchingRecord));
|
||||
Assert.False(compiledFilter(nonMatchingRecord));
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[InlineData(false, false, 2)]
|
||||
[InlineData(true, false, 2)]
|
||||
|
||||
Reference in New Issue
Block a user