Skip to content

Fix LINQ Take() silently modifying cached QueryRequestOptions objects#5292

Draft
Copilot wants to merge 2 commits intomainfrom
copilot/fix-5225
Draft

Fix LINQ Take() silently modifying cached QueryRequestOptions objects#5292
Copilot wants to merge 2 commits intomainfrom
copilot/fix-5225

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Jul 15, 2025

Problem

The LINQ Take() method was silently modifying the original QueryRequestOptions object passed by users, causing performance issues when the same options object was cached and reused across multiple queries.

Root Cause

In CosmosQueryClientCore.ExecuteItemQueryAsync(), the code directly modified the original QueryRequestOptions.MaxItemCount property:

requestOptions.MaxItemCount = pageSize;  // This modifies the original object!

Reproduction Scenario

// User caches QueryRequestOptions for reuse
private readonly QueryRequestOptions options = new QueryRequestOptions { MaxItemCount = -1 };

public async Task<List<Post>> GetPostsAsync(string user, int? take = null) 
{
    var query = container.GetItemLinqQueryable<Item>(requestOptions: options)
        .Where(item => item.User == user);
    
    if (take is not null) 
    {
        query = query.Take(take.Value);  // This silently modifies options.MaxItemCount!
    }

    // Convert to iterator and process...
}

// Later calls reuse the same options object but now MaxItemCount is permanently changed

This resulted in:

  1. First call without Take(): Works fine with MaxItemCount = -1
  2. Call with Take(1): Works but silently sets MaxItemCount = 1
  3. Subsequent calls without Take(): Very slow because MaxItemCount = 1 forces one item per request

Solution

Modified CosmosQueryClientCore.ExecuteItemQueryAsync() to create a shallow copy of the QueryRequestOptions before modifying MaxItemCount. This ensures the original cached object remains unchanged.

Key Changes

  • Create a new QueryRequestOptions instance copying all properties from the original
  • Modify only the copy's MaxItemCount property
  • Use the copy for the downstream operation
  • Added comprehensive unit test to validate the fix

Code Changes

// Before (problematic)
requestOptions.MaxItemCount = pageSize;

// After (fixed)
QueryRequestOptions requestOptionsCopy = new QueryRequestOptions { /* copy all properties */ };
requestOptionsCopy.MaxItemCount = pageSize;
// Use requestOptionsCopy instead of requestOptions

Testing

Added unit test MaxItemCountNotModifiedInOriginalQueryRequestOptions_SimpleCopyTest() that verifies:

  • Original QueryRequestOptions properties remain unchanged after copying
  • Copy receives the modified MaxItemCount value
  • All other properties are preserved correctly in the copy

Impact

  • Fixes the silent mutation issue - Cached QueryRequestOptions objects are no longer modified
  • Maintains backward compatibility - No breaking changes to public APIs
  • Minimal performance impact - Shallow copy operation is lightweight
  • Surgical fix - Only the problematic line was changed, no other logic affected

Fixes #5225.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: kundadebdatta <87335885+kundadebdatta@users.noreply.github.com>
Copilot AI changed the title [WIP] LINQ Take() silently changes MaxItemCount on QueryRequestOptions object Fix LINQ Take() silently modifying cached QueryRequestOptions objects Jul 15, 2025
Copilot AI requested a review from kundadebdatta July 15, 2025 23:18
@kundadebdatta
Copy link
Copy Markdown
Member

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the required format: "[Internal] Category: (Adds|Fixes|Refactors|Removes) Description"

Internal should be used for PRs that have no customer impact. This flag is used to help generate the changelog to know which PRs should be included. Examples:
Diagnostics: Adds GetElapsedClientLatency to CosmosDiagnostics
PartitionKey: Fixes null reference when using default(PartitionKey)
[v4] Client Encryption: Refactors code to external project
[Internal] Query: Adds code generator for CosmosNumbers for easy additions in the future.

@NaluTripician
Copy link
Copy Markdown
Contributor

Hi @kundadebdatta — this Copilot-authored draft ($(System.Collections.Hashtable.copilotpr)) has been idle ~287 days with no human owner driving it. Would you (or someone on the team) be willing to pick this up, or should we close it? Flagged as part of an open-PR cleanup pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LINQ Take() silently changes MaxItemCount on QueryRequestOptions object

3 participants