Ensure failed results always have a Problem #40
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
managedcode#32 managedcode#35 Begin work to improve Result state safety, ie disallow a Result to be both successful & failed, and to ensure failed results are never missing a Problem. Made constructors for Result types private. Replaced Result Create methods with CreateSuccess & CreateFailed to force parameters for either case. Result.Problem will never be null when the result is unsuccessful. If no problem was recorded, it falls back to returning Problem.GenericError.
Repeated changes from last commit in the CollectionResult type. IsFailed and HasProblem properties just return the opposite of IsSuccess, since failed results now always have a Problem by design. ThrowIfFail implementations don't rely on HasProblem. HasProblem is now functionally equivalent to IsFailed and is only necessary for the IResultProblem interface.
Added [JsonInclude] attribute to IsSuccess properties, as init setter is now private. Moved Json-related attributes from Problem properties to _problem backing fields. Added [JsonIgnore] attribute to Problem properties, as the backing field should now be serialized. Added [JsonInclude] attribute to _problem fields.
Changed several tests that expect 'HasProblem' to be false in the case of Fail methods that previously did not assign a Problem. This change has been made under the assumption that this is indeed an oversight: managedcode#32 (comment)
Change tests that use the AddInvalidMessage methods to operate on a failed result, since said methods now throw when invoked on a successful result. Restore a private unused constructor for Result<T>, which is accessed via Activator.CreateInstance in CommunicationOutgoingGrainCallFilter
Added two new tests that assert the AddInvalidMessage methods should throw an InvalidOperationException when the result is successful
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #40 +/- ##
=======================================
Coverage ? 75.24%
=======================================
Files ? 75
Lines ? 1850
Branches ? 292
=======================================
Hits ? 1392
Misses ? 458
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR enforces mutual exclusivity between successful and failed results by ensuring failed results always have a Problem and successful results never do. The changes prevent inconsistent state where a result could be both successful and have a problem.
- Replaced
Createmethod with separateCreateSuccessandCreateFailedmethods to enforce proper parameters - Made
IsSuccessandProblemproperties immutable to prevent external modification - Added automatic fallback to generic error for failed results without an explicit problem
- Updated all fail methods to use the new creation pattern and ensure Problem is always assigned
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ManagedCode.Communication/ResultT/Result.cs | Core Result implementation with new private constructor and separate creation methods |
| ManagedCode.Communication/Result/Result.cs | Core Result implementation with new private constructor and separate creation methods |
| ManagedCode.Communication/CollectionResultT/CollectionResult.cs | Core CollectionResult implementation with new private constructor and separate creation methods |
| ManagedCode.Communication/IResultProblem.cs | Interface change removing Problem setter |
| Various Fail/Succeed method files | Updated to use new CreateSuccess/CreateFailed methods |
| Various test files | Updated assertions to reflect that failed results now always have problems |
| Orleans converter files | Updated to use new creation methods in surrogate converters |
| AspNetCore extension files | Updated to use GetProblemNoFallback() to avoid automatic generic error fallback |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
KSemenenko
reviewed
Aug 21, 2025
| Problem = Problem.Validation((key, value)); | ||
| } | ||
| else | ||
| if (IsSuccess) throw new InvalidOperationException("Cannot add invalid message to a successful result"); |
Member
There was a problem hiding this comment.
maybe here we can change success to failed? in this case?
just asking
Contributor
Author
There was a problem hiding this comment.
I think this would be a better solution. Please let me know what you think and if you'd like me to make any changes and resubmit.
The AddInvalidMessage code is already duplicated between Result, Result<T> and CollectionResult<T>. We could instead move it to the Problem class to reduce duplication. The Result.AddInvalidMessage methods would be best to become obsolete:
// Make the Result.AddInvalidMessage methods obsolete
[Obsolete("Use Problem.AddInvalidMessage instead")]
public void AddInvalidMessage(string message)
{
if (!IsSuccess)
Problem.AddInvalidMessage(message);
// else, do nothing
}And here is a usage example:
if (result.IsSuccess)
{
// It does not make sense to add invalid messages to the successful result.
// If we really need to do that, make a new one.
var invalidResult = Result.Invalid("message");
}
else
{
result.Problem.AddInvalidMessage("message");
}
// User can still call Result.AddInvalidMessage without checking IsSuccess,
// it just won't do anything if successful; Result will remain successful.
result.AddInvalidMessage("message");Benefits:
- We retain state immutability of Results.
- We reduce code duplication.
- Readability improves. It is clear to the user that they should only add invalid messages to an existing Problem, not to a successful result.
- The
throwis removed.
Contributor
Author
There was a problem hiding this comment.
I've implemented the idea I mentioned above. Please let me know your thoughts, thanks
Moved main implementation of AddInvalidMessage methods to the Problem class (these were previously duplicated identically in all 3 Result types). The same is done for InvalidField & InvalidFieldError methods. AddInvalidMessage methods on Result types are now obsolete and no longer throw an exception when the result is successful. Moved AddInvalidMessage test methods for CollectionResult type to instead test directly on the Problem type.
Removed Problem.AddInvalidMessage methods added in last commit. Use the existing AddValidationError method instead which does the same thing, except safer. All unit tests now passing again.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Successful results and problematic results should be mutually exclusive. Failed results should always have a problem by design, and successful results should not. This PR ensures
Problemcan never be null whenIsSuccessisfalse. Additionally, it cleans up cases where a result could be both successful and failed.Type of Change
Changes Made
Result, ResultandCollectionResulttypes. I'll just refer toResult` for simplicity.Resultconstructor frominternaltoprivate.internalCreatemethods with aCreateSuccessandCreateFailedmethod to help enforce parameters in either case.Result.Fail(...)) now use these new methods, whereas previously some cases directly used the constructor.IsSuccessset method frominittoprivate init.Problemset method fromsettoprivate init.IResultProblem.Problem.setproperty setter.Problemproperty.Problem.getchecks if the backing field is null and success is false, and assigns a generic error in this case. This will only occur if the user uses the default result value, ie.Result r = default;. Also note that the Json-related attributes onProblemproperty have been moved to the backing field, and the property now has a[JsonIgnore]attribute.[JsonInclude]attributes to new private data to preserve deserialization.AddInvalidMessagemethods to throw anInvalidOperationExceptionwhen invoked on a successful result.HasProblem.Should().BeFalse()for failed results. These tests usedFail(...)overloads that previously did not assign aProblem, hence the assertion. These tests now assertShould().BeTrue().Testing
Checklist
Related Issues
Closes #32
Closes #35
Screenshots (if applicable)
Additional Notes
Since
IsSuccess&Problemproperties can no longer be publicly set, users can no longer construct a result like so:new Result { IsSuccess = false, Problem = ... };.