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

Regenerated mgmt plane packages that were affected due to a bug in generating polymorphic mappers #4468

Merged
merged 4 commits into from
Jul 27, 2019

Conversation

amarzavery
Copy link
Contributor

@@ -211,7 +209,7 @@ export const CoreResource: msRest.CompositeMapper = {
serializedName: "kind",
clientName: "kind"
},
uberParent: "CoreResource",
uberParent: "BaseResource",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@onalant - with this change, it should work. Thus resolving this comment Azure/autorest.typescript#458 (comment)

Copy link

@ghost ghost Jul 27, 2019

Choose a reason for hiding this comment

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

I think it was fine originally. I was more commenting on the fact that CoreResource was referred to as BaseResource.CoreResource in the discriminators, whereas other "base classes'' are usually given their own name. <= I was not clear about making this distinction, my apologies. In any case, I think this line should remain CoreResource to emit the proper discriminators.

The discriminators for CoreResource subtypes are still not as expected:

  'BaseResource.machine' : Machine,
...
  'BaseResource.process' : Process,
  'BaseResource.port' : Port,
  'BaseResource.clientGroup' : ClientGroup,
  'BaseResource.machineGroup' : MachineGroup,

I would imagine that the intended output is something like

  'CoreResource.machine' : Machine,
...
  'CoreResource.process' : Process,
  'CoreResource.port' : Port,
  'CoreResource.clientGroup' : ClientGroup,
  'CoreResource.machineGroup' : MachineGroup,

Copy link
Contributor Author

@amarzavery amarzavery Jul 27, 2019

Choose a reason for hiding this comment

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

Just verified by writing a sample and everything works as expected.
In fact I modified this operation signature machineGroups.create to take machineGroup of type CoreResourceUnion (Base class) and updated the createOperationSpec mapper to take the requestBody of type CoreResource mapper

  create(resourceGroupName: string, workspaceName: string, machineGroup: Models.CoreResourceUnion, options?: msRest.RequestOptionsBase | msRest.ServiceCallback<Models.MachineGroup>, callback?: msRest.ServiceCallback<Models.MachineGroup>): Promise<Models.MachineGroupsCreateResponse> {

And I provided MachineGroup as the request body and the sdk was able to correctly find out the mapper for MachineGroup from the discriminators map by looking for BaseResource.machineGroup.

sample

import * as msRestNodeAuth from "@azure/ms-rest-nodeauth";
import { ServicemapManagementClient, ServicemapManagementModels } from "@azure/arm-servicemap";
const subscriptionId = process.env["AZURE_SUBSCRIPTION_ID"] || "";

msRestNodeAuth.AzureCliCredentials.create().then((creds) => {
  const client = new ServicemapManagementClient(creds, subscriptionId);
  const resourceGroupName = "testresourceGroupName";
  const workspaceName = "testworkspaceName";
  const machineGroup: ServicemapManagementModels.MachineGroup = {
    displayName: "dp1",
    kind: "machineGroup"
  };
  client.machineGroups.create(resourceGroupName, workspaceName, machineGroup).then((res) => {
    console.log(res);
  });
}).catch((err) => {
  console.error(err);
});

I guess what you are missing is that in the swagger spec, "CoreResource" is an allOf "Resource" and "Resource" is tagged with extension "x-ms-azure-resource": true. It is a microsoft swagger extension that Autorest understands. When it sees this extension it will make the model as a child of BaseResource. Definition of BaseResource is present in @azure/ms-rest-azure-js package (runtime).

Copy link

Choose a reason for hiding this comment

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

Ah, I missed the allOf declaration. Thank you for the explanation and your time w.r.t. this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for filing the original issue and taking the time for a deep dive in the generator and the generated code.

msRestNodeAuth.interactiveLogin({{ tokenAudience: "https://graph.windows.net" }}).then((creds) => {
const client = new GraphRbacManagementClient(creds, subscriptionId, {
baseUri: "https://graph.windows.net"
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now updating the sample #3191

@amarzavery amarzavery merged commit 5da0a07 into Azure:master Jul 27, 2019
@ramya-rao-a
Copy link
Contributor

@daviwil fyi

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.

2 participants