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.

Sign up for GitHub

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

Jump to bottom

deserialize null faithfully #12009

Closed
deyaaeldeen opened this issue Oct 22, 2020 · 4 comments · Fixed by #14366
Closed

deserialize null faithfully #12009

deyaaeldeen opened this issue Oct 22, 2020 · 4 comments · Fixed by #14366
Assignees
@deyaaeldeen
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Milestone
[2021] April

Comments

@deyaaeldeen
Copy link
Member

deyaaeldeen commented Oct 22, 2020 •
edited
Loading

Currently core-http deserialize empty body response with a default value and not a null (here is an example). The reason we do not deserialize to null is we always attach a _response information to our response values and this is not possible with a null. In order to faithfully deserialize values coming form the server, we need to change the representation of _response. This should be taken into consideration in this issue: #10887. Because it is a breaking change, it should be planned for core-http v2.
@deyaaeldeen deyaaeldeen added this to the Backlog milestone Oct 22, 2020
@deyaaeldeen deyaaeldeen self-assigned this Oct 22, 2020
@HarshaNalluru
Copy link
Member

So, the "null" response would not have a corresponding _response?

@deyaaeldeen deyaaeldeen mentioned this issue Oct 23, 2020
Merged
@deyaaeldeen
Copy link
Member Author

@HarshaNalluru _response is added to the response using intersection types and the intersection of _response and null, is, yes you guessed it right, never. We sidestep the issue by not returning a null to begin with. To fix this and faithfully deserialize the response, we need to change our representation of the response to either make the the information in _response on-demand using a callback like what Python SDK does or wrap it around the actual value using what .Net SDK does. Any other suggestions are welcome too!

@ramya-rao-a ramya-rao-a added Azure.Core Client This issue points to a problem in the data-plane of the library. labels Oct 23, 2020
@ramya-rao-a ramya-rao-a modified the milestones: Backlog, MQ-2020 Oct 23, 2020
@xirzec
Copy link
Member

xirzec commented Oct 23, 2020

consider for #8535

@deyaaeldeen deyaaeldeen mentioned this issue Dec 17, 2020
Closed
@deyaaeldeen
Copy link
Member Author

deyaaeldeen commented Feb 8, 2021 •
edited
Loading

_response has been moved to be accessible through a callback in core-client. I think what remains to be done is to make sure we deserialize nulls as nulls in core-https.

@deyaaeldeen deyaaeldeen modified the milestones: MQ-2020, [2021] March Feb 8, 2021
@deyaaeldeen deyaaeldeen mentioned this issue Mar 18, 2021
Merged
@ghost ghost closed this as completed in #14366 Mar 22, 2021
ghost pushed a commit that referenced this issue Mar 22, 2021
@deyaaeldeen
[Core v2] Return null when the response is empty and nullable (#14366)
9e8890a 
Fixes #12009.

# Scenario

When the service does not return a body ([Example in our test server](https://github.com/Azure/autorest.testserver/blob/4b994be5fd68b3ac009af30399ad6b817630dbc8/legacy/routes/dictionary.js#L24)).

# Expectation

Response will be `null` if the mapper says it is `nullable`.

# Current State

Response is an empty object or array ([Example](https://github.com/Azure/autorest.typescript/blob/c8e68b845ccc3780d8c4be3787eb2c5e74272697/test/integration/bodyArray.spec.ts#L21)).

# Fix

Implements the expectation by doing case analysis and adds test cases.
jay-most pushed a commit to jay-most/azure-sdk-for-js that referenced this issue Apr 26, 2021
@deyaaeldeen @vindicatesociety
[Core v2] Return null when the response is empty and nullable (Azure#…
f68d5c0 
…14366)

Fixes Azure#12009.

# Scenario

When the service does not return a body ([Example in our test server](https://github.com/Azure/autorest.testserver/blob/4b994be5fd68b3ac009af30399ad6b817630dbc8/legacy/routes/dictionary.js#L24)).

# Expectation

Response will be `null` if the mapper says it is `nullable`.

# Current State

Response is an empty object or array ([Example](https://github.com/Azure/autorest.typescript/blob/c8e68b845ccc3780d8c4be3787eb2c5e74272697/test/integration/bodyArray.spec.ts#L21)).

# Fix

Implements the expectation by doing case analysis and adds test cases.
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2023
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@deyaaeldeen deyaaeldeen

Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Milestone
[2021] April
Development

Successfully merging a pull request may close this issue.

[Core v2] Return null when the response is empty and nullable deyaaeldeen/azure-sdk-for-js
4 participants
@xirzec @deyaaeldeen @HarshaNalluru @ramya-rao-a