Catch JSON deserialization exceptions and use them to return 400 errors

[jeffw-wherethebitsroam]

This code adds the JSON deserialization exception to
GraphQLRequestDeserializationResult and sets IsSuccessful = false.
Also, unmatched content-type should return 415 error code: Unsupported Media Type

[jeffw-wherethebitsroam]

This addresses some of the issues from #580

[Shane32]

Looks good to me, but @sungam3r should review.

[sungam3r]

Need small changes in the API design. I'm OK to merge after that.

[SK201]

Hello,
We are currently affected by this issue and would love to see the PR merged.

Thanks in advance !

[jeffw-wherethebitsroam]

I was waiting on feedback on how to propagate the exceptions and then forgot about this. I can update the PR :)

[Shane32]

Thanks @jeffw-wherethebitsroam ! Did the API surface change? We might need the apiapprovals file updated. Also it looks like it is failing the formatting check.

[Shane32]

@jeffw-wherethebitsroam I found the issue with the formatting tool; if you can merge master back into this branch, then it should run the CI tests properly.

[jeffw-wherethebitsroam]

@Shane32 I've merged master. The API surface did change (removed IsSuccessful from GraphQLRequestDeserializationResult, added GraphQLRequestDeserializationException), but the apiapprovals file is updated.

[jeffw-wherethebitsroam]

@Shane32 It looks like I need a maintainer to approve the workflows: "First-time contributors need a maintainer to approve running workflows"

[Shane32]

I think these changes look great! However, it contains breaking changes in the API surface, and has breaking changes to the semantics of the current request parsing methods. It really belongs in the next major version, I would think.

To make this a non-breaking change, we could retain the IsSuccessful property and add another Exception property of type GraphQLRequestDeserializationException, or null if no error. This really would perform identically, even though it's not as clean of a design. Then the decision would become if we should clean up the design for the next major version or not.

The remainder of the semantic changes, such as returning the proper status code upon failure, would only necessitate a bump to the minor version number.

There is one other alternative to getting these changes in the current version, and that would be to leave the IsSuccessful property there, but always return true, and mark it as obsolete. The property would need to be monitored in the middleware in case someone is using a custom JSON transport that returns false for that property. I don't really care for this idea.

Thoughts?

[jeffw-wherethebitsroam]

Ok, we're now pretty much back to the original version :) Its a bit uglier now since IsSuccessful = false and Exception != null both mean that the deserialization failed and either or both can be set. But it would be better to clean that up in the next major release.

[Shane32]

I just have so little time to spend on the next version, and there’s a whole lot of cleanup that could be done. Better to get this fix in the current version.

[sungam3r]

@jeffw-wherethebitsroam Thanks for this fix. Just reviewed it. LGTM, just a few cosmetic changes here. I had a long break from OSS this year but I returned. I think that when I read the remaining emails for this repo (~60 left), then I can release v5.1 including this fix (and others of course).

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 97.43590% with 1 line in your changes missing coverage. Please review.

Project coverage is 55.51%. Comparing base (63ed5a0) to head (7a2407c).
Report is 300 commits behind head on master.

Files with missing lines	Patch %	Lines
src/Transports.AspNetCore/GraphQLHttpMiddleware.cs	92.30%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #581      +/-   ##
==========================================
+ Coverage   54.84%   55.51%   +0.67%     
==========================================
  Files          63       64       +1     
  Lines        1506     1531      +25     
  Branches      152      154       +2     
==========================================
+ Hits          826      850      +24     
  Misses        627      627              
- Partials       53       54       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.