Make problemdetails settable in problemdetailscontext

[bjornen77]

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Make ProblemDetails settable in ProblemDetailsContext

Description

The ProblemDetails property in ProblemDetailsContext is now settable which makes it possible to
replace the current ProblemDetails instance when using the customize problem details feature.

Fixes #47633

[ghost]

Thanks for your PR, @bjornen77. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[bjornen77]

How can I solve the build errors?

[mitchdenny]

Hrm interesting. I'm not sure what the correct thing to do here is to express our intention. It seems wrong to update Shipped with init changed to set, but you've already done what you would normally do when adding API.

@halter73 do you know the what the correct way to handle this is? I'm going to see if I can find the implementation logic for this check to see if I can figure it out but thought you might know the right thing to do off the top of your head.

[mitchdenny]

OK, I think I've got it. We have to REMOVE the old API in unshipped and add the new one. I've made a change to your PR with what I think should work. We should know soon enough if thats the correct way to do it ;)

For future reference, here is the analyzer that does this:

https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.cs

[bjornen77]

Nice work @mitchdenny! Thank you for your help.

[mitchdenny]

I wonder what behavioural tests we need to cover this. Perhaps a test that results in a problem using MVC that we then override using the setter and then checking it in an early stage middleware (not make sure the replaced version made it through).

[captainsafia]

Hrm interesting. I'm not sure what the correct thing to do here is to express our intention. It seems wrong to update Shipped with init changed to set, but you've already done what you would normally do when adding API.

@halter73 do you know the what the correct way to handle this is? I'm going to see if I can find the implementation logic for this check to see if I can figure it out but thought you might know the right thing to do off the top of your head.

If you're in Visual Studio, there should be a codefix that lights up to automatically update the PublicAPI.txt files with the delta. If that isn't working, there might be something suspicious afoot.

I wonder what behavioural tests we need to cover this. Perhaps a test that results in a problem using MVC that we then override using the setter and then checking it in an early stage middleware (not make sure the replaced version made it through).

Adding a test in this class and in this one that uses a setter and validates the behavior of the WriteAsync call is probably sufficient here.

[bjornen77]

If you're in Visual Studio, there should be a codefix that lights up to automatically update the PublicAPI.txt files with the delta. If that isn't working, there might be something suspicious afoot.

@captainsafia I used the codefix you mentioned in Visual Studio and it added the new API in PublicApi.Unshiped.txt and the warning/code fix in Visual Studio went away. But then when I created the PR I got the build errors.

[bjornen77]

Adding a test in this class and in this one that uses a setter and validates the behavior of the WriteAsync call is probably sufficient here.

@captainsafia I think that both your links points to the same class?

[captainsafia]

Adding a test in this class and in this one that uses a setter and validates the behavior of the WriteAsync call is probably sufficient here.

@captainsafia I think that both your links points to the same class?

My bad, it should have been a link to https://github.com/dotnet/aspnetcore/blob/main/src/Http/Http.Extensions/test/ProblemDetailsDefaultWriterTest.cs.

If you're in Visual Studio, there should be a codefix that lights up to automatically update the PublicAPI.txt files with the delta. If that isn't working, there might be something suspicious afoot.

@captainsafia I used the codefix you mentioned in Visual Studio and it added the new API in PublicApi.Unshiped.txt and the warning/code fix in Visual Studio went away. But then when I created the PR I got the build errors.

Ah, I believe you've run into a bug wherein APIs that are removed are not correctly documented by the PublicAPI analyzer. I believe there is an issue for this somewhere...

[bjornen77]

My bad, it should have been a link to https://github.com/dotnet/aspnetcore/blob/main/src/Http/Http.Extensions/test/ProblemDetailsDefaultWriterTest.cs.

Thanks @captainsafia!

[bjornen77]

Tests are now added.

[mitchdenny]

Thanks for your contribution @bjornen77!

[ghost]

Hi @mitchdenny. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.