Check is value is undefined in appendQueryParams

[sarangan12]

@qiaozha has recently reported that during the testing of beginCreateOrUpdateAndWait operation in the @azure/arm-iothub library. During the testing, the code is erroring out with the message:

(node:17412) UnhandledPromiseRejectionWarning: TypeError: value is not iterable
    at appendQueryParams (C:\Users\sarajama\Projects\temp\node_modules\@azure\core-client\dist\index.js:1343:40)

On analysis, I found that the value is undefined which is causing the error. Adding a check resolves the issue.

@xirzec Could you please review and approve the PR? TIA

[deyaaeldeen]

The type for value is either string | string[] so I think the function is correct in handling it as such only. Could you check which caller puts an undefined in a map that is supposed to hold string | string[] only?

[sarangan12]

@deyaaeldeen If you look at the combinedParams object it will look as:

{
  'api-version' => '2021-07-01-preview',
  'operationSource' => 'other',
  'asyncinfo' => undefined
}

As you can see, the asyncinfo is undefined which is causing this issue. The URL looks like https://management.azure.com/subscriptions/3755ce94-6c61-4b5c-b0b1-b2ce86fc540f/providers/Microsoft.Devices/locations/eastus/operationResults/aWQ9b3NfaWhfNWZiNWU3OTEtZGJmOS00ZjFiLThkY2EtYzBiOTJmMTI2MjIxO3JlZ2lvbj1lYXN0dXM=?api-version=2021-07-01-preview&operationSource=other&asyncinfo

[deyaaeldeen]

Ops sorry, I was confused. I thought the value came from queryParams map not from combinedParams. Ok, I looked at combinedParams type and it is Map<string, string | string[]> which should not still include undefined in it. Since simpleParseQueryParams is the one that produces it, so it is the one that needs to be updated. It splits each param on = as follows: const [name, value] = pair.split("=", 2); without validating that value is actually defined even though the type does not allow for undefined. There're two options here, either relax the type to allow for undefined or validate value to always be defined. I think the former option makes sense here because we would like to preserve whatever is in the URL but I could be wrong.

@xirzec any thoughts on this?

[jeremymeng]

If the value is undefined, shouldn't we just push ${name}=?

[xirzec]

I'm a little torn. I want to say "of course nobody would want a query param with no value!" but as soon as I say that I worry some service of ours indeed would expect that.

I'm leaning towards doing Jeremy's suggestion, but in that case to Deya's point we should update the map to include | undefined as one of the possible values.

[deyaaeldeen]

If the value is undefined, shouldn't we just push ${name}=?

The original URL in Sarangan's example does not have the = sign, so I think we will have to preserve this which means distinguishing between value being undefined and being an empty string.

[sarangan12]

Ok. I have changed the code to include undefined as a possible return value during parsing and changed the code

[xirzec]

Good work finding this one!

[xirzec]

I'm a little torn. I want to say "of course nobody would want a query param with no value!" but as soon as I say that I worry some service of ours indeed would expect that.

I'm leaning towards doing Jeremy's suggestion, but in that case to Deya's point we should update the map to include | undefined as one of the possible values.

[xirzec]

Thanks for the update! One remark on the new behavior and I would love if we could add a test or two. 👍

[xirzec]

So this will cause undefined to show up as name=undefined in the query string, which seems to match how URLSearchParams handles this:

I'm fine with it, but can we test this change on the service that was impacted by the linked issue before merging?

[deyaaeldeen]

If the service did not like this, I think we will need to do:

Suggested change

	} else {
	searchPieces.push(`${name}=${value}`);
	} else if (value === undefined) {
	searchPieces.push(name);
	} else {
	searchPieces.push(`${name}=${value}`);

[xirzec]

The last else should be unreachable, right?

[deyaaeldeen]

I think you're right, we could just throw an error there just in case.

[sarangan12]

So this will cause undefined to show up as name=undefined in the query string, which seems to match how URLSearchParams handles this:

I'm fine with it, but can we test this change on the service that was impacted by the linked issue before merging?

Yes. I have tested the changes with the actual iothub service that was causing this issue. It works fine. Thanks

[ramya-rao-a]

@sarangan12 Please create a PR with the changelog entry for the changes in this PR. We will be releasing core-client this week
cc @deyaaeldeen

[sarangan12]

@sarangan12 Please create a PR with the changelog entry for the changes in this PR. We will be releasing core-client this week cc @deyaaeldeen

Done. #18851