Nested Polymorphism and Discriminators

[ghost]

2019-07-26T16:48:30Z: trying cherry-pick of be5280d from #457

2019-07-26T17:16:35Z: this is resolved by #457

IN SHORT: while loop here causes issues with nested polymorphism and discriminators.

Some Azure SDKs rely on discriminators to resolve serialization mappers at call-time. The mappers both validate and transform the input payload such that they are of the structure expected by the API. We discovered that in some instances the discriminator labels are not emitted correctly. We provide the example that first exposed this issue, the npm package @azure/arm-timeseriesinsights.

The mapper definitions in lib/models/mappers.ts appear as expected. However, in the last section specifying the discriminators, we see the following:

export const discriminators = {
  'CreateOrUpdateTrackedResourceProperties.EventSourceCreateOrUpdateParameters' : EventSourceCreateOrUpdateParameters,
  'CreateOrUpdateTrackedResourceProperties.Microsoft.EventHub' : EventHubEventSourceCreateOrUpdateParameters,
  'CreateOrUpdateTrackedResourceProperties.Microsoft.IoTHub' : IoTHubEventSourceCreateOrUpdateParameters,
  'BaseResource.EventSourceResource' : EventSourceResource,
  'BaseResource.Microsoft.EventHub' : EventHubEventSourceResource,
  'BaseResource.Microsoft.IotHub' : IoTHubEventSourceResource
};

We would like to draw attention to the lines of the form .+(Environment|EventSource)CreateOrUpdateParameters. If, for example, we call client.environments.createOrUpdate(...) with the following parameters:

const params = {
    kind: "LongTerm",
    location: "PLANET EARTH",
    sku: {
        name: "L1",
        capacity: 1
    },
    timeSeriesIdProperties: [
        {
            name: "FOO",
            type: "String"
        }
    ],
    storageConfiguration: {
        accountName: "FOOSTORE",
        managementKey: "SOOPER_SEKRAT"
    }
};

we would naturally expect the environment to provision. However, we are instead met with an error message:

(node:6948) UnhandledPromiseRejectionWarning: Error: Value cannot be null.
Parameter name: properties
Path ''.
    at new RestError (${SOME_PATH}/node_modules/@azure/ms-rest-js/dist/msRest.node.js:1399:28)
    at ${SOME_PATH}/node_modules/@azure/ms-rest-js/dist/msRest.node.js:2494:37
    at process._tickCallback (internal/process/next_tick.js:68:7)

Briefly, this is because the mapper was not found. In ms-rest-js/lib/serializer.ts, we notice the function getPolymorphicMapper. In particular, at getPolymorphicMapper+8,+11 we see the lines

const indexDiscriminator = discriminatorValue === typeName
  ? discriminatorValue
  : typeName + "." + discriminatorValue;
const polymorphicMapper = serializer.modelMappers.discriminators[indexDiscriminator];

At runtime, typeName will resolve to EnvironmentCreateOrUpdateParameters; but this does not match CreateOrUpdateTrackedResourceProperties as implied by the discriminators export above! Then, polymorphicMapper will be undefined, so that the original mapper object passed to getPolymorphicMapper will be returned. Of course, since this would be the mapper for the base class EnvironmentCreateOrUpdateParameters, the properties fields necessary for the Standard and LongTerm variants will be dropped.

The root of this issue seems to lie at src/vanilla/Model/CodeModelTS.cs#320. While (no pun intended) we would expect the polymorphic type to ``step up'' one level, it instead goes to the very base. That is, if we have

A
|
--> B
    |
    --> C

rather than using B as the polymorphic base of C, autorest.typescript will use A, thus generating the discriminator line A.C.

When we remove the while loop and instead step only one level, the above issues disappear. That being said, we do understand that stepping to the ``base-most'' class of an inheritance hierarchy may sometimes be the more correct behavior. Our question then is: what are the issues of only stepping one level in the inheritance hierarchy?

Attached is a patch file with the changes we made to our local autorest.typescript. If so desired, we can also provide a minimal project replicating the error described above.

step.patch.txt

[amarzavery]

@onalant - Thanks for filing the issue and the detailed explanation.

We started using the uber parent.child approach to distinguish between different children due to the issue described over here -> Azure/autorest#726 (comment) (Before developing the TS generator, we had similar stuff in the nodejs generator which was generating sdks meant to run only with nodejs runtime. However, some core concepts around [de]serialization still remain the same while developing isomorphic TS sdks.)

While we can use the immediate parent instead of the uber parent in the key of discriminators map to distinguish between the different children, I vaguely remember having an issue with that approach. Right now, if we adopt that approach then we need to make sure that the uberParent key in the mapper for any given model also points to it's immediateParent instead of the uberParent. Otherwise the lookup will fail again. The fix in #457 ensures that the uberParent key correctly points to a model's uber parent, thereby ensuring that lookup in the discriminator map happens correctly..

Let me know if the issue persists.

[ghost]

We seem to be unblocked thanks to the changes introduced by #457.

Thank you for the quick response and the fix! If the PR is going to be merged soon, is this issue still necessary for you folks?

[amarzavery]

We will merge the PR soon and publish a new version of the generator. This issue will be closed automatically by github once #457 is merged.

We will also send a PR with the new code changes for @azure/arm-timeseriesinsights in https://github.com/azure/azure-sdk-for-js.

Are there any other packages that you are using and see the same issue?

[ghost]

I have not used these directly, but I found the following packages (in addition to arm-timeseriesinsights) where the discriminators are not emitted properly:

• arm-kusto: .+DataConnection
• arm-edgegateway: .+Trigger
• arm-servicemap:
  • CoreResource: CoreResource itself [!?], Machine, Process, Port, ClientGroup, MachineGroup
  • Relationship: Connection, Acceptor

The commonality---with the exception of CoreResource and its self-discriminator---is that they do ``inheritance'' by using the line

...<BASE_TYPE>.type.modelProperties,

in their modelProperties without explicitly indicating the uberParent. Is this method permitted/accepted by AutoRest?

[amarzavery]

new versions of arm-kusto and and arm-timeseriesinsights have been published.