Azure / ms-rest-js

Runtime for isomorphic javascript libraries generated by Autorest
MIT License
55 stars 55 forks source link

'in' operator is less robust than other options for deriving key presence #440

Open Packmanager9 opened 3 years ago

Packmanager9 commented 3 years ago

Problem: Given that the 'in' operator will throw an error error and fail: 'Cannot use 'in' operator to search for 'top' in 1' (Line 4959 in 'msRest.node.js)

Solution A different method of deriving the key presence could be used.

Original:

  if (parent != undefined && parameterPathPart in parent) {
      parent = parent[parameterPathPart];
  }

Proposed:

  if (parent != undefined && Object.keys(parent).includes(parameterPathPart)) {
      parent = parent[parameterPathPart];
  } 

Alternatives: Tried removing the check all together, immediately fails.

jeremymeng commented 3 years ago

@Packmanager9 The current code assumes that parent is an object, but the error indicates that it could be a primitive type. Do you have some simplified repro?

https://github.com/Azure/ms-rest-js/blob/386ed0ff198efe12a7caf59f6d1d34bedcbbd22d/lib/serviceClient.ts#L721-L731

The only call to this method is on a serviceClient which should be an object. https://github.com/Azure/ms-rest-js/blob/386ed0ff198efe12a7caf59f6d1d34bedcbbd22d/lib/serviceClient.ts#L663-L664

Packmanager9 commented 3 years ago

Sure, here's the snippet I was using.

msRestNodeAuth.interactiveLogin().then((creds) => { const client = new LogicManagementClient(creds, subscriptionId); const top = 1; const filter = "testfilter"; client.workflows.listBySubscription(top, filter).then((result) => { console.log("The result is:"); console.log(result); }); }).catch((err) => { console.error(err); });

jeremymeng commented 3 years ago

@Packmanager9 could you please try wrapping the arguments to listBySubscription with an object?

-client.workflows.listBySubscription(top, filter)
+client.workflows.listBySubscription({top, filter})

The method expects an options object

  listBySubscription(options?: Models.WorkflowsListBySubscriptionOptionalParams): Promise<Models.WorkflowsListBySubscriptionResponse>;
jeremymeng commented 3 years ago

@Packmanager9 it's been a while, have you got a chance to look at the above suggestion?