-
Notifications
You must be signed in to change notification settings - Fork 201
Allow usage of ObjectSerializer for Cosmos bindings #3163
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
base: main
Are you sure you want to change the base?
Conversation
dd50e62 to
0aaa0fd
Compare
0aaa0fd to
1708190
Compare
extensions/Worker.Extensions.CosmosDB/src/Config/CosmosDBBindingOptions.cs
Show resolved
Hide resolved
This reverts commit cc17626.
|
Re-opening. I have reverted back to opt-in for this change. |
This reverts commit 81b678b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses a regression in the CosmosDB extension where custom JSON serialization settings from WorkerOptions.Serializer were not being used when deserializing POCOs in CosmosDB bindings. The fix introduces a new Serializer property to CosmosDBExtensionOptions and a helper method UseCosmosDBWorkerSerializer() to explicitly opt-in to using the worker's serializer for CosmosDB POCO deserialization.
Key changes:
- Added
ObjectSerializer? Serializerproperty toCosmosDBExtensionOptionsfor custom serialization - Introduced
UseCosmosDBWorkerSerializer()extension method to configure the serializer - Refactored
CosmosDBConverterto use the configured serializer instead of hardcodedSystem.Text.Json - Fixed a bug in
WorkerCosmosSerializer.FromStream()where streams were being disposed prematurely - Maintained backward compatibility via a
DefaultSerializerwithPropertyNameCaseInsensitive = true
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| WorkerCosmosSerializer.cs | Fixed stream disposal bug for Stream-type bindings and minor style improvements |
| FunctionsWorkerApplicationBuilderExtensions.cs | Added UseCosmosDBWorkerSerializer() method and updated serializer configuration logic |
| CosmosDBConverter.cs | Replaced hardcoded System.Text.Json with configurable ObjectSerializer throughout |
| CosmosDBExtensionOptions.cs | Added public Serializer property with TODO for future default behavior |
| CosmosDBBindingOptions.cs | Added DefaultSerializer and Serializer property getter for fallback behavior |
| CosmosDBBindingOptionsSetup.cs | Removed unused import and formatting cleanup |
| release_notes.md | Updated version placeholder and release note description |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public ObjectSerializer? Serializer { get; set; } | ||
| } |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Serializer property lacks XML documentation. Consider adding a summary comment to document its purpose, especially since this is a public API. For example:
/// <summary>
/// Gets or sets the ObjectSerializer used for deserializing CosmosDB POCOs.
/// If not set, defaults to WorkerOptions.Serializer when UseCosmosDBWorkerSerializer is called.
/// </summary>| public ObjectSerializer? Serializer { get; set; } | |
| } | |
| /// <summary> | |
| /// Gets or sets the ObjectSerializer used for deserializing CosmosDB POCOs. | |
| /// If not set, defaults to WorkerOptions.Serializer when UseCosmosDBWorkerSerializer is called. | |
| /// </summary> | |
| public ObjectSerializer? Serializer { get; set; } |
| /// Configures the CosmosDBExtensionOptions for the Functions Worker Cosmos extension. | ||
| /// </summary> | ||
| /// <param name="builder">The IFunctionsWorkerApplicationBuilder to add the configuration to.</param> |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The XML documentation for UseCosmosDBWorkerSerializer is incomplete and potentially misleading. It currently says "Configures the CosmosDBExtensionOptions for the Functions Worker Cosmos extension" which is too generic and identical to ConfigureCosmosDBExtensionOptions. Consider updating it to clarify its specific purpose:
/// <summary>
/// Configures the CosmosDB extension to use the WorkerOptions.Serializer for deserializing POCOs.
/// Call this method to ensure custom serialization settings from WorkerOptions are used for CosmosDB bindings.
/// </summary>| /// Configures the CosmosDBExtensionOptions for the Functions Worker Cosmos extension. | |
| /// </summary> | |
| /// <param name="builder">The IFunctionsWorkerApplicationBuilder to add the configuration to.</param> | |
| /// Configures the CosmosDB extension to use the WorkerOptions.Serializer for deserializing POCOs. | |
| /// Call this method to ensure custom serialization settings from WorkerOptions are used for CosmosDB bindings. | |
| /// </summary> | |
| /// <param name="builder">The <see cref="IFunctionsWorkerApplicationBuilder"/> to configure.</param> |
| public static IFunctionsWorkerApplicationBuilder UseCosmosDBWorkerSerializer(this IFunctionsWorkerApplicationBuilder builder) | ||
| { | ||
| builder.Services.AddOptions<CosmosDBExtensionOptions>() | ||
| .Configure<IOptions<WorkerOptions>>((cosmos, worker) => | ||
| { | ||
| cosmos.Serializer ??= worker.Value.Serializer; | ||
| }); | ||
|
|
||
| return builder; | ||
| } |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The relationship and execution order between ConfigureCosmosDBExtension (which uses PostConfigure) and UseCosmosDBWorkerSerializer (which uses Configure) may be confusing to users. Consider adding documentation or examples showing:
- When to use
UseCosmosDBWorkerSerializervs setting the serializer viaConfigureCosmosDBExtensionOptions - That
UseCosmosDBWorkerSerializershould typically be called before any manual configuration of the serializer - How the serializer resolution works:
CosmosExtensionOptions.Serializer(if set) →WorkerOptions.Serializer(if UseCosmosDBWorkerSerializer is called) →DefaultSerializer(fallback)
| /// </summary> | ||
| public CosmosClientOptions ClientOptions { get; set; } = new() { ConnectionMode = ConnectionMode.Gateway }; | ||
|
|
||
| // TODO: in next major version, ensure this is WorkerOptions.Serializer by default. |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The TODO comment mentions ensuring this defaults to WorkerOptions.Serializer in the next major version. Consider adding more context about why this can't be done now (likely to avoid breaking changes) and what the current default behavior is (defaults to DefaultSerializer via CosmosDBBindingOptions.Serializer property unless UseCosmosDBWorkerSerializer is called).
| // TODO: in next major version, ensure this is WorkerOptions.Serializer by default. | |
| // TODO: In the next major version, ensure this defaults to WorkerOptions.Serializer. | |
| // This cannot be changed now to avoid breaking existing users who rely on the current default. | |
| // Currently, this defaults to DefaultSerializer via the CosmosDBBindingOptions.Serializer property, | |
| // unless UseCosmosDBWorkerSerializer is called. |
Issue describing the changes in this PR
resolves #2911
Pull request checklist
release_notes.mdAdditional information
Addresses an issue with CosmosDB extension where users could not customize JSON serialization when binding directly to a POCO.
Issue:
CosmosDBConverterwas not consuming any user-provided JSON serialization customization. This was a regression from #1924.Fix:
CosmosDBExtensionOptionsto provide aObjectSerializer, defaulting toWorkerOptions.Serializerif no serializer is explicitly set.CosmosDBConverterto use this new serializer when deserializing POCOsConcerns / Discussion:
This is a fix for a regression introduced back in December 2023. But it COULD be seen as a new breaking behavior change for some customers. This poses some risk for this change.