Include empty entries when building InvocationRequest payload for OOP workers

[kshyju]

Fixes #8499

The current behavior is to skip entries which are null or empty. This causes issues like this. So we decided to introduce a new capability called IncludeEmptyEntriesInMessagePayload which language workers can advertise and if it is present, we will include the empty entries (empty, not null) in the array we create for the message payload.

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 same issue. I will add once this change is in v4.
• My changes do not require diagnostic events changes
  • Otherwise: I have added/updated all related diagnostic events and their documentation (Documentation issue linked to PR)
• I have added all required tests (Unit tests, E2E tests)

Additional information

Additional PR information
Please refer #8499 which has details about the current behavior and problems caused by that.

[kshyju]

This is a new capability and none of the worker are sending it now. We will start with dotnet worker and then share the info with other language workers so that they can also opt in for this behavior.

[mathewc]

Are you saying that if we simply updated the ToRpcStringArray method to correctly add nulls with full fidelity, existing workers won't be able to handle that? Can you link to the corresponding worker code showing where the null-ref occurs?

[kshyju]

Are you saying that if we simply updated the ToRpcStringArray method to correctly add nulls with full fidelity, existing workers won't be able to handle that? Can you link to the corresponding worker code showing where the null-ref occurs?

No. I did not mean that. The fix is correcting the current incorrect behavior/bug. So probably do not need to be considered as a breaking change or so. I will remove the capability check.

[kshyju]

Latest iteration has this capability check removed.

[mathewc]

So we'll now send along empty entries, but still skip nulls. For empty entries, are we sure workers handle this w/o breaking changes? Probably need to verify with some testing for the triggers supporting cardinality many (EventHubs, ServiceBus Batches).

This will address the linked issue (empty strings) but what about nulls? Is it possible for the triggers to send nulls here? If so, in those cases, we'll have the same issue.

[wesselkranenborg]

Is it really that this cannot be null? We are using Microsoft.Azure.Devices.Client.Message without a body (there is only constructor where you don't need to specify a body). We only set properties and a messageId on this Message. That IoTHub message is then routed to this eventhub. Is the default body of an IoT Hub message string.Empty?

[kshyju]

Language worker testing is currently blocked due to this issue on python. Tested JS, TS and Dotnetworker and they look good.

[fabiocav]

Historically, any behavior change like this is based on capability advertisement. That’s the safest way if you want to expedite the fix. If you don’t want to introduce a capability, thorough testing of all language workers will be required, and that wouldn’t cover external ones other teams may be maintaining.

is there a concern about the new capability?

[kshyju]

Brought back the capability check based on offline sync. Thank you!

[wesselkranenborg]

We are now testing this bugfix in our azure function but it looks like we are still hitting this issue. Nobody also replied to my comment above...

Is it really that this cannot be null? We are using Microsoft.Azure.Devices.Client.Message without a body (there is only constructor where you don't need to specify a body). We only set properties and a messageId on this Message. That IoTHub message is then routed to this eventhub. Is the default body of an IoT Hub message string.Empty?

It looks like our payload was just null and this bugfix is only helping when you are sending string.Empty. Which Microsoft.Azure.Devices.Client.Message doesn't do by default.

CC @kshyju, @fabiocav

[madelinegordon]

nit - name seems a little disjointed? Is "Rpc collection" a single concept? (e.g. would "ToRpcCollection_String_IncludeEmptyEntriesForTypedDataCollection" make sense?)

[kshyju]

ToRpc is the method name and other tests are using this format. So I will keep it as it is for consistency.