I think there are cases where this could be a breaking change. There could be cases where Token.isUnresolved returns true, but the value in the template is still a resolved string.

For example

const parameterName = Lazy.string({ produce: () => 'myParameterName' });

Token.isUnresolved(parameterName) will return true, but the value that will be output will be

{
  "Type": "AWS::SSM::Parameter::Value<STRING>",
  "Default": "myParameterName",
}

We might need to use Tokenization.isResolvable instead.

Hi @corymhall thanks! I confirmed you are right (I added a test case and it failed.)

Now I tried Tokenization.isResolvable but for some reason it was not evaluated as true for a cross stack reference. I'll dive deep the reason later.

Perhaps we need to resolve the token first, and check if it contains CFn intrinsics, as you've done on cfn-resources.ts.

Tokenization.isResolvable(attrs.parameterName) won't work as expected because typeof(attrs.parameterName) is always 'string', which fails this evaluation.

So to determine whether a parameter name can be resolved to a concrete string, we can actually resolve it. Like this:

if (typeof Stack.of(scope).resolve(attrs.parameterName) != 'string') 

However, if we resolve the token here, it means a lazy string isn't actually evaluated lazily enough. Instead it's evaluated immediately to determine the condition. I'm not sure if it will break something though. A similar approach is also used in s3-deployment module, so maybe it's just fine.

Alternatively, we can create a method like isLazyString to determine if a string is not a lazy value without resolving it. But then we cannot cover the case when Lazy.string produces cfn intrinsics like this:

Lazy.string({ produce: () => someResource.ref });

Another consideration is if we should really care this edge case. I think almost all of the devs don't use a Lazy string as a ssm parameter name to import it. At least we can fix it if someone would report an issue.

Hope you can suggest which way we should go, thanks! @corymhall

It seems like what we are really trying to do is determine if the value is an intrinsic or not. In that case I think it might be best to just add a isIntrinsic method to Intrinsic

Thanks for the insight! I agree with that the only case we have to handle is when parameterName contains intrinsics.

But attrs.parameterName is just a string (like ${Token[TOKEN.274]}) before resolution. Is it possible to determine if the string is actually an instrinsic without resolving it? (And I realized my isLazyString solution also falls in this question...)

@corymhall I see, but isn't it too complicated from users' perspective? We cannot tell which case works and which case doesn't.

If only we could stop using CfnParameter and use CfnDynamicReference instead for all cases... I see no point to use CfnParameter here. (Actually, isn't that the purpose of feature flags?)

There are some limitations #22239 (comment)

@corymhall Thanks, I didn't know that!

So how about letting users choose which they use, parameter or dynamic reference? We'll add a property like forceDynamicReference?: boolean (default to false) to CommonStringParameterAttributes. This is kind of a leaky abstraction, but it should at least solve all the problem above. Plus we can easily ensure there is no breaking change, without adding any feature flag.

@tmokmss lets do both. Lets cover what we can (my comment above) and then allow the user to force the dynamic reference. Neither should need a feature flag.

@corymhall Now dynamic reference is used only when cdk.Fn is explicitly used. It can address the original use case reported on the issue.

As for other parameter name containing a token, we cannot decide if it contains CFn intrinsics or not without resolving the token, which is not ideal for lazy tokens. In that case, we just use CfnParameter, and letting a user to force dynamic reference by passing a flag.

I made the change to only fromStringParameterAttributes, not fromListParameterAttributes. I think we can do this in another PR.