search

Azure / azure-functions-python-library

Azure Functions Python SDK
MIT License
151 stars 63 forks source link

Add missing date/time formats for Servicebus #95

Closed davidmrdavid closed 2 years ago

davidmrdavid commented 2 years ago

This is a working patch for: https://github.com/Azure/azure-functions-python-worker/issues/897

In short, it appears that the Python worker does not support some date and timedelta string formats that ServiceBus supports. This PR is the result of looking at the runtime data sent by the reproducer for the IcM referenced in the link above and manually adding in the missing date/timedelta string representations. It would be good to get the opinion of our NodeJS worker maintainers to understand the following:

(1) Are we still missing any other date time / timedelta formats compared to the node worker? (2) Where does the node worker handle the parsing of these dates? I couldn't find that information myself.

After getting the answers to the questions above, we can look into adding a few test cases to this PR. Thanks!

github-actions[bot] commented 2 years ago

:white_check_mark: Result of Pytest Coverage

AnatoliB commented 2 years ago

@ejizba, @alrod, please help answering the questions on the Node.js worker.

vrdmr commented 2 years ago

(1) Are we still missing any other date time / timedelta formats compared to the node worker?

This was the only parsing error that showed up in our telemetry. I have added some more formats to be future-safe.

(2) Where does the node worker handle the parsing of these dates? I couldn't find that information myself.

Node doesn't do a parsing explicitly - it leverages the JSON parsing to parse the incoming gRPC object and doesn't do any explicit parsing as such.

AnatoliB commented 2 years ago

@davidmrdavid @vrdmr Is this limited to Python Functions only, or we may need a similar fix in other OOP language workers?

davidmrdavid commented 2 years ago

@AnatoliB, I think it's worth triple-checking that this works in PowerShell and Java. As per Varad's answer, NodeJS should already be working as it leverages a different date-deserialization strategy :) .

However, I don't think testing on other OOP language workers should block the merging of this PR.

AnatoliB commented 2 years ago

However, I don't think testing on other OOP language workers should block the merging of this PR.

Sure, I completely agree with this.

@Francisco-Gamino, @amamounelsayed FYI: we may need similar fixes for PowerShell and Java.

Francisco-Gamino commented 2 years ago

However, I don't think testing on other OOP language workers should block the merging of this PR.

Sure, I completely agree with this.

@Francisco-Gamino, @amamounelsayed FYI: we may need similar fixes for PowerShell and Java.

Ack.