Azure / msrest-for-python

The runtime library "msrest" for AutoRest generated Python clients.
MIT License
41 stars 64 forks source link

Serialization for non-standard types yields incorrect output #223

Closed ludokriss closed 3 years ago

ludokriss commented 4 years ago

Hi,

I am using the event grid client SDK in python to generate custom events. I have come across an issue I can't seem to solve without going away from the event grid SDK.

The problem is that the event grid model serializer does not give me the correct output for when including types that are not the basic types. A simple reproducible example:

from azure.eventgrid.models import EventGridEvent
import datetime
import uuid

event=EventGridEvent(
            topic="test",
            id=uuid.uuid4(),
            subject="testUpdated",
            data={"time":datetime.datetime.now().replace(tzinfo=datetime.timezone.utc)},
            event_type="test.test",
            event_time=datetime.datetime.now().replace(tzinfo=datetime.timezone.utc),
            data_version=2.0,
)

print(event.serialize())

This would return

{'id': '3e02a22c-f327-4f62-af25-b71e0865888b', 'topic': 'product', 'subject': 'ProductUpdated', 'data': {'time': '2020-09-07 10:37:08.348679+00:00'}, 'eventType': 'supplychain.product', 'eventTime': '2020-09-07T10:37:08.348679Z', 'dataVersion': '2.0'}

the serialize is not called by me in the code I actually use, but it looks like that is what is called behind the scenes when I send off the event. I want the datetime (the key "time" in the above example) to be serialized just as the parent-level "event_time". My problem is not only with datetime as in the current example, but also with decimals.

I guess this fits in this repo and not in the EventGrid SDK repo, but feel free to redirect me there.

ludokriss commented 4 years ago

To follow up with a potential solution to the problem as well, you could implement a change in the current serialize_object to handle more types as it typically ends up here:

https://github.com/Azure/msrest-for-python/blob/98e2ebe7e5ed8a656f333963339ad8d20fc330d3/msrest/serialization.py#L936-L980

If you add some more types to this, so that you handle dates by passing datetime types to the date-serializer etc. then you are good.

rakshith91 commented 4 years ago

Hi @ludokriss Can you tell us what version of eventgrid sdk are you using? Also what version of python?

lmazuel commented 4 years ago

Hi @ludokriss In case you're using an old one, we just released a complete revamping of the SDK as preview: https://pypi.org/project/azure-eventgrid/2.0.0b1/

ludokriss commented 4 years ago

Hey, sorry that I didn't include versions. Here it is, with python 3.7: azure-eventgrid==1.3.0 msrest==0.6.18

Tested the new SDK so versions: azure-eventgrid==2.0.0b1 msrest==0.6.19

And the inconsistent behavior is still there. Btw., I like the new sdk 👍

lmazuel commented 4 years ago

Hi @ludokriss Got it, may I ask you to provide a data that has all the type you could expect to work by default? You mentionned decimals, but instead of guessing an actual Python code dict would be awesome :).

Note: I'm happy you like this new SDK ;)

lmazuel commented 4 years ago

Could you try https://github.com/Azure/msrest-for-python/pull/224 ?

ludokriss commented 4 years ago

Hey, #224 works perfectly for datetime types, so 👍 on that one. Added a pull request on the #224 PR for decimal types if you want to include that as well