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

Implement the Mvc PushFileStreamResult API #58161

Closed
wants to merge 1 commit into from

Conversation

0xced
Copy link
Contributor

@0xced 0xced commented Sep 30, 2024

Mvc PushFileStreamResult API

  • 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.

Description

This pull request implements PushFileStreamResult as described in #39383 along with the related File overloads on the ControllerBase class and the PushFileStreamResultExecutor.

Fixes #39383

@0xced 0xced requested a review from a team as a code owner September 30, 2024 18:42
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Sep 30, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 30, 2024
@0xced 0xced force-pushed the Mvc-PushFileStreamResult branch from a1440b9 to 944b437 Compare October 1, 2024 11:21
@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 Oct 10, 2024
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.

Thanks for taking the time to work on this, @0xced!

This looks good overall. To make sure that everything's checked, can you open a new API proposal issue specifically for the MVC-related changes? I'm hoping the review can happen fairly quickly since they build on top of the IResult-based implementations but I'd like to make sure we get the right scrutiny for the new methods on ControllerBase.

}
}

[LoggerMessage(1, LogLevel.Information, "Executing {FileResultType}, sending file with download name '{FileDownloadName}' ...", EventName = "ExecutingFileResultWithNoFileName", SkipEnabledCheck = true)]
Copy link
Member

Choose a reason for hiding this comment

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

Why SkipEnabledCheck = true here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because ExecutingFileResultWithNoFileName is already surrounded by if (logger.IsEnabled(LogLevel.Information)) inside the ExecutingFileResult method, exactly like in FileContentResultExecutor.

@davidfowl davidfowl requested a review from Copilot December 17, 2024 18:01

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.

Files not reviewed (2)
  • src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt: Language not supported
  • src/Mvc/Mvc.Core/test/PushFileStreamResultTest.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)

src/Mvc/Mvc.Core/src/Infrastructure/PushFileStreamResultExecutor.cs:64

  • The 'WriteFileAsync' method should handle the 'range' and 'rangeLength' parameters correctly, ensuring they are 'null' and '0' respectively, or handle them appropriately if they are not.
Debug.Assert(range == null);
@captainsafia captainsafia added the pr: pending author input For automation. Specifically separate from Needs: Author Feedback label Jan 6, 2025
@dotnet-policy-service dotnet-policy-service bot added the stale Indicates a stale issue. These issues will be closed automatically soon. label Jan 16, 2025
@0xced
Copy link
Contributor Author

0xced commented Jan 16, 2025

Please don't close this pull request.

@0xced
Copy link
Contributor Author

0xced commented Jan 21, 2025

Can I get a human to reopen this pull request please?

@captainsafia captainsafia reopened this Jan 21, 2025
@captainsafia
Copy link
Member

captainsafia commented Jan 21, 2025

@0xced Did you get a chance to file an API review issue for this? We'll need to get the API reviewed + approved before merging.

@Rick-Anderson
Copy link
Contributor

@tdykstra please review ///comments.

@0xced
Copy link
Contributor Author

0xced commented Jan 28, 2025

Did you get a chance to file an API review issue for this? We'll need to get the API reviewed + approved before merging.

I haven't got around yet but I'd like to do it. I just have other priorities right now.

Can we please reopen this again? And maybe also tell the bot to stop closing this PR every week? This bot repeatedly closing pull requests is such hostile behaviour towards contributors. 😟

@Rick-Anderson
Copy link
Contributor

I haven't got around yet but I'd like to do it. I just have other priorities right now.

That's why the bot is closing it. Once you get an API review issue for this, and add Fixes API review issue, the bot should stop closing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates 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
Development

Successfully merging this pull request may close these issues.

Add Results.Stream overload that takes a Func<Stream, Task>
4 participants