Skip to content

[SourceGen] Additional $return case#2111

Closed
satvu wants to merge 2 commits intomainfrom
satvu/add-http-case
Closed

[SourceGen] Additional $return case#2111
satvu wants to merge 2 commits intomainfrom
satvu/add-http-case

Conversation

@satvu
Copy link
Copy Markdown
Member

@satvu satvu commented Nov 30, 2023

Issue describing the changes in this PR

Case missed in PR #2098

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

While it is recommended to use this pattern for an HttpTrigger function with multiple output bindings, the following code is still valid and works in the legacy generator:

  [Function("Function2")]
  [QueueOutput("myqueue", Connection = "Con")]
  public HttpResponseData Run2([HttpTrigger(AuthorizationLevel.Function, "get", "post")] HttpRequestData req)
  {
      var response = req.CreateResponse(HttpStatusCode.OK);
      response.Headers.Add("Content-Type", "text/plain; charset=utf-8");

      response.WriteStringAsync("Welcome to Azure Functions!");

      return response;
  }

It produces the following metadata:

  {
    "name": "Function2",
    "scriptFile": "LegacyGeneratorTestApp.dll",
    "entryPoint": "FunctionApp4.Function2.Run2",
    "language": "dotnet-isolated",
    "properties": {
      "IsCodeless": false
    },
    "bindings": [
      {
        "name": "req",
        "direction": "In",
        "type": "httpTrigger",
        "authLevel": "Function",
        "methods": [
          "get",
          "post"
        ],
        "properties": {}
      },
      {
        "name": "$return",
        "direction": "Out",
        "type": "queue",
        "queueName": "myqueue",
        "connection": "Con",
        "properties": {}
      }
    ]
  }

Updating the source generator to reflect/include this case.

@satvu satvu requested review from kshyju and liliankasem and removed request for liliankasem November 30, 2023 19:42
@satvu satvu requested a review from liliankasem November 30, 2023 19:45
@fabiocav
Copy link
Copy Markdown
Member

Is this even a valid case? What would be the expectation for what gets persisted in the queue here? Looking at the code would lead me to believe it would be the serialized HttpResponseData

@satvu
Copy link
Copy Markdown
Member Author

satvu commented Nov 30, 2023

I didn't think it was a valid case, but when I tested it against the legacy generator it does not produce any errors on generation or startup - as discussed offline, will table this for validation and potentially raise an error for this pattern in both generation methods.

@satvu satvu closed this Nov 30, 2023
@satvu satvu deleted the satvu/add-http-case branch March 11, 2024 20:59
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.

3 participants