-
Notifications
You must be signed in to change notification settings - Fork 13
Fix ConfigurationValidator incorrectly validating non-existent CommerceConfiguration #579
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
Fix ConfigurationValidator incorrectly validating non-existent CommerceConfiguration #579
Conversation
… exists and add tests Co-authored-by: petrinecp <5637849+petrinecp@users.noreply.github.com>
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 fixes a bug where the ConfigurationValidator incorrectly validated non-existent CommerceConfiguration sections. The issue occurred because IConfigurationSection.GetSection() returns a non-null object even when a section doesn't exist in the configuration file.
Changes:
- Fixed the null check for
CommerceConfigurationto use.Exists()method instead of null comparison - Added comprehensive test coverage for the bug fix
- Added CLI project reference to enable testing of
ConfigurationValidator
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Migration.Tool.CLI/ConfigurationValidator.cs | Changed line 164 from is not null to ?.Exists() == true to properly detect section existence |
| Migration.Tool.Tests/ConfigurationValidatorTests.cs | Added three test cases covering missing section, empty values, and valid configuration scenarios |
| Migration.Tool.Tests/Migration.Tool.Tests.csproj | Added CLI project reference to enable testing of ConfigurationValidator class |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot build action is failing on |
Co-authored-by: petrinecp <5637849+petrinecp@users.noreply.github.com>
#581) * Fix ConfigurationValidator incorrectly validating non-existent CommerceConfiguration (#579) * Initial plan * Fix ConfigurationValidator to properly check if CommerceConfiguration exists and add tests Co-authored-by: petrinecp <5637849+petrinecp@users.noreply.github.com> * Fix code formatting to pass dotnet format verification Co-authored-by: petrinecp <5637849+petrinecp@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: petrinecp <5637849+petrinecp@users.noreply.github.com> * Adjust version matrix --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: petrinecp <5637849+petrinecp@users.noreply.github.com>
Fix ConfigurationValidator to properly check if CommerceConfiguration exists
Summary
Fixed the bug where the migration tool incorrectly throws a validation error about
CommerceConfigurationwhen the section doesn't exist in appsettings.json. This resolves issue #578.Changes:
GetSection()returns non-null even when section doesn't exist.Exists()method.Exists()checkFiles Modified:
if (commerceConfiguration is not null)toif (commerceConfiguration?.Exists() == true)- this is the core fixTest Results:
✅ All tests pass (3 ConfigurationValidatorTests + 17 existing tests = 20 total)
✅ Build succeeds with no errors
✅ Code formatting passes
dotnet format --verify-no-changes✅ Code review completed with no issues
Original prompt
This pull request was created from Copilot chat.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.