Azure / azure-functions-durable-js

JavaScript library for using the Durable Functions bindings
https://www.npmjs.com/package/durable-functions
MIT License
128 stars 46 forks source link

Incorrect ReturnType of `DurableOrchestrationClient.getStatus` #487

Closed christianemmert closed 1 year ago

christianemmert commented 1 year ago

Resolves #486

christianemmert commented 1 year ago

@microsoft-github-policy-service agree

hossam-nasr commented 1 year ago

Hi @christianemmert ,

Thank you so much for your contribution!

As others have pointed out, this would be a breaking change, so this change would have to be done on the new preview v3.x version. This is definitely a bugfix that we would like to see happen in v3.x, but I am uncomfortable accepting this change as-is without more manual testing and investigation. This PR would throw an error any time the client receives a 400, 404, or 500 response. But, there are still cases where those status codes contain valid DurableOrchestrationStatus objects that we should pass back to the user. We receive 400 when the instance failed or terminated, 404 when the instance is pending, and 500 when the instance failed with an unhandled exception.

There are two parts I see here:

  1. The SDK should throw an error when there's an empty response for whatever reason. This should guard us against potential unexpected behavior from the extension and still live up to the promise of always returning a DurableOrchestrationStatus
  2. That alone may not be the bugfix we seek, since it's possible the extension should be returning a valid DurableOrchestrationStatus, but isn't for some reason. So we need to investigate the cases in which an empty response is returned, and identify if that's expected behavior or a bug that should be fixed on the extension side.

Based on this, I'll go ahead and close this PR. The good news is that with the preview announcement of v3.x today, we should have more time to investigate this further and apply a fix!