Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deserialization error for LRO which has discriminator #2589

Closed
wants to merge 52 commits into from

Conversation

msyyc
Copy link
Member

@msyyc msyyc commented May 15, 2024

fixes #2428

There are some new findings for legacy test when I mark initial operation as stream operation:

  1. When LRO succeeds in first call, if will fail if use pipeline_response to deserialize since its context is null. One possible solution is to use pipeline_response.http_response which works both for sync and async.

  2. When LRO needs poll, azure-core will set stream=false by default for poll request, so lro works as before.

  3. for async initial, we need call response.load_body() for normal response otherwise the following error happens:
    image

  4. for async initial, we need call response.load_body() for error response otherwise the following error happens for test case test_sads_put_non_retry. However sync needn't:

image

  1. version-tolerant and legacy use different HttpResponse type
mode HttpResponse type .read() .load_body() .stream_download() .iter_bytes()
leagcy (sync) azure.core.pipeline.transport.RequestsTransportResponse No No Yes No
leagcy (async) azure.core.pipeline.transport.AioHttpTransportResponse No Yes Yes No
vesion-tolerant (sync/async) azure.core.rest.HttpResponse/AsyncHttpResponse Yes No No Yes

@msyyc
Copy link
Member Author

msyyc commented May 20, 2024

@msyyc msyyc marked this pull request as ready for review May 23, 2024 08:31
@msyyc msyyc changed the title [WIP] Fix deserialization error for LRO which has discriminator Fix deserialization error for LRO which has discriminator May 23, 2024
@msyyc msyyc disabled auto-merge May 30, 2024 07:10
@msyyc msyyc requested a review from iscai-msft May 30, 2024 08:12
f" {async_await} response.read() # Load the body in memory and close the socket",
]
)
response_read = f" {async_await}response.read() # Load the body in memory and close the socket"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can always call read, regardless of stream or not

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is very strange to judge _stream when its value is True, so I think it is better to keep the logic.

@msyyc
Copy link
Member Author

msyyc commented Jun 11, 2024

move to #2628

@msyyc msyyc closed this Jun 11, 2024
@msyyc msyyc deleted the deserialization-fix branch June 11, 2024 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] deserialization bug for sub model type
2 participants