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-rest-pipeline] Fix serialization of body of false #14218

Merged
4 commits merged into from
Mar 10, 2021

Conversation

deyaaeldeen
Copy link
Member

If the body contains just false, this check will confuse it with the body being non existent. This PR fixes this issue.

Copy link
Member

@jeremymeng jeremymeng left a comment

Choose a reason for hiding this comment

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

Looks like this is ported from

if ((httpRequest.body !== undefined && httpRequest.body !== null) || required) {
. We could add the null case too just to be safe?

@jeremymeng
Copy link
Member

Or is there a case that we actually want to use null request body

@deyaaeldeen
Copy link
Member Author

@jeremymeng I do not think body can be null, according to its type at least.

@bterlson
Copy link
Member

Little bit worried about null bodies since its a valid JSON value. Supporting all JSON values in body seems like a good idea to me.

@jeremymeng
Copy link
Member

@jeremymeng I do not think body can be null, according to its type at least.

@deyaaeldeen the union type has null, unless I am missing anything. I don't see boolean in it, a different issue though

@deyaaeldeen
Copy link
Member Author

@jeremymeng You are right, I missed it. Updated accordingly.

@deyaaeldeen deyaaeldeen requested a review from jeremymeng March 10, 2021 17:59
@deyaaeldeen
Copy link
Member Author

@bterlson good point! I think it makes sense to revert 88abff3. @jeremymeng thoughts?

@jeremymeng
Copy link
Member

I was looking at the truth table in serialize() and the condition we throw errors. It feels to me that we may also want to check nullable to ensure we don't pass null to serialize() if it is not the nullable case? thoughts?
https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/core/core-client/src/serializer.ts#L150

@jeremymeng
Copy link
Member

What I meant is

-    if ((request.body !== undefined && request.body !== null) || required) {
+    if ((request.body !== undefined && request.body !== null) || (nullable && request.body === null) || required) {

but it is an additional case to allow null so current PR should be good.

@ghost
Copy link

ghost commented Mar 10, 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 7eab864 into Azure:master Mar 10, 2021
@deyaaeldeen deyaaeldeen deleted the core-client-fix-false branch March 10, 2021 19:39
ghost pushed a commit that referenced this pull request Mar 12, 2021
Adds a test case for a JSON body that is
- false
- null [according to the spec](https://www.json.org/json-en.html)

Those tests were missed in #14218.
jay-most pushed a commit to jay-most/azure-sdk-for-js that referenced this pull request Apr 26, 2021
If the body contains just false, this check will confuse it with the body being non existent. This PR fixes this issue.
jay-most pushed a commit to jay-most/azure-sdk-for-js that referenced this pull request Apr 26, 2021
Adds a test case for a JSON body that is
- false
- null [according to the spec](https://www.json.org/json-en.html)

Those tests were missed in Azure#14218.
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request May 18, 2021
add AVS 2021-06-01 API (Azure#14215)

* add AVS 2021-06-01 API

* add cmdlets custom word

* rename ScriptCmdlet cmdletDescription to description (Azure#14218)

* Adding optional to the script parameter (Azure#14437)

Co-authored-by: David Becher <[email protected]>

Co-authored-by: david becher <[email protected]>
Co-authored-by: David Becher <[email protected]>
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.

3 participants