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

Add ShortCircuitAttribute #52571

Closed
wants to merge 3 commits into from
Closed

Add ShortCircuitAttribute #52571

wants to merge 3 commits into from

Conversation

fvoronin
Copy link
Contributor

@fvoronin fvoronin commented Dec 4, 2023

Fixes #49181

@fvoronin fvoronin requested review from a team and javiercn as code owners December 4, 2023 17:30
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 4, 2023
@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 Dec 4, 2023
@ghost
Copy link

ghost commented Dec 4, 2023

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

@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware and removed area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions labels Dec 4, 2023
Kahbazi
Kahbazi reviewed Dec 6, 2023
View reviewed changes
src/Http/Routing/src/ShortCircuit/ShortCircuitMetadata.cs Outdated
@@ -3,7 +3,7 @@

namespace Microsoft.AspNetCore.Routing.ShortCircuit;

internal sealed class ShortCircuitMetadata
internal sealed class ShortCircuitMetadata : IShortCircuitMetadata
Copy link
Member

Choose a reason for hiding this comment

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

There' no need for IShortCircuitMetadata interface. Since ShortCircuitMetadata is internal, I suggest to remove both IShortCircuitMetadata and ShortCircuitMetadata and only have ShortCircuitAttribute and update the middleware to work with the new attribute.

src/Http/Routing/src/ShortCircuitAttribute.cs
Comment on lines +15 to +18
/// <summary>
/// Constructs an instance of <see cref="ShortCircuitAttribute"/>.
/// </summary>
public ShortCircuitAttribute() { }
Copy link
Member

@Kahbazi Kahbazi Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You can remove 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.

Had to leave it, because the internal constructor is now present.

@captainsafia
Copy link
Member

@fvoronin Thanks for opening this PR! Since this introduces new API, it'll need to go through API review. I've marked the associated issue with api-ready-for-review so that it can be reviewed. You may need to react to feedback from the review after that.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Dec 20, 2023
@fvoronin fvoronin mentioned this pull request Jan 9, 2024
Open
1 task
@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-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 bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 20, 2024
@adityamandaleeka
Copy link
Member

@fvoronin Looks like the API was approved in review. Can you please fix the merge conflict and ensure the public API matches what's here: #49181 (comment)

@fvoronin
Copy link
Contributor Author

fvoronin commented Apr 5, 2024

Looks like the API was approved in review. Can you please fix the merge conflict and ensure the public API matches what's here: #49181 (comment)

Sorry, yes, of course. Please give me a couple of days.

@mitchdenny mitchdenny added the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label May 21, 2024
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Hello. I see that you've just added Needs: Author Feedback label to this PR.
That label is for Issues and not for PRs. Don't worry, I'm going to replace it with the correct one.

@dotnet-policy-service dotnet-policy-service bot added pr: pending author input For automation. Specifically separate from Needs: Author Feedback and removed Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. labels May 21, 2024
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Hi @fvoronin.
It seems you haven't touched this PR for the last two weeks. To avoid accumulating old PRs, we're marking it as stale. As a result, it will be closed if no further activity occurs within 4 days of this comment. You can learn more about our Issue Management Policies here.

@dotnet-policy-service dotnet-policy-service bot added the stale Indicates a stale issue. These issues will be closed automatically soon. label May 31, 2024
@fvoronin
Copy link
Contributor Author

fvoronin commented Jun 2, 2024

I asked a question in the original issue (#49181 (comment)), but no activity there yet.

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

@Kahbazi Kahbazi Kahbazi left review comments

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

@captainsafia captainsafia Awaiting requested review from captainsafia

Assignees

@adityamandaleeka adityamandaleeka

Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware 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 pr: pending author input For automation. Specifically separate from Needs: Author Feedback stale Indicates a stale issue. These issues will be closed automatically soon.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Short circuit attribute
7 participants
@fvoronin @captainsafia @adityamandaleeka @Kahbazi @mitchdenny @amcasey @wtgodbe