[Storage Queue] Migrating to TypeSpec#45649
Conversation
|
/azp run python - storage - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - storage - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| ) -> object: | ||
| # ``obj`` is the deserialized container (PeekedMessages/ReceivedMessages) | ||
| # whose messages live on ``items_property``; to make typing happy | ||
| messages = getattr(obj, "items_property", obj) |
There was a problem hiding this comment.
mypy yells at this line if it isnt guarded by getattr since the type is just Iterable
| "days": {"minimum": 1}, | ||
| } | ||
|
|
||
| _attribute_map = { |
| from azure.core.credentials_async import AsyncTokenCredential | ||
|
|
||
|
|
||
| class QueuesClientConfiguration(QueuesClientConfigurationInternal): |
There was a problem hiding this comment.
this gets overriden bc storage actually makes it own custom config and token in the _shared file, we have to do this here to allow for the credential to be None
| return {s.id: s.access_policy or AccessPolicy() for s in identifiers} | ||
| return ( | ||
| { | ||
| s.id: AccessPolicy._from_generated(s.access_policy) # pylint: disable=protected-access |
There was a problem hiding this comment.
wrapping to not expose the gen model now that we do to-gen/from-gen - can add to changelog
| self.chunk_size - len(data), | ||
| self.total_size - (index + len(data)), | ||
| ) | ||
| read_size = min(self.chunk_size - len(data), self.total_size - (index + len(data))) |
There was a problem hiding this comment.
related to blob tsp migration
| # Changes may cause incorrect behavior and will be lost if the code is regenerated. | ||
| # -------------------------------------------------------------------------- | ||
|
|
||
| VERSION = "12.18.0b1" |
There was a problem hiding this comment.
Should we keep this one up to date?
| ): | ||
|
|
||
| parallel = max_concurrency > 1 | ||
| if parallel and "modified_access_conditions" in kwargs: |
There was a problem hiding this comment.
I know this is technically for other packages but is this condition still possible or is it replaced by the below kwargs?
There was a problem hiding this comment.
yes I think these can be removed, I can remove these shared files for now as well if we want and can sync them cross sdks when blobs/datalake merge bc they are the ones that use them with these changes?
| include = ["metadata"] if include_metadata else None | ||
| command = functools.partial( | ||
| self._client.service.list_queues_segment, | ||
| self._client.service.get_queues, |
There was a problem hiding this comment.
Like I said, it's probably fine since its wrapped, I just thought it was weird because I thought the architects always wanted listing operations to begin with list in Python.
| decoded = self.decode(content, response) | ||
| # Store decoded content on a side attribute to bypass the _RestField | ||
| # descriptor which would re-serialize bytes back to base64. | ||
| message._decoded_content = decoded |
There was a problem hiding this comment.
This change is a little scary to me. Are we sure this will always happen even if customers write their own decode policies? Also, messages passed through here will always go from the from_generated method that was modified to handle this case? Do we have good test coverage around this?
There was a problem hiding this comment.
The reason for this change was that this was assigning the decoded message back to message_text, in autorest models the type of message_text was not enforced so even though BinaryBaase64DecodePolicy was returning bytes it let you pass it through to the message_text param which is type str. Now the msrest models use _RestField which is converting the bytes back to to a string (dezer_xml_str fn in getattr) when giving the stored value back to the user. Are there cases where customers have written their own decode policy, do custom decode policies all inherit from the MessageDecodePolicy and do you know if they would be doing anything hacky to the base class here? If a customer writes their own decode policy that returns bytes or a non-string value it would ultimately become a string without this change. The operations that I see using a DecodeMessagePolicy (recieve message, receive messages and peek messages) all call QueueMessage.from_generated on the output so the _decoded_content should be parsed on the other end correctly. Are there particular tests you would want run against this?
There was a problem hiding this comment.
Example mini sample I ran (what the behavior would look like in typespec without this change)
|
For version and naming: naming we can fix here with this if you want: Azure/azure-rest-api-specs#44081, version is specified in the tspconfig.yaml on the rest-api-specs branch so you can update it there with new versions in typespec to keep the generated _version.py in alignment |
NOTE: autorest and typespec models have breaking changes between them as a part of a design choice made when creating the emitter - If we want to ensure no backcompat failures (is_xml_model, as_dict, serialize, deserialize etc.) we can patch the models in _generated/models/_patch.py, if we are okay with the changes listed we will not need these patches.
Queue tests passing same as in main