Skip to content

Handle null and undefined values of result and result.return in invocationRequest #364

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

Closed
wants to merge 14 commits into from

Conversation

davidmrdavid
Copy link
Contributor

@davidmrdavid davidmrdavid commented Jan 22, 2021

This PR addresses this issue in the Durable JS SDK.
Previously a "falsy" return value would fail to serialize due to logic in invocationRequest that led the worker to assume that there was no return value.

This PR simply refines some if-statements to ensure we only avoid serialization when result and result.return are either null or undefined. :)

ToDos:
I was hoping to add at least one unit test to this, but I couldn't figure out a way to set result variable here by looking at the WorkerChannel tests. Any ideas, pointers, or examples I could follow? Thanks!

@davidmrdavid davidmrdavid changed the title Introduce Truthy type to mitigate "Falsy" checks Handle null and undefined values of result and result.return in invocationRequest Jan 26, 2021
@davidmrdavid davidmrdavid marked this pull request as ready for review January 26, 2021 02:15
@davidmrdavid davidmrdavid requested a review from mhoeger January 26, 2021 17:21
@mhoeger
Copy link
Contributor

mhoeger commented Jan 26, 2021

Thanks David! I would try adding a test just like this one: https://github.com/Azure/azure-functions-nodejs-worker/blob/v2.x/test/WorkerChannelTests.ts#L445-L456

Although this is definitely a bugfix, since the current behavior has been in place for so long I wonder if it counts as a breaking change and something we should be careful about? I'm trying to think about the impacts a change here may have on a customer's system, and I think it may depend on the type of binding (ex: if it's an HTTP output, honoring a "false" value might return something different or if it's a blob output maybe "0" means that no blob is output, or something like that). @fabiocav - any thoughts on this bugfix and potential breaking impact?

@davidmrdavid
Copy link
Contributor Author

@fabiocav, could we get your thoughts Marie's question above?

@fabiocav
Copy link
Member

This is indeed a breaking change and has the potential to negatively impact production workloads with a behavior change.

I agree that it is a good change, but likely behavior that would need to be put behind a flag until we have a new major version out.

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