ms-rest-js icon indicating copy to clipboard operation
ms-rest-js copied to clipboard

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

Open Packmanager9 opened this issue 4 years ago • 4 comments

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.

Packmanager9 avatar Mar 18 '21 17:03 Packmanager9

@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

jeremymeng avatar Mar 22 '21 18:03 jeremymeng

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); });

Packmanager9 avatar Mar 22 '21 21:03 Packmanager9

@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 avatar Mar 22 '21 22:03 jeremymeng

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

jeremymeng avatar Aug 16 '21 16:08 jeremymeng