core: CfnResource.isCfnResource is too permissive

[michanto]

Describe the bug

I created a construct class with a cfnResourceType property. I use that type to search for a CfnResource under the parent of that construct. But my non-CfnResource class is recognized as a CfnResource by the CDK because it has a cfnResourceType property! Wrong!

Expected Behavior

public static isCfnResource(x: any): x is CfnResource {
    return CfnElement.isCfnElement(x) && x.cfnResourceType !== undefined;
  }

That would be a lot better because isCfnElement has an RTTI symbol property.

Current Behavior

public static isCfnResource(x: any): x is CfnResource {
    return x !== null && typeof(x) === 'object' && x.cfnResourceType !== undefined;
  }

Reproduction Steps

class BucketHelperClass extends Construct {
  readonly cfnResourceType = CfnBucket.CFN_RESOURCE_TYPE_NAME
}
let helper = new BucketHelperClass(new App(), "Helper")
if (CfnResource.isCfnResource(helper)) {
  throw new Error('No it is not a CfnResource!');
}

Possible Solution

Either this:

public static isCfnResource(x: any): x is CfnResource {
    return CfnElement.isCfnElement(x) && x.cfnResourceType !== undefined;
  }

Or maybe:

const CFN_RESOURCE_SYMBOL = Symbol.for('@aws-cdk/core.CfnResource');

Additional Information/Context

When I try to synthesize a stack with a fake CfnResource, I get the following error:

[CDK] TypeError: source.addDependency is not a function

CDK CLI Version

2.120.0 (build 58b90c4)

Framework Version

No response

Node.js Version

10.2.5

OS

MacOS

Language

TypeScript

Language Version

No response

Other information

No response

[pahud]

Agree. But looking at this discussion

#28001 (review)

What do you think about it? @tmokmss @go-to-k ?

[michanto]

Both could be supported by just adding isCfnElement to the existing?

Or you could adopt my Construct RTTI class

[tmokmss]

I agree that the current state is undesirable and should be fixed. However, I can't think of a way to make sure it's not a breaking change. I think the proposed change will not cause any problem, but there are so many cdk users in the world...

[go-to-k]

I have the same opinion with @tmokmss . (But I think it's probably OK.)

[michanto]

Even if you decide not to fix isCfnResource itself, you should fix the addDependency error on synthesis. That's definitely a bug.