fix(dotnet/connectors/openai): deduplicate top-level JSON keys when using ExtraBody - #14264
fix(dotnet/connectors/openai): deduplicate top-level JSON keys when using ExtraBody#14264Nithin (nithin42) wants to merge 6 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR addresses OpenAI/Azure OpenAI request failures caused by duplicate top-level JSON keys when OpenAIPromptExecutionSettings.ExtraBody is applied via System.ClientModel’s JsonPatch, by adding a pipeline-level sanitization step that rewrites outgoing JSON to ensure unique top-level property names (last-write-wins).
Changes:
- Added a per-call
DeduplicateJsonKeysPipelinePolicyinClientCoreto rewrite JSON request bodies with deduplicated top-level keys. - Added unit tests to assert that
toolsand other top-level keys (e.g.,temperature) are not duplicated in the final request payload.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| dotnet/src/Connectors/Connectors.OpenAI/Core/ClientCore.cs | Registers and implements an HTTP pipeline policy that deduplicates top-level JSON object keys before sending requests. |
| dotnet/src/Connectors/Connectors.OpenAI.UnitTests/Services/OpenAIChatCompletionExtraBodyTests.cs | Adds tests validating that ExtraBody patching does not produce duplicate top-level keys in the request body. |
Suppressed comments (2)
dotnet/src/Connectors/Connectors.OpenAI.UnitTests/Services/OpenAIChatCompletionExtraBodyTests.cs:34
ITestOutputHelperis injected and stored but never used. This introduces unnecessary dependencies and can trigger compiler warnings (assigned but never used). Either use it (e.g., to log the request JSON on failure) or remove it and restore the parameterless constructor.
private readonly ITestOutputHelper _output;
public OpenAIChatCompletionExtraBodyTests(ITestOutputHelper output)
{
this._output = output;
dotnet/src/Connectors/Connectors.OpenAI.UnitTests/Services/OpenAIChatCompletionExtraBodyTests.cs:16
- After removing the unused
ITestOutputHelperinjection,using Xunit.Abstractions;becomes unused as well and should be removed to avoid warnings.
using Xunit;
using Xunit.Abstractions;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using var memoryStream = new System.IO.MemoryStream(); | ||
| message.Request.Content.WriteTo(memoryStream, default); | ||
| byte[] bytes = memoryStream.ToArray(); | ||
| if (bytes.Length == 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| string rawJson = System.Text.Encoding.UTF8.GetString(bytes); | ||
| if (!rawJson.StartsWith('{')) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| string cleanJson = DeduplicateTopLevelJsonKeys(rawJson); | ||
| if (!string.Equals(rawJson, cleanJson, StringComparison.Ordinal)) | ||
| { | ||
| message.Request.Content = System.ClientModel.BinaryContent.Create(BinaryData.FromString(cleanJson)); | ||
| } |
| using System; | ||
| using System.Collections.Generic; | ||
| using System.IO; | ||
| using System.Net; | ||
| using System.Net.Http; |
| var dictionary = new Dictionary<string, System.Text.Json.JsonElement>(StringComparer.Ordinal); | ||
| foreach (var prop in root.EnumerateObject()) | ||
| { | ||
| dictionary[prop.Name] = prop.Value.Clone(); | ||
| } | ||
|
|
||
| using var stream = new System.IO.MemoryStream(); |
@microsoft-github-policy-service agree |
…y and add exception safety
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
dotnet/src/Connectors/Connectors.OpenAI.UnitTests/Services/OpenAIChatCompletionExtraBodyTests.cs:5
using System.IO;appears unused in this test file (the onlyMemoryStreamusage is fully-qualified asSystem.IO.MemoryStream), which can fail builds that treat unused usings as warnings-as-errors. Remove the unused using or use the unqualified type name.
using System.IO;
dotnet/src/Connectors/Connectors.OpenAI.UnitTests/Services/OpenAIChatCompletionExtraBodyTests.cs:251
- Same as above: regex-counting can match nested occurrences or string values. Prefer counting top-level property names via
Utf8JsonReaderto ensure there is exactly one top-leveltemperaturekey.
var jsonString = Encoding.UTF8.GetString(this._messageHandlerStub.RequestContent!);
int tempKeyCount = System.Text.RegularExpressions.Regex.Matches(jsonString, "\"temperature\"\\s*:").Count;
Assert.Equal(1, tempKeyCount);
dotnet/src/Connectors/Connectors.OpenAI.UnitTests/Services/OpenAIChatCompletionExtraBodyTests.cs:228
- Counting occurrences with a regex can produce false positives (e.g., if the string "tools" appears inside a string value) and doesn’t ensure the match is a top-level property. Using
Utf8JsonReaderto countPropertyNametokens atCurrentDepth == 1makes the assertion precise and resilient.
This issue also appears on line 249 of the same file.
var jsonString = Encoding.UTF8.GetString(this._messageHandlerStub.RequestContent!);
int toolsKeyCount = System.Text.RegularExpressions.Regex.Matches(jsonString, "\"tools\"\\s*:").Count;
Assert.Equal(1, toolsKeyCount);
dotnet/src/Connectors/Connectors.OpenAI/Core/ClientCore.cs:307
- This policy always buffers the full request body into a new byte[] via
ToArray()even when no changes are made.MemoryStream.TryGetBuffer(and checkingLengthfirst) avoids an extra allocation/copy on every request and reduces per-call overhead.
using var memoryStream = new System.IO.MemoryStream();
message.Request.Content.WriteTo(memoryStream, default);
byte[] bytes = memoryStream.ToArray();
… and clean unused usings
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
dotnet/src/Connectors/Connectors.OpenAI.UnitTests/Services/OpenAIChatCompletionExtraBodyTests.cs:252
- This test currently only checks that
temperatureappears once, but it should also validate the surviving value is the patched value (0.5) to confirm deduplication keeps the correct field/value pair.
using var doc = JsonDocument.Parse(this._messageHandlerStub.RequestContent!);
int tempKeyCount = doc.RootElement.EnumerateObject().Count(p => p.NameEquals("temperature"));
Assert.Equal(1, tempKeyCount);
}
dotnet/src/Connectors/Connectors.OpenAI/Core/ClientCore.cs:303
SanitizeMessageContentbuffers every outgoing request body into memory viaContent.WriteTo(...)before even checking whether the payload is JSON. This adds avoidable overhead for non-JSON requests (e.g., multipart uploads for audio/files) and can significantly increase memory use for large payloads.
Consider short-circuiting based on the request Content-Type header before reading the body, and only running the deduplication logic for application/json payloads.
if (message.Request.Content is null)
{
return;
}
using var memoryStream = new System.IO.MemoryStream();
message.Request.Content.WriteTo(memoryStream, default);
byte[] bytes = memoryStream.ToArray();
dotnet/src/Connectors/Connectors.OpenAI.UnitTests/Services/OpenAIChatCompletionExtraBodyTests.cs:229
- The new tests only assert that the JSON DOM contains a single
tools/temperatureproperty, but they don't verify that the remaining property is the expected value after deduplication. Adding a value assertion helps ensure the deduplication step preserves the intended last-write-wins semantics (e.g.,toolscontainsweb_search, andtemperatureis0.5).
This issue also appears on line 249 of the same file.
using var doc = JsonDocument.Parse(this._messageHandlerStub.RequestContent!);
int toolsKeyCount = doc.RootElement.EnumerateObject().Count(p => p.NameEquals("tools"));
Assert.Equal(1, toolsKeyCount);
}
…sert surviving patched values in tests
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
dotnet/src/Connectors/Connectors.OpenAI/Core/ClientCore.cs:316
SanitizeMessageContentallocates an extra byte[] viamemoryStream.ToArray()on every request. Since this policy runs per-call, avoiding the copy can reduce allocations for large chat payloads.
using var memoryStream = new System.IO.MemoryStream();
message.Request.Content.WriteTo(memoryStream, default);
byte[] bytes = memoryStream.ToArray();
if (bytes.Length == 0)
{
return;
}
string rawJson = System.Text.Encoding.UTF8.GetString(bytes).TrimStart('\uFEFF', ' ', '\t', '\r', '\n');
dotnet/src/Connectors/Connectors.OpenAI/Core/ClientCore.cs:372
DeduplicateTopLevelJsonKeyscallsstream.ToArray(), which creates another full copy of the sanitized payload. You can avoid the extra allocation by reading directly from theMemoryStreambuffer.
return System.Text.Encoding.UTF8.GetString(stream.ToArray());
dotnet/src/Connectors/Connectors.OpenAI.UnitTests/Services/OpenAIChatCompletionExtraBodyTests.cs:210
- The new regression coverage validates deduplication for literal top-level keys (e.g., "tools" and "temperature"), but doesn’t cover the JSONPath-style keys called out in the issue/PR description (e.g., "$.tools" and "$.tools[0].type"). Adding at least one test for those forms would better protect the scenario that originally broke.
public async Task ExtraBodyToolsDoesNotEmitDuplicateToolsKeyInRequestBodyAsync()
…d add JSONPath regression tests
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
dotnet/src/Connectors/Connectors.OpenAI/Core/ClientCore.cs:325
- SanitizeMessageContent UTF8-decodes the entire request payload into a string before confirming it’s a JSON object (the
{check happens afterEncoding.UTF8.GetString(...)). This can create large, unnecessary allocations for non-JSON/binary requests, and does extra work even when the body isn’t JSON. Consider peeking the first non-whitespace UTF-8 byte (and BOM) in the buffered bytes before decoding to a string.
int offset = memoryStream.TryGetBuffer(out buffer) ? buffer.Offset : 0;
int count = (int)memoryStream.Length;
string rawJson = System.Text.Encoding.UTF8.GetString(bytes, offset, count).TrimStart('\uFEFF', ' ', '\t', '\r', '\n');
if (!rawJson.StartsWith('{'))
dotnet/src/Connectors/Connectors.OpenAI/Core/ClientCore.cs:358
DeduplicateTopLevelJsonKeysclones every property value (prop.Value.Clone()) even though theJsonDocumentstays alive until after writing the deduplicated output. The clones add avoidable allocations/CPU on every request that passes through this policy. You can keepJsonElementvalues backed by the currentJsonDocumentwithout cloning here.
{
hadDuplicates |= dictionary.ContainsKey(prop.Name);
dictionary[prop.Name] = prop.Value.Clone();
Fixes #14156
Motivation and Context
When developers use
OpenAIPromptExecutionSettings.ExtraBodyto pass custom or preview parameters to OpenAI/Azure OpenAI models (such as web search tools viatools,temperature,reasoning_effort, or custom vendor fields),System.ClientModel'sJsonPatchappends patched properties onto the outgoing JSON request body.Because
ChatCompletionOptions's default JSON model serializer already writes built-in fields (such astools: []),JsonPatchappends a secondtoolsproperty at the end of the JSON object. This creates duplicate top-level keys in the serialized HTTP request payload (e.g.,{"messages":[...],"tools":[],"tools":[{"type":"web_search"}]}), causing OpenAI/Azure OpenAI API gateways to reject the request with400 Bad Request.Solution: Generic Pipeline Policy (Non-Hardcoded)
Instead of hardcoding
if (key == "tools")workarounds inOpenAIPromptExecutionSettings.cs, this PR introduces a generic, non-intrusive solution at the HTTP pipeline level:DeduplicateJsonKeysPipelinePolicy: Added a newPipelinePolicyinsideClientCore.csregistered atPipelinePosition.PerCall.tools,$.tools,$.tools[0],temperature, or vendor extensions), it deduplicates top-level object properties (last-write-wins) using standardJsonDocument/Utf8JsonWriterparsing before transmitting the request.Verification & Testing
ExtraBodyToolsDoesNotEmitDuplicateToolsKeyInRequestBodyAsyncandExtraBodyDoesNotEmitDuplicateTopLevelKeysInRequestBodyAsyncinOpenAIChatCompletionExtraBodyTests.cs.Connectors.OpenAI.UnitTestspass cleanly (dotnet test)."tools"key and valid JSON.