search

Azure / azure-storage-python

Microsoft Azure Storage Library for Python
https://azure-storage.readthedocs.io
MIT License
338 stars 240 forks source link

Milliseconds stripped from DateTime & EntityProperty(EdmType.DATETIME) during serialization. #556

Closed PhillVixx closed 5 years ago

PhillVixx commented 5 years ago

Which service(blob, file, queue) does this issue concern?

Possibly all that use azure-storage-common

Which version of the SDK was used? Please provide the output of pip freeze.

azure-common==1.1.18 azure-cosmosdb-nspkg==2.0.2 azure-cosmosdb-table==1.0.5 azure-nspkg==3.0.2 azure-storage-common==1.4.0

What problem was encountered?

The _to_utc_datetime function in azure/storage/common/_serialization.py does not include milliseconds and so all datetime objects have milliseconds stripped when being sent to azure.

Have you found a mitigation/solution?

Adding .%f seems to work: “return value.strftime('%Y-%m-%dT%H:%M:%S.%fZ')”

Note: for table service, please post the issue here instead: https://github.com/Azure/azure-cosmosdb-python.

zezha-msft commented 5 years ago

Hi @PhillVixx, thanks for reaching out!

Which API are you using? I assume is a table API, right?

PhillVixx commented 5 years ago

Hi @PhillVixx, thanks for reaching out!

Which API are you using? I assume is a table API, right?

@zezha-msft Indeed, not sure how that affects the bug though as it's in 'azure-storage-common' which could be used anywhere. I logged the bug here under 'azure-storage-python' as that is where 'azure-storage-common' resides - as opposed to logging it say under 'azure-cosmosdb-python'. Specifically the bug is here: https://github.com/Azure/azure-storage-python/blob/master/azure-storage-common/azure/storage/common/_serialization.py#L45

zezha-msft commented 5 years ago

Hi @PhillVixx, thanks for the clarification!

Please log a bug with the azure-cosmosdb-python instead. It is not a bug in the context of azure-storage-common.

PhillVixx commented 5 years ago

Sorry @zezha-msft but that makes no sense to me; the bug IS IN azure-storage-common, line 45 of azure-storage-common/azure/storage/common/_serialization.py to be precise. Is this some sort of internal project ownership issue as to why you want it logging in a different area? Please help me understand.

zezha-msft commented 5 years ago

Hi @PhillVixx, thanks for following up on this. To clarify, it's definitely not about project ownership or bureaucracy.

The reason why I closed it, is because it is simply not a bug in the azure-storage-common package. If you searched for the usage of _to_utc_datetime, you'd see that it's used inside azure-storage-common to serialize dates for various authentication-related functions. The method is fulfilling its purpose without any problem. In addition, the leading underscore implies that it is internal, and shouldn't be used outside of the package.

Therefore, to fix the problem with Table APIs that you are facing(feel free to share which ones they are), the serialization fix should be done in azure-cosmosdb-python instead.

PhillVixx commented 5 years ago

Hi @zezha-msft, for clarity I'm not using _to_utc_datetime directly, I call tableservice.insert_or_replace_entity, from, as you rightly point out: azure-cosmosdb-python. However, and to cut a long story short, it calls _convert_entity_to_json from azure.cosmosdb.table._serialization which in turn uses the offending _to_utc_datetime from azure.storage.common._serialization. So whilst I understand your points, the assertion that _to_utc_datetime should not be used outside the package is clearly broken by your own colleagues (during a code refactoring in Nov 2017 they moved _to_utc_datetime out of cosmosdb.common into storage.common) and if this cross package use is actually allowed (it's Microsoft's code after all) then there's a case to answer about whether there should be a different datetime serializer used for the authentication-related functions to which you refer. So whether it's a bug in cosmosdb or in storage depends on how you cut it, and as I mentioned previously the fact that it's in a 'common' package would suggest that it will be used all over and not just within it's own locality.

zezha-msft commented 5 years ago

Hi @PhillVixx, I understand and appreciate your perspective.

However, the word 'common' in the package name doesn't mean everything in it must be used by every package that depend on it. The internal functions (the ones with an underscore) are reserved for the common package itself. Of course, there's no concept of private methods in Python, so the underscore prefix is just a convention and not obligation. But the convention is indeed broken by by azure-cosmosdb-python. Therefore, the bug is in azure-cosmosdb-python, since it is using the method in the unintended way.