dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.37k stars 9.99k forks source link

Controller [FromBody] argument is null if project references "System.Data.SqlClient 4.1.0" #1605

Closed another-guy closed 8 years ago

another-guy commented 8 years ago

If my web api project references NuGet package System.Data.SqlClient 4.1.0 then a controller defined in the same project can't retrieve an argument passed in request body. It took me a while to find a diff (see below) that shows what code change introduces the bug in my project.

Here's a controller code I have

public class SqlTaskPersistence : ITaskPersistence
{
    public Task Save(OneLineTask oneLineTask) // <- This oneLineTask argument is always null :(
    {

Of course I have some service Startup tweaks (e.g. Dependency Injection configuration). I can provide this code too if needed. Is there a way for me to verify there are no errors/exceptions while service is being started? At the moment I test it only on a dev machine.

Could you give me a hint about what I am doing wrong?

bug

rynowak commented 8 years ago

Can you please include your controller code (ok to omit the method bodies) and some information about what the request looks like? If this is in a public repo, just give us a pointer and we'll take a look.

another-guy commented 8 years ago

@rynowak Here you go sir. The following commit fixes the [FromBody] argument = null issue:

https://github.com/another-guy/TheNextOne/commit/684ac93e75a4de7b7580835b61c252bdf1b0bb28

To test the API locally you can execute the following request in Fiddler/Postman:

Verb + url: POST http://localhost:5001/tasks Headers: Content-Type:application/json Body:

{
"id": "some id",
"description": "some description",
"created": "2014-01-01T02:00:00.123Z",
"status": "some status"
}
rynowak commented 8 years ago

Thanks, I gave this a try and was able to see what you were seeing.

The issue here is that there was a breaking change between the 1.0.0-RC2 and 1.0.0 release to ValueTask<T> that impacted Kestrel. Referencing "System.Data.SqlClient": "4.1.0" pulls a framework package that isn't compatible with RC2 Kestrel and causes the break.

To fix this you can either upgrade your ASP.NET binaries to 1.0.0 or use "System.Data.SqlClient": "4.1.0-rc2-24027" for the time being (this is the release of SqlClient that corresponds with our RC2.

As far as why this isn't failing in a better way, I'm going to open an issue for this on MVC. The error coming from Kestrel was a MethodMissingException, but because it was bubbling up for a formatter, it was getting turned into a model state error. We need to reevaluate this.

another-guy commented 8 years ago

@rynowak Thanks! I made my project use the rc2 version of System.Data.SqlClient and it indeed worked well for me.

It is funny, I saw the error in ModelState while debugging. I was unable to relate this error to a null reference issue I observed. Thank you for opening #4920. I hope improving this code will allow other developers to see their projects following the Fail Fast principle.

codeConcussion commented 8 years ago

I'm running into something similar. I upgraded from RC2 to Release and a weird error appeared. I post json to a controller method and [FromBody] works fine the first time. But with subsequent calls (that are exactly identical) I get a null value for the [FromBody] argument.

It appears to be Kestrel related because it works fine running locally with IISExpress or on our test server under IIS. Ideas?

rynowak commented 8 years ago

@codeConcussion did you look in ModelState? Are there any exceptions in there? Try dumping the contents to a log or console.

codeConcussion commented 8 years ago

@rynowak Just looked at it. No errors in ModelState. None on the first call that works (obviously) and none on the second call that fails.

It could be a completely different issue or just some boneheaded mistake I'm making but it sure seems strange. I'll try to create a slimmed down project from scratch and see if I can recreate it.

rynowak commented 8 years ago

If you can try to create a repro we'd be happy to take a look.

codeConcussion commented 8 years ago

@rynowak - While creating the repro project I discovered the issue. It's unrelated to this issue even though the end result is the same.

We wrote middleware that is reading the HttpContext.Request.Body stream. The stream is a Microsoft.AspNetCore.Server.Kestrel.Internal.Http.FrameRequestStream which you can't reset (context.Request.Body.Position = 0 or context.Request.Body.Seek(0, SeekOrigin.Begin)) so instead we were just setting the body to a new MemoryStream which seemed to work. Now it causes issues - works the first time but not after.

I also tried doing context.Request.Body.CopyTo(newMemoryStream) and then reading the stream I copied it to instead of directly reading the request body, but that also causes the same issue.

Any suggestions on how to correctly read the request body's stream?

davidfowl commented 8 years ago

See https://github.com/aspnet/KestrelHttpServer/issues/940. You can work around it by resetting the stream back to the original after the response is written.

rynowak commented 8 years ago

@codeConcussion you can see an example here, https://github.com/aspnet/HttpAbstractions/blob/3e69df87f89601a5ede32c6765b736ab922b8fee/src/Microsoft.AspNetCore.Http/Internal/BufferingHelper.cs

but as @davidfowl pointed out, you will currently have a bad time if you don't reset the stream to the original stream when unwinding.

@Tratcher did we ship the buffering middleware?

Tratcher commented 8 years ago

@rynowak 0.1.0 only: https://www.nuget.org/packages/Microsoft.AspNetCore.Buffering/