[Storage Files] Migrating to TypeSpec#46929
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates azure-storage-file-share to a TypeSpec-generated (dpcodegen) codebase and adjusts tests/recording infrastructure to keep content validation and playback working under the new client surface.
Changes:
- Introduces TypeSpec
tsp-location.yamland package metadata to drive generation and API review (apiview-properties.json,_metadata.json). - Switches sync/async public clients to the new generated
FileClientplumbing and updates request/response handling (including content-validation stream handling). - Updates tests and test-proxy configuration for TypeSpec differences (query ordering), and disables legacy-transport tests.
Reviewed changes
Copilot reviewed 59 out of 64 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/storage/azure-storage-file-share/tsp-location.yaml | Adds TypeSpec source location for generation. |
| sdk/storage/azure-storage-file-share/tests/test_file.py | Skips legacy transport tests (including content-validation variant). |
| sdk/storage/azure-storage-file-share/tests/test_file_async.py | Skips async legacy transport tests (including content-validation variant). |
| sdk/storage/azure-storage-file-share/tests/conftest.py | Adjusts test-proxy sanitizers and matcher to tolerate query reordering. |
| sdk/storage/azure-storage-file-share/setup.py | Removes legacy setup.py packaging. |
| sdk/storage/azure-storage-file-share/pyproject.toml | Adds PEP 517/518 packaging config and dependencies for the migrated package. |
| sdk/storage/azure-storage-file-share/MANIFEST.in | Updates packaging includes (notably py.typed). |
| sdk/storage/azure-storage-file-share/CHANGELOG.md | Updates unreleased notes for the migration-related behavior changes. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/py.typed | Adds PEP 561 marker file. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/aio/_share_service_client_async.py | Ports async service client to new generated FileClient and request shape. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/aio/_share_client_async.py | Ports async share client calls/serialization to new generated API surface. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/aio/_lease_async.py | Updates async lease operations to new generated parameter names (e.g., lease_duration, lease_id). |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/aio/_file_client_async.py | Ports async file client to new generated API surface and snapshot/content-validation handling. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/aio/_directory_client_async.py | Ports async directory client to new generated API surface and parameter names. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_version.py | Updates version/header for generated packaging flow. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_shared/uploads.py | Updates shared upload helpers to new generated operation parameter names. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_shared/uploads_async.py | Updates async shared upload helpers to new generated operation parameter names. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_shared/response_handlers.py | Adjusts delegation key parsing for new model shapes. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_shared/request_handlers.py | Minor formatting adjustments to error messages. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_shared/policies.py | Updates content validation to wrap iter_bytes/iter_raw streams used by TypeSpec-generated code. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_share_service_client.py | Ports sync service client to new generated FileClient and parameter names. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_share_client.py | Ports sync share client to new generated FileClient and model conversions. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_share_client_helpers.py | Updates helper request shape to match TypeSpec-generated parameters. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_serialize.py | Flattens access-condition handling to match TypeSpec-generated parameter style. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_parser.py | Adds _strip_snapshot_from_url helper for new generated base URL expectations. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_models.py | Adds back-compat mixin and model conversion helpers bridging old public models to new generated base. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_lease.py | Updates sync lease operations to new generated parameter names (e.g., lease_duration, lease_id). |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/operations/_service_operations.py | Removes old AutoRest-generated service operations module. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/operations/_patch.py | Adds operation patching to restore historical defaults (e.g., file_range_write). |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/operations/init.py | Switches to TypeSpec-generated operations module wiring. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/models/_patch.py | Adds _BackCompatMixin for legacy msrest-like model APIs. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/models/_enums.py | Updates enums to TypeSpec-generated naming/docs. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/models/init.py | Updates model exports to the TypeSpec-generated module set. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/aio/operations/_service_operations.py | Removes old AutoRest-generated async service operations module. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/aio/operations/_patch.py | Adds async operation patching for historical defaults. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/aio/operations/init.py | Switches to TypeSpec-generated async operations module wiring. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/aio/_patch.py | Adds async generated-client patching (Range header policy, pipeline wrapping). |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/aio/_configuration.py | Updates generated async configuration to TypeSpec pattern (AAD credential policy). |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/aio/_client.py | Updates generated async client (constructor/signature and send_request). |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/aio/init.py | Updates async generated package exports to FileClient. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/_version.py | Adds generated-package version constant. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/_utils/serialization.py | Adds fast-path header deserialization and typing updates for new generator. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/_utils/init.py | Updates generator attribution/header. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/_patch.py | Adds generated-client patching (Range header policy, pipeline wrapping). |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/_configuration.py | Updates generated sync configuration to TypeSpec pattern (AAD credential policy). |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/_client.py | Updates generated sync client (constructor/signature and send_request). |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_generated/init.py | Updates generated package exports and __version__. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_file_client.py | Ports sync file client to new generated API surface and snapshot/content-validation handling. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_file_client_helpers.py | Updates helper option building for new generated parameter names. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_directory_client.py | Ports sync directory client to new generated API surface and parameter names. |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_deserialize.py | Wraps download iterators so properties/response metadata can be attached (TypeSpec download shape). |
| sdk/storage/azure-storage-file-share/apiview-properties.json | Adds APIView cross-language mapping metadata for the migrated API surface. |
| sdk/storage/azure-storage-file-share/_metadata.json | Adds TypeSpec API version metadata. |
| source_authorization = kwargs.pop("source_authorization", None) | ||
| source_mod_conditions = get_source_conditions(kwargs) | ||
| _source_mod_conditions = get_source_conditions(kwargs) | ||
| access_conditions = get_access_conditions(kwargs.pop("lease", None)) |
There was a problem hiding this comment.
Yes, are we losing anything here?
There was a problem hiding this comment.
no, none of the kwarsg here getting removed were ever used by the upload_range_from_url operation
| ### Other Changes | ||
| - Legacy transports will not be supported moving forward | ||
| - Return type of AccessPolicies in get_share_access_policies is now the public AccessPolicy model | ||
|
|
There was a problem hiding this comment.
If we removed 3.9 support as part of this PR, let's add this. Can follow format of comment we've used before though. Also, I think the README should be updated too. We can also take care of this after or separately.
| @@ -4010,6 +4011,7 @@ def test_legacy_transport(self, **kwargs): | |||
| file_data = file_client.download_file().readall() | |||
| assert file_data == b"Hello World!" # data is fixed by mock transport | |||
|
|
|||
| @pytest.mark.skip("Legacy transports will not be supported moving forward") | |||
| @@ -4131,6 +4132,7 @@ async def test_legacy_transport(self, **kwargs): | |||
| file_data = await (await file_client.download_file()).readall() | |||
| assert file_data == b"Hello Async World!" # data is fixed by mock transport | |||
|
|
|||
| @pytest.mark.skip("Legacy transports will not be supported moving forward") | |||
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| ) -> None: | ||
| self.encryption_in_transit = encryption_in_transit | ||
|
|
||
| def _to_generated(self): |
There was a problem hiding this comment.
should these all be static?
jalauzon-msft
left a comment
There was a problem hiding this comment.
A lot of comments for async also apply to sync.
| file_range_write: Union[str, "_models.FileRangeWriteType"] = "update", | ||
| **kwargs: Any, | ||
| ) -> None: | ||
| return await super().upload_range(optional_body, file_range_write=file_range_write, **kwargs) |
There was a problem hiding this comment.
What is the point of this patch? Seems to just call the super as-is
There was a problem hiding this comment.
it sets the file_range_write default value to update to align with what the previous autorest operation did
| from azure.core.credentials_async import AsyncTokenCredential | ||
|
|
||
|
|
||
| class RangeHeaderPolicy(SansIOHTTPPolicy): |
There was a problem hiding this comment.
Instead of adding this through a patch, can we just add this like we do any other policy? Add it to shared policies file and add it to the list of policies in base_client? Since we don't really rely on the generated pipeline configuration, doing this here may be confusing or get missed/forgotten. Would simplify things as well.
| def get_access_conditions( | ||
| lease: Optional[Union["ShareLeaseClient", "ShareLeaseClientAsync", str]], | ||
| ) -> Optional[LeaseAccessConditions]: | ||
| def get_access_conditions(lease: Optional[Union["ShareLeaseClient", "ShareLeaseClientAsync", str]]) -> Optional[str]: |
There was a problem hiding this comment.
Can we rename this now? Maybe just get_lease_id or something like that. Probably rename call sites too. Basically I was quite confused when I saw lease_id=access_conditions. Probably apply the same to get_source_access_conditions as well.
| if self.snapshot: | ||
| params = kwargs.pop("params", {}) or {} | ||
| params["sharesnapshot"] = self.snapshot | ||
| kwargs["params"] = params |
There was a problem hiding this comment.
What is params? Query params? Why does the new generated code not support it?
| @@ -1,3 +1,4 @@ | |||
| # pylint: disable=line-too-long,useless-suppression | |||
There was a problem hiding this comment.
useless-suppression feels like something we could just fix? This applies to a few files.
| return snapshot or path_snapshot | ||
|
|
||
|
|
||
| def _strip_snapshot_from_url(url: str) -> str: |
There was a problem hiding this comment.
Again, I don't think this should be needed if we modify base_client directly.
| process_storage_error(error) | ||
| return {"public_access": response.get("share_public_access"), "signed_identifiers": identifiers or []} | ||
| return { | ||
| "public_access": response.get("share_public_access"), |
There was a problem hiding this comment.
We were previously returning the generated model directly? Were we exporting the generated one directly? Probably would be best to return the hand-written one if we have it. Would also want to change the export in that case. I think as long as they are the same it wouldn't be breaking?
| @@ -0,0 +1,3385 @@ | |||
| ```py | |||
| namespace azure.storage.fileshare | |||
There was a problem hiding this comment.
We recently added these to main so I'm not sure if they will conflict or if they are the same.
| ### Features Added | ||
|
|
||
| ### Other Changes | ||
| - Legacy transports will not be supported moving forward |
There was a problem hiding this comment.
Can we elaborate on this a bit more? Like something about custom transports that return the old types.
| ### Other Changes | ||
| - Legacy transports will not be supported moving forward | ||
| - Return type of AccessPolicies in get_share_access_policies is now the public AccessPolicy model | ||
|
|
There was a problem hiding this comment.
If we removed 3.9 support as part of this PR, let's add this. Can follow format of comment we've used before though. Also, I think the README should be updated too. We can also take care of this after or separately.
…into storagefiles
need to ensure content-validation is working as expected