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.

Sign up for GitHub

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

Jump to bottom

Use IProblemDetailService in ValidationProblem #51464

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Use IProblemDetailService in ValidationProblem #51464

wants to merge 3 commits into from

Conversation

dnperfors
Copy link
Contributor

Use IProblemDetailService in ValidationProblem

ValidationProblem should use IProblemDetailService so that project wide customizations can be done

Description

This is basically a modified version of #47100

Fixes #47631

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Oct 18, 2023
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 18, 2023
@ghost
Copy link

ghost commented Oct 18, 2023

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

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 26, 2023
@amcasey
Copy link
Member

amcasey commented Oct 27, 2023

Seems like a faithful port of the previous PR. Could/should JsonHttpResult<ProblemDetails> as well?

@amcasey
Copy link
Member

amcasey commented Oct 27, 2023

/azp run

(Failures look related to node download issues)

@azure-pipelines
Copy link

Command 'run

(Failures' is not supported by Azure Pipelines.



Supported commands

  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@amcasey
Copy link
Member

amcasey commented Oct 27, 2023

/azp run

@ghost ghost removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 27, 2023
@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@dnperfors
Copy link
Contributor Author

Seems like a faithful port of the previous PR. Could/should JsonHttpResult<ProblemDetails> as well?

Good catch, will add that after the weekend.

captainsafia
captainsafia approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I'm OK with factoring out the work to update how JsonHttpResult<ProblemDetails> is handled to a different PR.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Nov 7, 2023
davidfowl
davidfowl reviewed Nov 7, 2023
View reviewed changes
src/Http/Http.Results/src/ValidationProblem.cs

HttpResultsHelper.Log.WritingResultAsStatusCode(logger, StatusCode);
httpContext.Response.StatusCode = StatusCode;

return HttpResultsHelper.WriteResultAsJsonAsync(
if (problemDetailsService is null || !await problemDetailsService.TryWriteAsync(new() { HttpContext = httpContext, ProblemDetails = ProblemDetails }))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The performance regression here is a little unfortunate:

  • async state machine
  • ProblemsDetailsContext object

Is there no way to optimize this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, I have not much experience in optimising asynchronous calls area...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidfowl I don't see any synchronous APIs on IProblemDetailService.

I imagine http results are very hot code, but this should be scoped to a particular class of failure results, shouldn't it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidfowl Any further thoughts on this one?

@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment Feb 13, 2024
@dotnet dotnet deleted a comment Feb 13, 2024
@dotnet-policy-service dotnet-policy-service bot added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 13, 2024
@wtgodbe wtgodbe removed the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 13, 2024
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidfowl davidfowl davidfowl left review comments

@adityamandaleeka adityamandaleeka adityamandaleeka left review comments

@amcasey amcasey amcasey left review comments

@captainsafia captainsafia captainsafia approved these changes

@Tratcher Tratcher Awaiting requested review from Tratcher

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

Assignees

@adityamandaleeka adityamandaleeka

Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use IProblemDetailsService in ValidationProblem
6 participants
@dnperfors @amcasey @davidfowl @adityamandaleeka @captainsafia @wtgodbe