Implement from_dict #16509
Implement from_dict #16509
Conversation
ghost
added
the
|
/azp run python - eventgrid - tests |
rakshith91
requested review from
lmazuel,
swathipil and
yunhaoling
as code owners
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
||
| def _decode(content): | ||
| try: | ||
| return base64.b64decode(content.encode('utf-8')) |
There was a problem hiding this comment.
dumb question: what does the content look like, utf-8 base64 string?
also probably worth a better method name _decode_base64_string?
There was a problem hiding this comment.
"data_base64":'Y2xvdWRldmVudA==',
There was a problem hiding this comment.
so we decided not to expose data_base64 to users at all
see #16661
|
/azp run python - eventgrid - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - eventgrid - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| Returns the deserialized CloudEvent object when a dict is provided. | ||
|
|
||
| :param event: The dict representation of the event which needs to be deserialized. | ||
| :type event: dict |
There was a problem hiding this comment.
I think it would be helpful if we could add description on the key-value items we accepts -- but it's a doc stuff
There was a problem hiding this comment.
hmm, the payload normally comes directly from a storage queue - i can put a link to the relevant spec
There was a problem hiding this comment.
If not all the key-value pairs, at least mentioning that data_base64 will always be decoded and set as the value for the data field would be helpful. In general, maybe mentioning that from_dict will never result in the data_base64 field being set in the resulting CloudEvent object (if I'm getting that right).
| Returns the deserialized EventGridEvent object when a dict is provided. | ||
|
|
||
| :param event: The dict representation of the event which needs to be deserialized. | ||
| :type event: dict | ||
|
|
||
| :rtype: EventGridEvent |
There was a problem hiding this comment.
same comment as above regarding to the description of the dict accepted key-value items.
| assert event.data == b'cloudevent' | ||
| assert event.data_base64 == None |
There was a problem hiding this comment.
as I mentioned in the from_dict, the dictionary item is data_base64 while we actually set it to the data which I think kinda breaks consistency from the experience of using the client constructor.
| source=event.pop("source", None), | ||
| type=event.pop("type", None), | ||
| specversion=event.pop("specversion", None), | ||
| data=event.pop("data", None) or _decode(event.pop("data_base64", None)), |
There was a problem hiding this comment.
In my understanding, to use the from_dict for CloudEvent, users should be doing this:
dict = {
data_base64 = b'<base64 bytes>'
}
# or
dict = {
data = data
}which means internally we do:
data=event.pop("data", None)
data_base64=event.pop("data_base64", None),
is setting input data_base64 to the data argument by design?
There was a problem hiding this comment.
I think it is by design, and mirrors this. Similar to sending data that is of bytes type (sent as data_base64 in the outgoing request), when from_dict is used to deserialize, it sets data_base64 back to data.
There was a problem hiding this comment.
I'm thinking of the following two creation case which makes me feel the inconsistency, from intuition I would expect two cases to return the "same" CloudEvent :
event_created_from_class = CloudEvent(data_base64=b'bytes_data')
event_created_from_class.data is None
event_created_from_class.data_base64 is not None
event_created_from_dict = CloudEvent.from_dict({'data_base64':b'bytes_data'})
event_created_from_dict.data is not None
event_created_from_dict.data_base64 is Noneapart from that, I notice that from_dict could take both data and data_base64, and if both are set data_base64 gets ignored:
# I think this should be an invalid input?
event_created_from_dict = CloudEvent.from_dict({'data': 'somedata', 'data_base64':b'bytes_data'})There was a problem hiding this comment.
(I could also be wrong, might be missing some context here, just thinking out loud)
There was a problem hiding this comment.
That two ways that data_base64 can be deserialized is a little confusing since, no matter what, only data will end up being set. Maybe clarifying this in the docstring of from_dict would be useful?
There was a problem hiding this comment.
we could also do a cross-language check
There was a problem hiding this comment.
update: we decided not to expose data_base64
#16661
|
Closing in favor of #16661 |
No description provided.