fix: ServiceClient org metadata and multi-layer environment resolution#98
fix: ServiceClient org metadata and multi-layer environment resolution#98joshsmithxrm merged 7 commits intomainfrom
Conversation
Summary of ChangesHello @joshsmithxrm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the reliability and user experience for connecting to Dataverse environments. It resolves a critical bug where organization metadata was often missing from Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements by fixing organization metadata population and adding a multi-layer environment resolution service. The new EnvironmentResolutionService is a great architectural enhancement that centralizes and simplifies how environments are resolved, correctly handling different authentication methods like service principals and interactive users. The fix for the lazy-loading of ServiceClient metadata is clever and effectively solves the described issue. However, I've identified a regression in several credential providers where the cancellationToken is no longer respected for the initial token acquisition, which could make the application unresponsive. I've left comments with details on this. Overall, these are excellent changes that improve the robustness and usability of the authentication and environment management features.
There was a problem hiding this comment.
Pull request overview
This PR fixes ServiceClient organization metadata population issues and adds multi-layer environment resolution to support service principals with URL-based environment selection.
Key Changes:
- Fixed lazy initialization of ServiceClient org metadata by eagerly accessing
ConnectedOrgFriendlyNamein 5 credential providers - Added
EnvironmentResolutionServicethat tries direct Dataverse connection before falling back to Global Discovery - Updated
ppds env selectandppds auth update --environmentcommands to use the new resolution service
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/PPDS.Auth/Discovery/EnvironmentResolutionService.cs |
New multi-layer environment resolution service with direct connection and Global Discovery fallback |
src/PPDS.Auth/Credentials/InteractiveBrowserCredentialProvider.cs |
Switched to ConnectionOptions constructor and added eager metadata access |
src/PPDS.Auth/Credentials/DeviceCodeCredentialProvider.cs |
Switched to ConnectionOptions constructor and added eager metadata access |
src/PPDS.Auth/Credentials/ManagedIdentityCredentialProvider.cs |
Switched to ConnectionOptions constructor and added eager metadata access |
src/PPDS.Auth/Credentials/GitHubFederatedCredentialProvider.cs |
Switched to ConnectionOptions constructor and added eager metadata access |
src/PPDS.Auth/Credentials/AzureDevOpsFederatedCredentialProvider.cs |
Switched to ConnectionOptions constructor and added eager metadata access |
src/PPDS.Cli/Commands/Env/EnvCommandGroup.cs |
Replaced Global Discovery-only logic with EnvironmentResolutionService |
src/PPDS.Cli/Commands/Auth/AuthCommandGroup.cs |
Replaced string parsing with EnvironmentResolutionService for validation |
src/PPDS.Auth/CHANGELOG.md |
Documented ServiceClient metadata fix and new resolution service |
src/PPDS.Cli/CHANGELOG.md |
Documented environment resolution improvements for service principals |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Issue #86: Added SkipDiscovery = false to ConnectionOptions in 5 credential providers (InteractiveBrowser, DeviceCode, ManagedIdentity, GitHubFederated, AzureDevOpsFederated) to force org metadata discovery. Issues #89 + #88: Added EnvironmentResolutionService for multi-layer resolution: - Direct Dataverse connection first (works for service principals) - Global Discovery fallback (for interactive users) - Updated ppds env select and ppds auth update --environment to use new resolver 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
ServiceClient org metadata discovery is lazy - properties are only populated when first accessed. Since the connection pool clones the seed client before properties are accessed, the clones had empty org metadata. Fix: Access ConnectedOrgFriendlyName immediately after creating the ServiceClient to trigger discovery while values can still be copied to clones. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Testing confirmed that SkipDiscovery = false is not required. The eager property access alone triggers org metadata discovery. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Added detailed comments explaining why we access ConnectedOrgFriendlyName immediately after ServiceClient creation. Referenced that PAC CLI uses the same pattern (ConnectedOrgToSelectedOrganizationInfo after Connect). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add token cache priming with cancellationToken in GitHubFederated, AzureDevOpsFederated, and ManagedIdentity credential providers - Simplify CanUseGlobalDiscovery to use 'is or' pattern 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
joshsmithxrm
force-pushed
the
fix/environment-resolution
branch
from
6904f3e to
334ae11
Compare
joshsmithxrm
temporarily deployed
to
test-dataverse
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
joshsmithxrm
temporarily deployed
to
test-dataverse
GitHub Actions
Inactive
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Fixed return description to match actual Result type - Removed incorrect exception documentation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
joshsmithxrm
force-pushed
the
fix/environment-resolution
branch
from
582611d to
b9bd8d2
Compare
joshsmithxrm
temporarily deployed
to
test-dataverse
GitHub Actions
Inactive
Summary
ConnectedOrgFriendlyName,ConnectedOrgUniqueName,ConnectedOrgIdwere empty)ppds env selectandppds auth update --environmentRoot Cause (#86)
ServiceClient uses lazy initialization for org metadata properties. The connection pool clones clients before properties are accessed, so clones had empty metadata.
Fix: Access
ConnectedOrgFriendlyNameimmediately after ServiceClient creation to force lazy initialization before cloning.Changes
Credential Providers (5 files)
InteractiveBrowserCredentialProvider.csDeviceCodeCredentialProvider.csManagedIdentityCredentialProvider.csGitHubFederatedCredentialProvider.csAzureDevOpsFederatedCredentialProvider.csAdded eager property access after ServiceClient creation.
New Service
EnvironmentResolutionService.cs- Multi-layer resolution:CLI Commands
EnvCommandGroup.cs- Updatedppds env selectto use resolverAuthCommandGroup.cs- Updatedppds auth update --environmentto use resolverTest plan
ppds env whoshows populated Unique Name and Friendly Nameppds env select <url>works with service principals (direct connection)ppds env select <name>works with user auth (Global Discovery fallback)🤖 Generated with Claude Code