Skip to content
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

feat: Add support for ProblemDetails extensions #788

Merged
merged 2 commits into from
Nov 18, 2019

Conversation

Syrus666
Copy link
Contributor

What kind of change does this PR introduce?
Updates the ProblemDetails to support extensions to the spec. This behaves the same as the MS implementation of ProblemDetails extensions: https://github.com/aspnet/Mvc/blob/master/src/Microsoft.AspNetCore.Mvc.Core/ProblemDetails.cs#L62

This means additional properties on the response will be populated in the new Extensions property.
I have chosen to not change the behaviour of the Errors property in case it's a breaking change for other users.

What is the current behavior?
Extension problem details extension properties are ignored

What is the new behavior?
Extension properties on the problem details response are populated in to new property

What might this PR break?
It shouldn't cause any regressions with this being additive.

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Other information:
If you'd like to discuss further I'm "syrus" on reactivex.slack.com

Syrus666 and others added 2 commits November 18, 2019 12:24
+ Updated ProblemDetails class with new property (to prevent breaking changes) that supports [JsonExtensionData]. I've not changed the Errors collection to ensure there are no unexpected changes in people's current usage of the property.
+ Added a unit test
@dnfclas
Copy link

dnfclas commented Nov 18, 2019

CLA assistant check
All CLA requirements met.

@clairernovotny
Copy link
Member

This looks good. What change were you considering for Errors?

@Syrus666
Copy link
Contributor Author

What I'd considered as an alternative was a breaking change of changing the Errors property to be [JsonExtensionData] IDictionary<string, object> rather than Dictionary<string, string[]> and using that as the property to deserialise other properties in same namespace rather than introducing a new one.

Outside of that being a naughty breaking change you could then have scenarios where the problem details responses they were currently getting were like

{
    "type": "type",
    "title": "title",
    "detail": "detail",
    "status": 500,
    "errors": { 
        "foo":[ "bar" ] 
    },
    "baz": 123
}

Which would mean even forcibly casting the values of the Errors dictionary to an array to make their code compile again would cause problems for them as post this PR Errors would've also now contained baz.

So I thought it safest to stick with the new property.

Or I could be over thinking it and no-one uses the Errors property for anything :D

@clairernovotny clairernovotny merged commit 96b11d4 into reactiveui:master Nov 18, 2019
@clairernovotny
Copy link
Member

Thanks for the explanation. I've merged this, but it would still be helpful for any doc updates that describe how the ProblemDetails works. Would be great if you could add another small PR for that?

@Syrus666 Syrus666 deleted the pd_ext branch November 18, 2019 21:52
@lock lock bot locked and limited conversation to collaborators Feb 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants