refactor: migrate UserInvitationResult to AndreGoepel.Core.Result<T> - #152
Merged
Conversation
Replaces the repo's ad-hoc UserInvitationResult record with the canonical AndreGoepel.Core.Result<UserInvitationDetails>, now that AndreGoepel.Core sits below marten-identity in the dependency graph. IdentityResult usage in UserStore/RoleStore is untouched, per #148's guardrail.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Adds
AndreGoepel.Corev1.0.1 (AndreGoepel.Core.Result<T>) as a dependency ofAndreGoepel.Marten.Identityand uses it for the one real ad-hoc result type in this repo. Per #148's guardrail,Microsoft.AspNetCore.Identity.IdentityResultusage inUserStore/RoleStoreis untouched — that stays the idiomatic contract the framework expects.1.
UserInvitationResult→Result<UserInvitationDetails>UserInvitationService.InviteAsync/ResendAsyncnow returnResult<UserInvitationDetails>(UserInvitationDetails=(User User, string Token)) instead of the bespokeUserInvitationResultrecord.Errors → Error reconciliation: the old type wrapped a full
IdentityResultand exposed a computedErrorMessagethat already joinedResult.Errors.Select(e => e.Description)for display — so the "collection vs single string" question was largely already answered by the existing code. I audited every failure path inInviteAsync/ResendAsync:NotAuthorized,DuplicateEmail,InvitationAlreadyAccepted— each constructs exactly oneIdentityError.userManager.CreateAsync(user)anduserStore.AddToRoleAsync(...)return real frameworkIdentityResults that could in principle carry more than one error.Rather than assume single-error-only,
Result<UserInvitationDetails>.Erroris built by joiningIdentityResult.Errors.Select(e => e.Description)with", "— identical to the oldErrorMessagebehavior, so no information is dropped even in the multi-error case.What is genuinely lost:
IdentityError.Code(e.g.NotAuthorized,DuplicateEmail,InvitationAlreadyAccepted) is no longer carried at this boundary, sinceResult<T>.Erroris a plain string. I checked every consumer before accepting this:UserInvitationMailer(the only direct consumer) never branched on.Code, only joined.Errors.Select(e => e.Description).InviteUser.razor.csandUsers.razor(the Blazor UI) consumeUserInvitationMailer's return value, which isIdentityResult— untouched by this PR — and also only ever display the joined description text, never branch on.Code.UserInvitationServiceTests) asserted on.Code; these are updated to assert on the message content instead (e.g.Assert.Contains("administrator authority", result.Error)).So the code-branching contract
IdentityErrorCodesdocuments ("consuming apps branch on them") is preserved everywhere it actually applies — it was never exercised at theUserInvitationResultboundary to begin with.UserInvitationMailerkeeps its own publicTask<IdentityResult>signature unchanged (that's the Blazor-facing contractInviteUser.razor.cs/Users.razoruse, out of scope here) and internally reconstructs anIdentityResult.FailedfromResult<T>.Errorwith a generic code, since nothing reads it.2. Passkey
?? throw new InvalidOperationException("User not found")(UserStore.cs ~728/764/790) — left as documented follow-upAll three throw sites live inside
UserStore<TUser>methods (AddOrUpdatePasskeyAsync,GetPasskeysAsync,FindPasskeyAsync) that implementMicrosoft.AspNetCore.Identity.IUserPasskeyStore<TUser>. Unlike #143'sAddToRoleAsync/RemoveFromRoleAsync(which had a repo-owned overload the store's own callers used, separate from the interface-mandated one), these three are called directly throughUserManager<TUser>in real app code (IdentityComponentsEndpointRouteBuilderExtensions.cs:171). Their return types —Task,Task<IList<UserPasskeyInfo>>,Task<UserPasskeyInfo?>— are dictated by the ASP.NET Core Identity framework, not this repo, so changing them toResult<T>isn't a "many call sites" tradeoff, it's a hard interface-compliance blocker:UserManager's own wrapper methods have fixed signatures we cannot change.I also considered a narrower change — silently returning
[]/nullinstead of throwing for the two methods whose existing signatures could already encode "not found" (GetPasskeysAsyncreturning empty,FindPasskeyAsyncalready returningUserPasskeyInfo?) — but decided against it: these methods are re-querying for aTUserthat was already loaded by the caller, so "not found" here means the user record vanished between load and this call (e.g. deleted concurrently) or a stale/detached user was passed in. Silently treating that as "no passkeys" would mask a real data-consistency bug rather than surface it. Leaving this as a fail-fast throw stays consistent with how the rest of the store treats unexpected state.Leaving this as documented follow-up rather than forcing a
Result<T>/nullable shape that would either not compile against the framework interface or quietly change failure semantics.Verification
dotnet csharpier format .— clean, only the files this PR touches were reformatted.dotnet build— 0 errors, 0 warnings (TreatWarningsAsErrorsis active).dotnet test --filter "FullyQualifiedName!~E2ETests"— all green: 191 unit tests, 130 Blazor bUnit tests, 92 integration tests (Testcontainers/PostgreSQL, Docker was available), including allUserInvitationServiceTests.dotnet list package --vulnerable --include-transitive— clean across all projects.Closes #148
🤖 Generated with Claude Code