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

[BUG FIX] HTTP Trigger Functions with Multiple Output Bindings #2322

Merged
merged 25 commits into from
Apr 5, 2024

Conversation

satvu
Copy link
Member

@satvu satvu commented Mar 5, 2024

Issue describing the changes in this PR

resolves #2079

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)

Additional information

Currently, function metadata generation in both legacy and source-generation have special handling of HttpResponseData types in multiple output binding scenarios. Neither of these generators handle new types introduced in ASP.NET Core Integration (like IActionResult).

This PR introduces the following features:

  • Introduction of HttpResponseOutputAttribute
    • Using this attribute on the intended HTTP response object will signal to generators to correctly generate function metadata
    • Explicit labeling will make the addition of new types easier and more regression-proof
  • Handling of new attribute in source generator
  • ASP.NET Core Integration package changes
  • Introduction of TryGetHttpResponse<T>
  • Refactoring to handle http response types in multiple output binding scenarios (where http is not a $return binding and has to be searched for inside of the output bindings list instead).
  • Samples update

Not included:

  • Fix in legacy generator (will follow up in separate PR).

@satvu satvu marked this pull request as ready for review March 6, 2024 19:35
Copy link
Contributor

@mattchenderson mattchenderson left a comment

Choose a reason for hiding this comment

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

My main request is for changes to the sample so it can be leveraged in the docs.

samples/AspNetIntegration/MultipleOutputBindings.cs Outdated Show resolved Hide resolved
sdk/Sdk.Generators/Constants.cs Outdated Show resolved Hide resolved
@satvu satvu requested review from fabiocav and kshyju March 27, 2024 17:40
@mattchenderson
Copy link
Contributor

Just confirming - the sample changes have been dropped from this PR, correct? For docs, should I plan on an inline revision then?

@satvu
Copy link
Member Author

satvu commented Mar 27, 2024

Just confirming - the sample changes have been dropped from this PR, correct? For docs, should I plan on an inline revision then?

For now, yes - the sample with the docsnippet will be added after the SDK is released and we can update the sample app reference.

@fabiocav
Copy link
Member

I've also pushed some changes to the middleware to help with some of the issues. Done a bit quickly, so the tests mentioned and a review, would be good.

sdk/release_notes.md Outdated Show resolved Hide resolved
sdk/release_notes.md Outdated Show resolved Hide resolved
@satvu satvu requested a review from fabiocav April 4, 2024 22:22
@mattchenderson mattchenderson dismissed their stale review April 5, 2024 20:52

Doc request no longer relevant

@satvu satvu merged commit de33408 into main Apr 5, 2024
22 checks passed
@satvu satvu deleted the satvu/poco-return-metadata branch April 5, 2024 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using Multiple Output Bindings with ASP.NET Core Integration
5 participants