refactor: add logging to GetComponentTypeAsync catch block#110
refactor: add logging to GetComponentTypeAsync catch block#110joshsmithxrm merged 2 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 enhances the security and robustness of the application's authentication mechanisms. It introduces a dedicated secure credential store to isolate sensitive information from profile configurations, aligning with best practices for handling secrets. Concurrently, it refines error handling and logging within the plugin registration process, making the system more resilient and easier to diagnose. The underlying profile storage schema has been updated to a more flexible and secure version, with a clear migration path for existing users. 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
|
joshsmithxrm
had a problem deploying
to
test-dataverse
GitHub Actions
Failure
![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 a significant and valuable security enhancement by refactoring authentication profile storage, moving secrets from a JSON file to a platform-native secure store using MSAL.Extensions. The introduction of a v2 profile schema is handled well, with a clear (though destructive) migration path for v1 profiles. The secondary change to add logging and specific exception handling in PluginRegistrationService also improves diagnostics and robustness. The code is well-structured and includes comprehensive test updates. I have a few suggestions to further improve exception handling and code quality, such as refining JsonException handling in ProfileStore.cs and addressing a redundant null check.
There was a problem hiding this comment.
Pull request overview
This PR adds diagnostic logging to PluginRegistrationService and replaces generic exception handling with specific FaultException handlers for better error categorization. Additionally, it includes a major refactoring to profile storage schema v2 with platform-native secure credential storage.
Key Changes:
- Added
ILogger<PluginRegistrationService>dependency for diagnostic logging in exception handlers - Replaced generic catch clause with specific
FaultException<OrganizationServiceFault>andFaultExceptionhandlers inGetComponentTypeAsync - Migrated profile storage to v2 schema using array-based storage and name-based active profile tracking
- Implemented
SecureCredentialStorefor platform-native credential encryption (DPAPI/Keychain/libsecret) - Moved secrets from profile JSON to secure credential store
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/PPDS.Cli/Plugins/Registration/PluginRegistrationService.cs |
Added logger dependency and specific exception handling for metadata retrieval failures |
src/PPDS.Cli/Commands/Plugins/ListCommand.cs |
Updated to inject logger for PluginRegistrationService |
src/PPDS.Cli/Commands/Plugins/DiffCommand.cs |
Updated to inject logger for PluginRegistrationService |
src/PPDS.Cli/Commands/Plugins/DeployCommand.cs |
Updated to inject logger for PluginRegistrationService |
src/PPDS.Cli/Commands/Plugins/CleanCommand.cs |
Updated to inject logger for PluginRegistrationService |
src/PPDS.Cli/Commands/Auth/AuthCommandGroup.cs |
Updated to use secure credential store for storing secrets during profile creation |
src/PPDS.Cli/CHANGELOG.md |
Documented breaking changes and new logger requirement |
src/PPDS.Auth/Profiles/ProfileStore.cs |
Refactored to support v2 schema with v1 migration (deletion) logic |
src/PPDS.Auth/Profiles/ProfileCollection.cs |
Changed from dictionary to array storage with name-based active profile |
src/PPDS.Auth/Profiles/EnvironmentInfo.cs |
Removed redundant Id field |
src/PPDS.Auth/Profiles/AuthProfile.cs |
Removed secret fields (ClientSecret, CertificatePassword, Password) and added Authority field |
src/PPDS.Auth/Profiles/ProfilePaths.cs |
Added CredentialCacheFileName constant |
src/PPDS.Auth/Credentials/SecureCredentialStore.cs |
New class providing platform-native encrypted credential storage |
src/PPDS.Auth/Credentials/ISecureCredentialStore.cs |
New interface defining secure credential storage contract |
src/PPDS.Auth/Credentials/CredentialProviderFactory.cs |
Added CreateAsync method supporting secure store lookups and environment variable overrides |
src/PPDS.Auth/Credentials/ClientSecretCredentialProvider.cs |
Updated factory methods to support credential store and environment variables |
src/PPDS.Auth/Credentials/CertificateFileCredentialProvider.cs |
Updated factory methods to support credential store for passwords |
src/PPDS.Auth/Credentials/JwtClaimsParser.cs |
Refactored claim extraction with helper methods |
src/PPDS.Auth/CHANGELOG.md |
Documented breaking changes for profile v2 schema and secure credential storage |
tests/PPDS.Cli.Tests/Plugins/Registration/PluginRegistrationServiceTests.cs |
Added 6 unit tests for basic PluginRegistrationService operations |
tests/PPDS.Cli.Tests/PPDS.Cli.Tests.csproj |
Added Moq package reference for testing |
tests/PPDS.Auth.Tests/Profiles/ProfileStoreTests.cs |
Updated tests for v2 schema including migration tests |
tests/PPDS.Auth.Tests/Profiles/ProfileCollectionTests.cs |
Updated tests for name-based active profile tracking |
tests/PPDS.Auth.Tests/Profiles/EnvironmentInfoTests.cs |
Removed Id parameter from tests |
tests/PPDS.Auth.Tests/Profiles/AuthProfileTests.cs |
Removed secret field tests and updated validation tests |
tests/PPDS.Auth.Tests/Credentials/SecureCredentialStoreTests.cs |
New comprehensive test suite for secure credential storage |
tests/PPDS.Auth.Tests/Credentials/CredentialProviderFactoryTests.cs |
Updated tests for new credential store requirements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/PPDS.Cli.Tests/Plugins/Registration/PluginRegistrationServiceTests.cs
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
joshsmithxrm
force-pushed
the
xenodochial-black
branch
from
e21ed0c to
82f4bce
Compare
- Add required ILogger<PluginRegistrationService> dependency - Update all callers (ListCommand, CleanCommand, DiffCommand, DeployCommand) to get logger from service provider and pass to constructor - Replace generic catch clause with specific FaultException handlers - Log failures at Debug level for troubleshooting while maintaining graceful degradation behavior (returns 0 to skip solution addition) - Add System.ServiceModel using for FaultException types - Add unit tests for PluginRegistrationService Fixes #61 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
joshsmithxrm
force-pushed
the
xenodochial-black
branch
from
82f4bce to
253e3bc
Compare
|
@gemini-code-assist review |
joshsmithxrm
temporarily deployed
to
test-dataverse
GitHub Actions
Inactive
There was a problem hiding this comment.
Code Review
This pull request is a solid improvement, correctly adding dependency injection for the logger and enhancing exception handling in PluginRegistrationService with more specific FaultException catches and diagnostic logging. The changes are consistently applied across all relevant command handlers. The addition of unit tests for PluginRegistrationService is also a great step forward for code quality.
My review includes a couple of suggestions for the new test file to further improve its robustness and consistency with the project's coding standards, primarily by using the existing early-bound entity classes and by adding a test case to verify the new logging behavior.
tests/PPDS.Cli.Tests/Plugins/Registration/PluginRegistrationServiceTests.cs
Outdated
Show resolved
Hide resolved
tests/PPDS.Cli.Tests/Plugins/Registration/PluginRegistrationServiceTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Refactor tests to use early-bound entity classes (PluginAssembly, SdkMessage) instead of late-bound Entity with magic strings - Fix exception handling tests to use UpsertPluginTypeAsync (which actually calls GetComponentTypeAsync) instead of UpsertPackageAsync (which uses CreateWithSolutionHeaderAsync and doesn't call it) - Add tests for FaultException, FaultException<OrganizationServiceFault>, and solution addition skip when componentType returns 0 Addresses bot review comments #8, #14, #15 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
joshsmithxrm
temporarily deployed
to
test-dataverse
GitHub Actions
Inactive
Summary
ILogger<PluginRegistrationService>dependency to enable diagnostic loggingFaultExceptionhandlers for better error categorizationChanges
GetComponentTypeAsyncTest plan
Fixes #61
🤖 Generated with Claude Code