Skip to content

Add AnswerRequired precondition reason and carry the reason in details - #20

Open
amicklerTMM wants to merge 2 commits into
feature23:mainfrom
amicklerTMM:portal-result-types
Open

amicklerTMM wants to merge 2 commits into
feature23:mainfrom
amicklerTMM:portal-result-types

Conversation

@amicklerTMM

Copy link
Copy Markdown
  • PreconditionFailedReason.AnswerRequired = 4, with a default message.
  • One reason-to-status map (PreconditionFailedReasonHttpExtensions) shared by the MVC and Minimal API adapters; AnswerRequired maps to 422.
  • Problem-details bodies for a precondition failure carry a "reason" extension member.
  • Precondition theories enumerate every reason instead of a hard-coded list.

…m details

- PreconditionFailedReason.AnswerRequired = 4, with a default message.
- One reason-to-status map (PreconditionFailedReasonHttpExtensions) shared by the MVC and
  Minimal API adapters; AnswerRequired maps to 422.
- Problem-details bodies for a precondition failure carry a "reason" extension member.
- Precondition theories enumerate every reason instead of a hard-coded list.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@amicklerTMM

Copy link
Copy Markdown
Author

Add an AnswerRequired precondition reason, and carry the reason in problem details

Janus (Portal) has been running a private copy of this kernel and is moving onto the package. The last difference its clients rely on is one precondition reason: an operation that can't go ahead until the caller answers a question. This PR adds that reason, and adds the reason to problem-details bodies so clients can branch on it without parsing the title.

Changes

F23.Kernel

  • PreconditionFailedReason.AnswerRequired = 4: the operation can't go ahead until the caller supplies an answer, such as a confirmation or a choice between alternatives.
  • A default message for it. Without one, PreconditionFailedResult.Message would throw ArgumentOutOfRangeException for a result created with no message.

F23.Kernel.AspNetCore

  • One map from reason to status, shared by both adapters. The new internal PreconditionFailedReasonHttpExtensions.ToHttpStatusCode() is used by both the MVC and Minimal API adapters. Each useProblemDetails switch now has one precondition case instead of one per reason.

    Reason Status
    NotFound 404 (unchanged)
    ConcurrencyMismatch 412 (unchanged)
    Conflict 409 (unchanged)
    AnswerRequired 422
  • Problem-details bodies for any precondition failure carry a reason extension member, for example "reason": "AnswerRequired". This is purely additive: title and status are unchanged. The title is free text and is often a custom message, so it isn't something a client should branch on.

  • Without problem details, AnswerRequired returns UnprocessableEntityResult / UnprocessableEntity. That matches the other reasons, which have no body in this mode.

  • The XML <returns> lists are updated, and now say which status each reason gets.

Points for review

  1. 422 for AnswerRequired is a judgement call. It says the request is well-formed but can't be processed as it stands. 428 Precondition Required was considered, but RFC 6585 ties it to conditional headers such as If-Match, and 409 is already Conflict. Happy to change it.
  2. A new enum value is a behavioural change for consumers. Any consumer code that switches over PreconditionFailedReason and throws on an unknown value will throw if handed AnswerRequired. That's worth a line in the release notes.
  3. The version isn't bumped. I left that to the release commit, following what 0.3.0 did.

Tests

  • The precondition theories now enumerate Enum.GetValues<PreconditionFailedReason>() through a shared PreconditionFailedReasons.All() member, instead of hard-coding the list. A reason added later is checked for a default message, a status code, and a reason member without anyone editing a test.
  • There are new tests for the status map (every reason, the exact codes, and a throw on an undefined value) and for both adapters with and without problem details.
  • dotnet test -f net8.0: 168 passed, 0 failed. They ran on the .NET 10 runtime via DOTNET_ROLL_FORWARD=Major, because no 8.0 ASP.NET runtime is installed locally. net9.0 wasn't run locally; CI covers it.

@RadicalCaitlin RadicalCaitlin left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. The shared reason-to-status map, the reason member in problem details, and the theories that enumerate every reason are all good generic improvements, and I'd like to land them.

My hesitation is with AnswerRequired itself. The existing reasons describe the state of the resource: it doesn't exist (NotFound), the caller's version is stale (ConcurrencyMismatch), or its state conflicts with the request (Conflict). They're generic to any API and map cleanly onto HTTP semantics. AnswerRequired describes an interaction protocol, where the server needs more input from the caller. That's application workflow rather than resource state, and the kernel can only half-support it: it can say an answer is needed, but not what the question is, so consumers still need their own convention to carry the question.

What clients need is a value they can branch on that isn't the title. Could the kernel provide that generically instead? For example:

  • Add an optional code to PreconditionFailedResult, such as string? Code or an extensions dictionary, and write it into problem details next to reason.
  • An app returns Conflict, or whichever existing reason fits, with Code = "AnswerRequired". With the extensions route, the question data can travel in the same response.

That keeps the enum to general HTTP concepts and avoids the new-enum-value breaking change for consumers. It also gives any app a place for its own app-specific cases.

Suggestion: split this PR. Land the shared map, the reason member and the test changes, and replace AnswerRequired with the code/extensions mechanism.

One question: do your clients depend on the 422 status itself, or only on being able to tell this case apart? If the status matters, the code approach alone won't cover it, and we should talk about that.

Review feedback: AnswerRequired describes an interaction protocol rather than resource
state, so it doesn't belong in PreconditionFailedReason. The app-specific case moves to an
optional code on the result, in a separate PR.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@amicklerTMM

Copy link
Copy Markdown
Author

This PR now contains only the generic parts: the shared reason-to-status map, the reason member in problem details, and the theories that enumerate every reason. AnswerRequired is gone, so PreconditionFailedReason is unchanged from 0.3.0 and there's no new-enum-value break.

I think I can skip the code entirely and use ValidationFailed, as it will provide the field that is missing in its response and we can branch off of that, and missing a required field is actually a validation error. The 422 itself is not required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants