Skip to content

Python: Prefer injected DocumentDB clients over connection settings - #9999

Open
Yufeng He (he-yufeng) wants to merge 7 commits into
microsoft:mainfrom
he-yufeng:fix/azure-documentdb-injected-client-precedence
Open

Yufeng He (he-yufeng) wants to merge 7 commits into
microsoft:mainfrom
he-yufeng:fix/azure-documentdb-injected-client-precedence

Conversation

@he-yufeng

@he-yufeng Yufeng He (he-yufeng) commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Motivation & Context

AzureDocumentDBStore and AzureDocumentDBCollection reject caller-supplied PyMongo clients, databases, and collections whenever redundant connection settings are present. They also require an explicit database name for an account-level injected client instead of using the existing environment selector. This prevents configuration layers from injecting a fully configured object while still forwarding unrelated construction settings.

Fixes #9954, which is labelled ready-for-implementation and defines the injection, selector, and ownership acceptance cases. No competing open PR exists for this issue.

Description & Review Guide

  • What are the major changes? Injected objects now bypass connection strings, client options, selected .env files, and ambient connection settings. Account clients resolve a missing database selector from AZURE_DOCUMENTDB_DATABASE_NAME. Collections created from an injected client or database resolve a missing collection selector from an explicit argument, the registered model definition, then AZURE_DOCUMENTDB_COLLECTION_NAME. Resource settings are loaded separately, so connection parsing and validation cannot leak back into these paths.
  • Injected database and collection names still support exact matching validation, multiple injected objects are still rejected, and caller-owned clients remain open after wrapper cleanup. No-injection initialization and connector-owned cleanup are unchanged.
  • The package guide and constructor documentation now describe precedence, selector fallback, and ownership. New regression coverage spans all five store/collection injection targets, explicit and environment selectors, missing selectors, invalid ambient settings, name mismatches, and registered-model fallback without an explicit definition. The original settings matrix produced 14 failures on the base and passes after the change. Missing-selector errors now name the actual database_name or collection_name constructor argument. On current head abc573b49c, the complete connector suite is 130 passed, 1 skipped; mypy and zuban report no issues in the four test source files, Pyright reports zero source errors, and Ruff passes.
  • What is the impact of these changes? Alpha construction stops raising for redundant settings and consistently treats injected objects as caller-owned. Provider operation and schema options are untouched.
  • What do you want reviewers to focus on? Please check the separation between client-construction settings and resource selectors, especially the registered-definition-before-environment collection-name precedence and the preservation of multiple-injection and name-mismatch errors.

Related Issue

Fixes #9954

AI Assistance

  • No material AI assistance was used.
  • This is an AI-assisted contribution. I reviewed, understood, and verified all submitted content and accept responsibility for it.

AI assistance details: implementation, tests, documentation, and this PR description. The author reviewed the final diff, ran the base-to-fixed regression controls and connector suite, and checked Ruff, Pyright, and repository hooks locally.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR links to an agreed issue with no competing open PR, or the Related Issue section documents a trivial-change or repository-automation exception.
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix); a workflow keeps the label and title prefix in sync automatically.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Missing-selector errors reference a nonexistent constructor argument, and precedence tests leave registered-model collection names untested against conflicting environment settings.

2 open issues
What changed in this PR

This PR addresses #9954 by making the Python DocumentDB connector prefer caller-injected PyMongo objects over connection settings while preserving caller ownership.

Changes:

  • Separates resource-selector resolution from client-construction settings.
  • Adds environment fallback for unresolved selectors, with model-defined collection names taking precedence.
  • Documents injection behavior and adds regression tests for precedence, validation, and cleanup.
File Description
python/​packages/​azure-documentdb/​tests/​azure_documentdb/​test_settings.py Adds injection, selector, name-mismatch, and ownership regression coverage.
python/​packages/​azure-documentdb/​README.md Explains injected-object precedence and selector fallback.
python/​packages/​azure-documentdb/​agent_framework_azure_documentdb/​_vector_store.py Updates injection handling, isolates selector settings, and documents constructor behavior.

Review effort: Balanced

@he-yufeng

Copy link
Copy Markdown
Contributor Author

The two hosted coverage failures are outside this connector diff. On abc573b49c, running TestStateTypeRegistry::test_registered_child_tag_does_not_hijack_message_payload followed by test_mixed_declaration_only_batch_defers_siblings_until_all_host_results reproduces both failures in _sessions.py: an application codec registered as text receives a framework text-content dictionary containing text, while that test codec expects value. This PR does not change core.

The same ordered control passes on the current head of #9250. I am keeping this PR scoped to DocumentDB rather than adding an unrelated core patch, and will merge main and recheck the hosted suite after that session restoration work lands.

On the current head, the complete DocumentDB connector suite passes with 130 passed and 1 skipped.

@he-yufeng

Copy link
Copy Markdown
Contributor Author

Merged current main in 9200a4549.

The coverage failure came from test state leaking across the aggregate worker: test_registered_child_tag_does_not_hijack_message_payload registers a custom decoder under text, and the later approval tests then used that decoder for serialized text content. Current main isolates the state registry for that test class.

On the merged head, DocumentDB is 130 passed and 1 skipped, while the session and function-invocation files are 675 passed and 1 skipped. Syntax, strict Pyright, and the pre-commit hooks pass. The PR diff remains limited to the DocumentDB connector, its tests, and the package README.

This branch was successfully deployed

1 active deployment
github-app-auth — 9200a454 Deployed Nov 7, 2026 by he-yufeng via add_label #27349
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: [Bug]: Azure DocumentDB injected clients do not take precedence over connection settings

2 participants