fix(cfn-include): fails to load SAM resources

[skinny85]

We incorrectly handled union-types in the fromCloudFormation() generated code.
Also we merged the 'Transform' sections of the CloudFormation template incorrectly,
which has also been fixed in this change.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[rix0rrr]

Man this code is complicated. Both the task it's trying to achieve, but also in the implementation.

I trust that it does what it needs to do. Got some notes on potential cleanup strategies, do with those as you will.

[rix0rrr]

Accidentally quadratic here. Not sure the set ever grows big enough for it to be an issue, but hope you thought about this and weighed the risk intentionally.

[skinny85]

Yes. I don't think this will ever realistically contain more than 2 elements.

[rix0rrr]

If you made a function that returns unique variable names (and honestly, it could be global, why not?) you could reduce on the complexity of this.deeperCopy() and below the 2 deeperCopy()s.

In this generated code, there's no requirement that we use the least amount of variable names possible.

Just put this somewhere in this file and use it:

const freshVariable = (() => {
  let ctr = 1;
  return () => `var${ctr++}`;
}());

If you really wanted to be fancy you could make it scoped and reduce the counter again after the block to really end up with minimal variable names:

let ctr = 0;
function withFreshVariable<A>(block: (var: string) => A) {
  ctr += 1;
  const ret = block(`var${ctr}`);
  ctr -= 1;
  return ret;
}

// To be used as:
return withFreshVariable(var1 => withFreshVariable(var2 => `const ${var2} = ${var1}`));

// Or even:
function renderLambda(block: (var: string) => string) {
  return withFreshVariable((var) => `(${var}: any) => ${block(var)}`);
}

// To be used as:
return renderLambda(v => `${FromCfnVisitor.fromExpression(v).visitAtom(itemType)}`);

[skinny85]

I tried your withFreshVariable refactoring, but it doesn't really do what you want in this case:

        const arg1 = `prop${this.depth}`;
        const arg2 = `prop${this.depth + 1}`;
        const mappers = itemTypes
          // we need a copy 2 levels deep, as we're already using prop2 in arg2 here
          .map(type => `(${arg2}: any) => ${this.deeperCopy(arg1).deeperCopy(arg2).visitAtom(type)}`)
          .join(', ');

        return `${CFN_PARSE}.FromCloudFormation.getArray(${this.baseExpression}, ` +
          `(${arg1}: any) => ${CFN_PARSE}.FromCloudFormation.getTypeUnion([${validatorNames}], [${mappers}], ${arg1})` +
          ')';

withFreshVariable does nothing for us here (they would have to be nested calls).

I did a small refactoring of the logic though, and I think it's much more clear now. Let me know how you like it compared to the old one.

[rix0rrr]

withFreshVariable does nothing for us here (they would have to be nested calls).

Yes, they have to be nested calls. That's why it takes a block and returns the block's return value. Sorry if that wasn't clear.

[rix0rrr]

I had to spent a solid 20 minutes perusing the code to see why you needed 2 copies here. Let's not? :)

[skinny85]

Let me know if it's clearer why that's needed in this new version.

[rix0rrr]

Feels like this function would be better off taking an array of pairs, rather than a pair of arrays?

[skinny85]

I modeled this after the existing unionMapper function in runtime.ts.

Making it an array of pairs would make it much more difficult to write code like this:

        const validatorNames = types.map(type => genspec.validatorName(type).fqn).join(', ');

        const arg = `prop${this.depth}`;
        const mappers = types
          .map(type => `(${arg}: any) => ${this.deeperCopy(arg).visitAtom(type)}`)
          .join(', ');

        return `${CFN_PARSE}.FromCloudFormation.getTypeUnion([${validatorNames}], [${mappers}], ${this.baseExpression})`;

With the current modeling, you can easily generate separate validators and mappers. Making getTypeUnion () take an array of pairs would make this a fair bit more complicated.

[rix0rrr]

Once we get rid of the depth, this doesn't really need any instance variables anymore, and the "copy" word implies a lot of statekeeping that is making me uncomfortable. Wouldn't factory functions make more sense?

FromCloudFormationFactoryVisitor.fromExpression(...)
// ^--- this is a little long then, might want to rename it

[skinny85]

Can't get rid of depth because of #9442 (comment)

[rix0rrr]

Hey something just popped into my head -- if the thing the mappers return are themselves functions (which I think they are), then we don't need to render any lambdas, do we?

Because:

(x: any) => doAThing(x)

// The same as
doAThing

// (modulo "this" binding because... you know... JavaScript)

Might simplify the code some more. Do we really really need the lambdas?

[skinny85]

Hey something just popped into my head -- if the thing the mappers return are themselves functions (which I think they are), then we don't need to render any lambdas, do we?

Because:

(x: any) => doAThing(x)

// The same as
doAThing

// (modulo "this" binding because... you know... JavaScript)

Might simplify the code some more. Do we really really need the lambdas?

We can't really render functions in the visit* methods (otherwise, that's how I would do it from the start).

Let's take CloudFront's CfnDistributionProps. They take a required complex object as one of its properties, DistributionConfigProperty. So the function for them has to look like this:

function CfnDistributionPropsFromCloudFormation(properties: any): CfnDistributionProps {
    properties = properties || {};
    return {
        distributionConfig: DistributionConfigPropertyFromCloudFormation(properties.DistributionConfig),
        // ...
    };
}

where DistributionConfigPropertyFromCloudFormation has to return DistributionConfigProperty in order for the code to compile.

However, if there is a different property that takes an optional DistributionConfigProperty, the code has to look like this:

function SomePropsFromCloudFormation(properties: any): SomeProps {
    properties = properties || {};
    return {
        distributionConfig: properties.DistributionConfig == null ? undefined : DistributionConfigPropertyFromCloudFormation(properties.DistributionConfig),
        // ...
    };
}

So visitAtom has to return either DistributionConfigPropertyFromCloudFormation(properties.DistributionConfig), or properties.DistributionConfig == null ? undefined : DistributionConfigPropertyFromCloudFormation(properties.DistributionConfig). It cannot just return DistributionConfigPropertyFromCloudFormation (the function itself).

[rix0rrr]

So visitAtom has to return either DistributionConfigPropertyFromCloudFormation(properties.DistributionConfig), or properties.DistributionConfig == null ? undefined : DistributionConfigPropertyFromCloudFormation(properties.DistributionConfig). It cannot just return DistributionConfigPropertyFromCloudFormation (the function itself).

Ah, but using higher order functions we could:

function fromOptional(innerMapper: (x: any) => any) {
  return (x: any) => x != null ? innerMapper(x) : undefined;
}

function SomePropsFromCloudFormation(properties: any): SomeProps {
    properties = properties || {};
    return {
        distributionConfig: fromOptional(DistributionConfigPropertyFromCloudFormation)(properties.DistributionConfig),
        // ...
    };
}

I'm not saying there has to be exactly one function that we return the name of. I'm saying we return an expression that evaluates to a function. In this case: fromOptional(DistributionConfigPropertyFromCloudFormation).

[rix0rrr]

Again, this has the goal of JUST increasing depth, which is not super clear to me.

[rix0rrr]

I added a commit showing what I mean, with a description of the change in the commit body.

Having the visitor return a function which is then applied to the actual value is also how the serializer works, so it makes for a nice symmetry, see here:

https://github.com/aws/aws-cdk/blob/master/tools/cfn2ts/lib/codegen.ts#L502

If you don't like it you can revert it (but I hope you wouldn't).

Might have made some mistakes around tags, btw. Although I guess the only reason to have the 'as any' there was to escape the type system, and it seems to work equally well at the top level...

[skinny85]

I added a commit showing what I mean, with a description of the change in the commit body.

Having the visitor return a function which is then applied to the actual value is also how the serializer works, so it makes for a nice symmetry, see here:

https://github.com/aws/aws-cdk/blob/master/tools/cfn2ts/lib/codegen.ts#L502

If you don't like it you can revert it (but I hope you wouldn't).

Might have made some mistakes around tags, btw. Although I guess the only reason to have the 'as any' there was to escape the type system, and it seems to work equally well at the top level...

Looks nice, thanks! I agree it's simpler.

For tags, all of the tests pass, so I assume it's working 😃.

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: dcc2899
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).