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

[Core v2] Return null when the response is empty and nullable #14366

Merged
13 commits merged into from
Mar 22, 2021

Conversation

deyaaeldeen
Copy link
Member

Fixes #12009.

Scenario

When the service does not return a body (Example in our test server).

Expectation

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

Current State

Response is an empty object or array (Example).

Fix

Implements the expectation by doing case analysis and adds test cases.

@deyaaeldeen
Copy link
Member Author

/azp js run ai-text-analytics tests

@azure-pipelines
Copy link

Command 'js' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@deyaaeldeen
Copy link
Member Author

/azp run js ai-text-analytics tests

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@deyaaeldeen
Copy link
Member Author

/azp run js - ai-text-analytics tests

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@deyaaeldeen
Copy link
Member Author

/azp run js - ai-text-analytics - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@deyaaeldeen
Copy link
Member Author

/azp run js - eventgrid - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@deyaaeldeen
Copy link
Member Author

/azp run js - data-tables - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@deyaaeldeen deyaaeldeen requested a review from xirzec March 19, 2021 02:48
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

This is looking much cleaner! A few last remarks

sdk/core/core-client/src/utils.ts Outdated Show resolved Hide resolved
sdk/core/core-client/src/utils.ts Outdated Show resolved Hide resolved
sdk/core/core-client/src/utils.ts Outdated Show resolved Hide resolved
sdk/core/core-client/src/utils.ts Show resolved Hide resolved
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Happy to see this utility function looking cleaner! 🥳

@ghost
Copy link

ghost commented Mar 19, 2021

Hello @deyaaeldeen!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 9e8890a into Azure:master Mar 22, 2021
@deyaaeldeen deyaaeldeen deleted the corev2/return-null branch March 22, 2021 13:34
ghost pushed a commit that referenced this pull request Mar 26, 2021
Updates the changelog with the work done in #14366 and renames `isPrimitiveType` to `isPrimitiveBody`.

EDIT: I also refactored `flattenResponse` further so it now has just one call to `handleNullableResponseAndWrappableBody`.
jay-most pushed a commit to jay-most/azure-sdk-for-js that referenced this pull request Apr 26, 2021
…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.
jay-most pushed a commit to jay-most/azure-sdk-for-js that referenced this pull request Apr 26, 2021
Updates the changelog with the work done in Azure#14366 and renames `isPrimitiveType` to `isPrimitiveBody`.

EDIT: I also refactored `flattenResponse` further so it now has just one call to `handleNullableResponseAndWrappableBody`.
This pull request was closed.
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.

deserialize null faithfully
3 participants