Return distinct error for JSON null body instead of 'non-empty request body'#65595
Open
kubaflo wants to merge 2 commits intodotnet:mainfrom
Open
Return distinct error for JSON null body instead of 'non-empty request body'#65595kubaflo wants to merge 2 commits intodotnet:mainfrom
kubaflo wants to merge 2 commits intodotnet:mainfrom
Conversation
…t body' Fixes dotnet#40415 When a POST request with Content-Type: application/json has body containing the literal JSON value 'null', both SystemTextJsonInputFormatter and NewtonsoftJsonInputFormatter would return InputFormatterResult.NoValue(). BodyModelBinder then generated the error 'A non-empty request body is required.' — misleading because the body IS non-empty (it contains valid JSON). The fix changes the formatters to return InputFormatterResult.Failure() with a distinct error message when the deserialized model is null and the parameter does not allow empty input. The Newtonsoft formatter tracks whether the JSON reader actually parsed tokens (hadJsonContent) to distinguish JSON null from an exhausted/empty stream. For nullable parameters, behavior is unchanged — EmptyBodyBehavior inference (from .NET 7) already sets TreatEmptyInputAsDefaultValue=true, so JSON null is accepted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes misleading MVC model-binding behavior where a JSON request body containing the literal value null is treated as an “empty body”, by returning a formatter failure with a dedicated error message when the body is required.
Changes:
- Add a new localized resource string for the “JSON null is invalid for required body” error.
- Update both
SystemTextJsonInputFormatterandNewtonsoftJsonInputFormatterto returnInputFormatterResult.Failure()(with ModelState error) when deserialization results innulland empty input is not allowed. - Add/adjust unit tests covering
nullJSON behavior (and Newtonsoft whitespace handling).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Mvc/Mvc.Core/src/Resources.resx | Adds new resource string for the distinct “JSON null is invalid” error. |
| src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs | Returns Failure() with the new error message when required body deserializes to null. |
| src/Mvc/Mvc.NewtonsoftJson/src/Resources.resx | Adds the same new resource string for NewtonsoftJson MVC package. |
| src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs | Adds hadJsonContent tracking and returns Failure() with the new error message for required null JSON. |
| src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs | Adds tests asserting distinct error behavior for required null JSON. |
| src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs | Adds targeted whitespace-input tests to preserve existing Newtonsoft behavior. |
src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs
Outdated
Show resolved
Hide resolved
- Use context.Metadata.DisplayName/Name instead of context.ModelName for error message placeholder (ModelName is empty for top-level body binding) - Change 'parameter' to 'parameter or property' since [FromBody] applies to both Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. |
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.
🤖 AI Summary
🔍 Automated Fix Report
🔍 Pre-Flight — Context & Validation
Issue: #40415 - JSON "null" request body rejected as an empty request body
Area: area-mvc (
src/Mvc/)PR: #41002 (closed/stale — will create new)
Key Findings
nullbody (4 bytes:n,u,l,l) deserializes tomodel == nullin bothSystemTextJsonInputFormatterandNewtonsoftJsonInputFormatterInputFormatterResult.NoValue()when model is null andTreatEmptyInputAsDefaultValueis falseBodyModelBinderthen adds error: "A non-empty request body is required." — misleading because the body IS non-emptyFromBody.AllowEmptyBehavior = Allowbased nullability information #39754) —TreatEmptyInputAsDefaultValue= true, so JSON null acceptednullnullin formatters, returnFailure()with distinct error messageTest Command
dotnet test src/Mvc/Mvc.Core/test/Microsoft.AspNetCore.Mvc.Core.Test.csproj --filter "FullyQualifiedName~JsonInputFormatter"Fix Candidates
🧪 Test — Bug Reproduction
Test Result: ✅ TESTS CREATED
Test Command:
dotnet test src/Mvc/Mvc.Core/test/Microsoft.AspNetCore.Mvc.Core.Test.csproj --filter "NullJsonInput"Tests Created: 2 tests in base class (run by both STJ and Newtonsoft), 2 Newtonsoft-specific
Tests Written
ReadAsync_WithNullJsonInput_WhenAllowingEmptyInput_SetsModelToNull— Verifies JSONnullis accepted when TreatEmptyInputAsDefaultValue=trueReadAsync_WithNullJsonInput_WhenNotAllowingEmptyInput_ReturnsFailureWithDistinctError— Verifies JSONnullreturns Failure with distinct error message (not "non-empty request body")ReadAsync_WithWhitespaceInput_WhenAllowingEmptyInput_SetsModelToNull— (Newtonsoft-specific) Whitespace accepted when allowedReadAsync_WithWhitespaceInput_WhenNotAllowingEmptyInput_ReturnsFailure— (Newtonsoft-specific) Whitespace returns FailureConclusion
Tests verify that JSON
nullbody now returns a distinct error message about null values rather than the misleading "non-empty request body required" message. Both SystemTextJsonInputFormatter and NewtonsoftJsonInputFormatter are covered via the shared test base class.🚦 Gate — Test Verification & Regression
Gate Result: ✅ PASSED
Test Command:
dotnet test src/Mvc/Mvc.Core/test/Microsoft.AspNetCore.Mvc.Core.Test.csproj --filter "JsonInputFormatter"New Tests vs Buggy Code
Regression Check
Conclusion
Tests properly verify the bug fix. The key gate test fails on buggy code because the old code returns
NoValue()(HasError=false) instead ofFailure()(HasError=true) with a distinct error message.🔧 Fix — Analysis & Comparison (✅ 5 passed)
Fix Exploration Summary
Total Attempts: 5
Passing Candidates: 5
Selected Fix: Attempt 0 — Return Failure() with distinct resource string in formatters
Attempt Results
Cross-Pollination
All models converge on the same core insight:
InputFormatterResult.NoValue()is wrong when the body contains JSONnullbecauseBodyModelBinderinterprets it as "empty body". Models differ on WHERE to make the change (formatter vs binder) and HOW to detect JSON null.Exhausted: Yes
Comparison
Recommendation
Attempt 0 is the best fix:
hadJsonContentflag in Newtonsoft to avoid breaking the exhausted-stream edge case✅ Attempt 0: PASS
Attempt 0: Return Failure with distinct error message in formatters
Model: claude-sonnet-4.6 (manual implementation)
Approach
FormatExceptionMessage_NullJsonIsInvalidto both Mvc.Core and Mvc.NewtonsoftJsonSystemTextJsonInputFormatter: when model == null && !TreatEmptyInputAsDefaultValue, add model error and returnFailure()instead ofNoValue()NewtonsoftJsonInputFormatter: same change, but withhadJsonContentflag check (trackingjsonReader.TokenType != JsonToken.None) to distinguish JSONnullfrom exhausted streamKey Design Decisions
hadJsonContentcheck because STJ throwsJsonExceptionon empty/exhausted streamshadJsonContentbecause Newtonsoft returns null (no exception) for empty streamsFiles Changed
src/Mvc/Mvc.Core/src/Resources.resxsrc/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cssrc/Mvc/Mvc.NewtonsoftJson/src/Resources.resxsrc/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cssrc/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cssrc/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs📄 Diff
✅ Attempt 1: PASS
Attempt 1: BodyModelBinder Content-Length Detection
Approach
Fix the misleading "non-empty request body is required" error by modifying BodyModelBinder (not the formatters) to detect when the request body contained content that deserialized to null.
Key Difference from Attempt 0
Failure()with a new resource string error. AddedhadJsonContentflag in Newtonsoft formatter.NoValue(). The fix is entirely inBodyModelBinder, which checksHttpRequest.ContentLength > 0in the NoValue handler to distinguish between an empty body and a body containing JSON null.Changes Made
src/Mvc/Mvc.Core/src/Resources.resx: Added new resource stringModelBinding_NullRequestBodyValueRequired= "The request body contained a null JSON value, which is not valid for a required parameter. A non-null value is required."src/Mvc/Mvc.Core/src/ModelBinding/Binders/BodyModelBinder.cs: In the NoValue handler (theelsebranch after checkingresult.HasErrorandresult.IsModelSet), added a check forhttpContext.Request.ContentLength > 0. If the body had content, uses the new null-specific message. Otherwise, falls back to the existingMissingRequestBodyRequiredValueAccessor()message.src/Mvc/Mvc.Core/test/ModelBinding/Binders/BodyModelBinderTests.cs: Added testBindModel_NullJsonInput_WhenNotAllowingEmptyInput_ProducesDistinctErrorthat verifies the error message contains "null" and does NOT contain "non-empty" when the formatter returns NoValue and the request had ContentLength > 0.Why This Approach
MissingRequestBodyRequiredValueAccessoris still used for truly empty bodies📄 Diff
⚪ Attempt 2: UNKNOWN
Attempt 2 - Alternative approach
Idea
Treat a literal JSON
nulltoken as an invalid input (whenTreatEmptyInputAsDefaultValueisfalse) inside the JSON input formatters, and record a model-state error message that explicitly referencesnull.This avoids the misleading
BodyModelBinderfallback message ("A non-empty request body is required."), which is appropriate for truly empty bodies, but not for a non-empty body containing the JSON tokennull.Implementation details
Shared behavior: when the body deserializes to
nulland empty input is not allowed:null, add a model-state error using the existing message provider:context.Metadata.ModelBindingMessageProvider.ValueMustNotBeNullAccessor("null")InputFormatterResult.Failure().InputFormatterResult.NoValue().System.Text.Json formatter (
SystemTextJsonInputFormatter):DeserializeAsyncreturnsnull, so for small payloads (<= 32 bytes) the formatter enables buffering and re-reads the body text to distinguish"null"from whitespace.Newtonsoft formatter (
NewtonsoftJsonInputFormatter):jsonReader.TokenTypeafter deserialization and treatJsonToken.Nullas thenull-token case.Tests
ReadAsync_WithNullJsonInput_WhenNotAllowingEmptyInput_ReturnsFailureWithDistinctErrorinJsonInputFormatterTestBase."null"now yieldsHasError == truewhen empty input isn't allowed.📄 Diff
✅ Attempt 3: PASS
Attempt 3 Approach
Use the formatter's existing model-binding message provider path for null values instead of returning
NoValue.nulland empty input is not allowed, add a model-state error viaValueMustNotBeNullAccessor("null").InputFormatterResult.Failure()soBodyModelBinderdoes not emit the generic non-empty-body error.📄 Diff
This approach avoids the misleading "non-empty request body" path by surfacing a null-value validation error directly from the input formatter. It reuses existing message-provider infrastructure (
ValueMustNotBeNullAccessor) and does not require new APIs or content-length heuristics.⚪ Attempt 4: UNKNOWN
Attempt 4: Override ReadAsync in formatters
Model: gemini-3-pro-preview
Approach
ReadAsync(notReadRequestBodyAsync) in both formattersReadAsyncreturnsNoValuebut the request body was actually non-emptyFailure()JsonInputFormatter_ValueMustNotBeNullresource stringKey Difference from Other Attempts
ReadAsynclevel (wrapper around the base behavior) rather than insideReadRequestBodyAsyncFiles Changed
📄 Diff