New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Errors when Serializing to JSON #4160

Closed
capesean opened this Issue Feb 25, 2016 · 14 comments

Comments

Projects
None yet
8 participants
@capesean

capesean commented Feb 25, 2016

I am hitting an error that I can't quite pinpoint. If you can tell me how to debug it better I'll gladly provide the details. This was working fine with EF7 before the change to EntityFrameworkCore 1.0, but now I'm getting these two types of errors. I don't think it's an EF issue because the data gets returned from the database fine, it seems to error when it gets serialized in the WebApi. Although maybe it's an issue with Json.Net.

I have an EF model of clients that have projects. A project has a Project Manager, a Project Type, (etc) as related entities.

When I query the API endpoint to list the projects, I run this code:

return Ok(
    DbContext.Projects
        .Include(p => p.Client)
        .Include(p => p.ProjectManager)
        .Include(p => p.ProjectType)
    );

I get the error:

HTTP Error 502.3 - Bad Gateway
The specified CGI application encountered an error and the server terminated the process.

Most likely causes:
The CGI application did not return a valid set of HTTP errors.
A server acting as a proxy or gateway was unable to process the request due to an error in a parent gateway.

If I remove the line .Include(p => p.Client) then I get a different issue: the json gets truncated after about 259 projects. If I put a .Take(258) in there, then it works fine.

It looks like 2 different issues - the CGI error and the truncating error. As mentioned, this worked a few days ago before I upgraded to AspNetCore and EntityFrameworkCore.

I'm not sure how to get more error information out but if direct me I'll do it.

@capesean

This comment has been minimized.

Show comment
Hide comment
@capesean

capesean Feb 26, 2016

This appears to be an issue with Entity Framework.

capesean commented Feb 26, 2016

This appears to be an issue with Entity Framework.

@capesean

This comment has been minimized.

Show comment
Hide comment
@capesean

capesean Feb 26, 2016

It's not entirely an EF issue. Yes, there is a circular reference issue in EF, but if I do JsonConvert.SerializeObject on the EF result and then send that string back, it works fine. But if I just do Ok(queryresults) then the result is truncated.

capesean commented Feb 26, 2016

It's not entirely an EF issue. Yes, there is a circular reference issue in EF, but if I do JsonConvert.SerializeObject on the EF result and then send that string back, it works fine. But if I just do Ok(queryresults) then the result is truncated.

@gdoron

This comment has been minimized.

Show comment
Hide comment
@gdoron

gdoron Feb 26, 2016

I had something similar, try setting this in the Configuration method in the Startup class:

services.AddMvc().AddJsonOptions(options => {
                options.SerializerSettings.ReferenceLoopHandling = ReferenceLoopHandling.Ignore;
            });

gdoron commented Feb 26, 2016

I had something similar, try setting this in the Configuration method in the Startup class:

services.AddMvc().AddJsonOptions(options => {
                options.SerializerSettings.ReferenceLoopHandling = ReferenceLoopHandling.Ignore;
            });
@gdoron

This comment has been minimized.

Show comment
Hide comment
@gdoron

gdoron Feb 26, 2016

@Eilon shouldn't the default needs to be changed?
Since if the default value comes into play, it will throw an exception...

gdoron commented Feb 26, 2016

@Eilon shouldn't the default needs to be changed?
Since if the default value comes into play, it will throw an exception...

@Eilon

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Feb 26, 2016

Member

I think the default value of not ignoring reference loops is correct because you want to avoid serializing things that have loops to begin with. Changing the default would likely mask problems with the data being serialized, no?

Member

Eilon commented Feb 26, 2016

I think the default value of not ignoring reference loops is correct because you want to avoid serializing things that have loops to begin with. Changing the default would likely mask problems with the data being serialized, no?

@JamesNK

This comment has been minimized.

Show comment
Hide comment
@JamesNK

JamesNK Feb 27, 2016

Member

Erroring when a loop is encountered is standard behavour for serializers.

Options are there to fix the error (including updating the serialized object). I don't think you should pick one for devs.

Member

JamesNK commented Feb 27, 2016

Erroring when a loop is encountered is standard behavour for serializers.

Options are there to fix the error (including updating the serialized object). I don't think you should pick one for devs.

@OlegKi

This comment has been minimized.

Show comment
Hide comment
@OlegKi

OlegKi Feb 27, 2016

@Eilon: I find changing of default for the best way. The usage of .Include is the standard way which suggest Entity Framework. The most people just follow the way and then spend a lot of his time in finding the origin of "HTTP Error 502.3 - Bad Gateway" error.

@JamesNK : I could agree with you if the initial model were oriented on serialization. The developer want just include some field which could be get by INNER JOIN and so he uses .Include. As the result he get "HTTP Error 502.3 - Bad Gateway" error, which gives absolutely no information about the origin of the problem.

Thus, in my personal opinion, changing of defaults would be make Entity Framework more user friendly, especially for newcomers and especially if one unable to report the error with the exact reason on the problem.

OlegKi commented Feb 27, 2016

@Eilon: I find changing of default for the best way. The usage of .Include is the standard way which suggest Entity Framework. The most people just follow the way and then spend a lot of his time in finding the origin of "HTTP Error 502.3 - Bad Gateway" error.

@JamesNK : I could agree with you if the initial model were oriented on serialization. The developer want just include some field which could be get by INNER JOIN and so he uses .Include. As the result he get "HTTP Error 502.3 - Bad Gateway" error, which gives absolutely no information about the origin of the problem.

Thus, in my personal opinion, changing of defaults would be make Entity Framework more user friendly, especially for newcomers and especially if one unable to report the error with the exact reason on the problem.

@gdoron

This comment has been minimized.

Show comment
Hide comment
@gdoron

gdoron Feb 27, 2016

@Eilon I agree that if you pass an entity with reference loop to a view you're doing something wrong because you're not using ViewModel/DTO.
But unfortunately from the places I worked at (and from Stackoverflow questions) most developers do not use DTOs, and in matter of fact even the scaffolding provided by MS for creating new Controller do not use DTOs...

gdoron commented Feb 27, 2016

@Eilon I agree that if you pass an entity with reference loop to a view you're doing something wrong because you're not using ViewModel/DTO.
But unfortunately from the places I worked at (and from Stackoverflow questions) most developers do not use DTOs, and in matter of fact even the scaffolding provided by MS for creating new Controller do not use DTOs...

@Eilon

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Mar 7, 2016

Member

Using view models and DTOs is definitely recommended. It's really not a goal for MVC to allow general serialization of EF data models. The reality is that there's effectively a bug in the app, and the bug needs to be fixed, not ignored.

Completely ignoring reference loops is a Very Bad IdeaTM because it just means the serialized data is missing stuff and you can't predict where. That's not good.

Member

Eilon commented Mar 7, 2016

Using view models and DTOs is definitely recommended. It's really not a goal for MVC to allow general serialization of EF data models. The reality is that there's effectively a bug in the app, and the bug needs to be fixed, not ignored.

Completely ignoring reference loops is a Very Bad IdeaTM because it just means the serialized data is missing stuff and you can't predict where. That's not good.

@Eilon Eilon closed this Mar 7, 2016

@gdoron

This comment has been minimized.

Show comment
Hide comment
@gdoron

gdoron Mar 7, 2016

@Eilon I agree, but the scaffolding and templates should be adjusted to use DTOs (AutoMapper is probably a good idea).
And if possible, changing the error message you get if you if you pass a model with a reference loop from HTTP Error 502.3 - Bad Gateway to something a little bit more reasonable, otherwise once ASP.NET Core will go public, it will be the most asked question on Stack Overflow... 😆

gdoron commented Mar 7, 2016

@Eilon I agree, but the scaffolding and templates should be adjusted to use DTOs (AutoMapper is probably a good idea).
And if possible, changing the error message you get if you if you pass a model with a reference loop from HTTP Error 502.3 - Bad Gateway to something a little bit more reasonable, otherwise once ASP.NET Core will go public, it will be the most asked question on Stack Overflow... 😆

@Eilon

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Mar 7, 2016

Member

@gdoron issues with the templates can be logged here: https://github.com/aspnet/Templates/

However, one thing that's been a constant challenge for us when making the templates is balancing out how easy it is to understand vs. how "real world" something is. Using view models, binding models, and serialization models is definitely more powerful, but it's a large maintenance cost for an app, and more difficult for someone starting to understand (e.g. "how come when I add/change a property in my model I have to do it in 4 places??").

Regarding the Bad Gateway error that's a really tough one: the problem is that you can't know about the error until it's too late, unless you buffer the entire response (which could be several GB or more!).

Member

Eilon commented Mar 7, 2016

@gdoron issues with the templates can be logged here: https://github.com/aspnet/Templates/

However, one thing that's been a constant challenge for us when making the templates is balancing out how easy it is to understand vs. how "real world" something is. Using view models, binding models, and serialization models is definitely more powerful, but it's a large maintenance cost for an app, and more difficult for someone starting to understand (e.g. "how come when I add/change a property in my model I have to do it in 4 places??").

Regarding the Bad Gateway error that's a really tough one: the problem is that you can't know about the error until it's too late, unless you buffer the entire response (which could be several GB or more!).

@datumgeek

This comment has been minimized.

Show comment
Hide comment
@datumgeek

datumgeek May 21, 2017

The fix (ignore loops) worked great for me

my scenario: Web API with EF Core - Returning child data along with parent (via Include)

quite common, i'm guessing...

Ran into the issue described above. Didn't get an error, just no response from the web api.

Happened to stumble upon this thread.

Added the fix (ignore loops).

Worked like a champ !!!

datumgeek commented May 21, 2017

The fix (ignore loops) worked great for me

my scenario: Web API with EF Core - Returning child data along with parent (via Include)

quite common, i'm guessing...

Ran into the issue described above. Didn't get an error, just no response from the web api.

Happened to stumble upon this thread.

Added the fix (ignore loops).

Worked like a champ !!!

@alexlomba87

This comment has been minimized.

Show comment
Hide comment
@alexlomba87

alexlomba87 May 22, 2018

I agree with @Eilon, but this bug is still unresolved as to date, and two years have passed.
The issue happens even when literally following Microsoft's own tutorials for ASP Core beginners. This should not happen. I myself, learning ASP from scratch, I have lost several hours to understand what was the issue and then come here. The "fix" (ignore loops) worked for me, but I agree it cannot be regarded as a long term solution.

alexlomba87 commented May 22, 2018

I agree with @Eilon, but this bug is still unresolved as to date, and two years have passed.
The issue happens even when literally following Microsoft's own tutorials for ASP Core beginners. This should not happen. I myself, learning ASP from scratch, I have lost several hours to understand what was the issue and then come here. The "fix" (ignore loops) worked for me, but I agree it cannot be regarded as a long term solution.

@Fieel

This comment has been minimized.

Show comment
Hide comment
@Fieel

Fieel Jul 25, 2018

@gdoron solution is a bad practice because we'd rather avoid circular dependency. I just want my object and its children propreties, simple as that.

Ignoring the circular loop and adding the option in the startup.cs file is not a solution at all, it's just a workaround..

I'm kinda shocked because i'm learning the framework and stumbling upon critical bugs such as this one. As @alexlomba87 noticed, the bug is present and ignored in most .NET Core tutorials and documentation pages. I just lost one entire day at work trying to fix this 👎 and there is no solution

Fieel commented Jul 25, 2018

@gdoron solution is a bad practice because we'd rather avoid circular dependency. I just want my object and its children propreties, simple as that.

Ignoring the circular loop and adding the option in the startup.cs file is not a solution at all, it's just a workaround..

I'm kinda shocked because i'm learning the framework and stumbling upon critical bugs such as this one. As @alexlomba87 noticed, the bug is present and ignored in most .NET Core tutorials and documentation pages. I just lost one entire day at work trying to fix this 👎 and there is no solution

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