fix: Avoid unstable fqn for TerraformElements

[x3cion]

Hi,

when using dependsOn to depend on a resource with overwritten logical id, the fqn does not fit the resource anymore, as it is generated within the constructor. I took an approach to stabilizing the fqn and also avoiding potential problems when using the fqn before changing the id.

This change might be a breaking change.

Let me know if you need any more infos!

[jsteinich]

@x3cion thanks for putting this together.
One thing I'm curious about is whether fqn needs to be cached at all. A previous implementation didn't have it that way. I'm thinking there is a case where it matters, but the structure you put in place would make it easy to test that out.

This should resolve #1656.

[x3cion]

I looked through the child classes and the only exception I found to the fqn generation was TerraformProvider, which uses an alias. Since the setter and getter for alias stated some weird usage, I didn't try to dig deeper for this case.

My reasoning behind caching the token was, considering how Token.asString() works, a new token would be created for every call to fqn. At the same time my understanding is, fqn's have to be stable identifiers, so the cached _fqnToken is used to disallow changing the fqn base values after fqn has been called once, uncovering a possible issue.

Let me know if I should change the implementation.

[jsteinich]

a new token would be created for every call to fqn

The internals of Token.asString do cache values so that the same token should be returned.

he only exception I found to the fqn generation was TerraformProvider, which uses an alias

TerraformProvider.fqn is only used during synthesis (should probably be private), so that should be fine to leave as is.

At the same time my understanding is, fqn's have to be stable identifiers, so the cached _fqnToken is used to disallow changing the fqn base values after fqn has been called once, uncovering a possible issue.

That is correct. Good catch there.

I'll need to make some adjustments to #1725 to account for these changes, but that will probably be a separate PR after they are both merged.

[x3cion]

@jsteinich the point about Token.asString made me think, so I tried it out. I changed the code:
https://github.com/x3cion/terraform-cdk/blob/fiddle/unstable-fqn/packages/cdktf/lib/terraform-element.ts#L54

This makes the test fail:

  ● fqn is stable

    expect(received).toBe(expected) // Object.is equality

    Expected: "${TfToken[TOKEN.0]}"
    Received: "${TfToken[TOKEN.1]}"

      76 |   const elementWithFQN = new TerraformElement(stack, "test", "valid_type");
      77 |   const fqn = elementWithFQN.fqn;
    > 78 |   expect(elementWithFQN.fqn).toBe(fqn);
         |                              ^
      79 |
      80 |   // May not override logical id after fqn has been requested
      81 |   expect(() => elementWithFQN.overrideLogicalId("new-id")).toThrow();

      at Object.<anonymous> (test/resource.test.ts:78:30)

I was also guessing it works like that when I investigated the code. The entry is actually only cached on the token itself by cachedValue, but there is no global cache as far as I see. Could this uncover some other problems?

[ansgarm]

Thank you for improving this @x3cion 🎉
Found only two small things, nothing major.

[ansgarm]

@x3cion Let's get this merged! Do you want to rebase this branch onto main or should I?

[x3cion]

@ansgarm I rebased now, you'll need to merge, though. :)

[ansgarm]

Will do, once CI passed 👍

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.