RFC 201: Refactoring support

[rix0rrr]

This is a request for comments about refactoring support. See #201 for
additional details.

APIs are signed off by @RomainMuller.

Rendered Link

---

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

[straygar]

Any particular reason this is the case?

[eladb]

API thoughts:

• The term "refactor" doesn't necessarily mean "rename". Perhaps be a bit more explicit about the name of this method.
• It might be useful to also support directly referencing the target object (less potential for mistakes based on string matching)
• Why do we need this as the first argument if we are already in the node?

this.node.moveToPath("MyBucket", "MyConstruct/MyBucket");
this.node.moveTo("MyBucket", myConstruct.myBucket);

[Chriscbr]

I feel like refactorings in the other direction could also be useful -- for example, if I have an app structured like this, and I wanted to move ChildA to be part of the root (while keeping its old ID at synthesis-time).

- root
  - Parent1
    - ChildA
    - ChildB

---

side note: Reading the RFC, I wondered if we would gain any expressive power in constructs by taking a page from the world of filesystems and introducing a symlink-like concept. This would be a special construct that redirects to some other path in the construct tree, and would be handled specially during construct tree traversal. Something like:

new Link(this, "MyBucket", "MyConstruct/MyBucket");
new Link(this, "MyBucket", myConstruct.myBucket);
new Portal(this, "MyBucket", myConstruct.myBucket);

But I'm not sure if this API as much sense 🤷

[Chriscbr]

How do you imagine this error message might look like?

[Chriscbr]

Since the API is a method call and not "new"-ing something, it reads to me like an operation, not a declaration, so as a user I'd expect I could call this multiple times. Would something like this be possible?

this.node.refactor(this, 'MyBucket', 'MyConstructV1/MyBucket');
this.node.refactor(this, 'MyConstructV1/MyBucket', 'MyConstructV2/MyBucket');

[NetaNir]

First, thank you so much for thinking about this problem. As someone who owns several large scale application managed by the CDK, I can attest to how painful hitting this can be, especially how risky it is, as sometimes, the only way out (beside reverting the code) is to delete resources from the console 😱.

The CDK default behavior for allocation logical ids is unsafe, its a good getting starting experience, but it comes at the expense of long term maintenance pain. For that reason I think the solution should be on the getting started path, and not after the user hits this sharp edge (thinking of an emergency deployment, not so fun realizing you cant deploy your app). Having said that, I realize this might not be possible.

This RFC propose a solution in the construct level, but the problem mostly impacts the AWS CDK users, due to how CloudFormation handles logical ids, so maybe it will be easier to solve it on the Stack level.

The "easiest" solution from the framework perspective would be to simply use the user provided id, and let them deal with the consequence of collisions. In case of a collisions, a detailed error message will be thrown, and the user will have to fix it. I agree that this might not be a great getting started experience, but this has the advantage of letting the user make a decision when they build a production app. Working backward from a (imaginary) CDK best practices section:

Things to consider when building a production CDK application

By default the CDK will allocate logical ids based on the construct path in the construct tree. This means that if you move your construct around the code, it might result in a change to its logical id, that will trigger a resource replacement, if you use "named resources", e.g. BucketName, the deployment will fail when trying to create a new resource. If this is indeed an issue for you, you can opt out of this CDK default behavior by setting the context key:

construct-id:no-default-generation

This will override the default behavior and use the user provided id (the second arg when initializing a construct). Note that this means that you will have to manage the Ids uniqueness yourself.

While not perfect it will allow the user to make a concise decision before deploying a full application.

Also worth noting that there is support for refactoring today, by using the Default id on the added layer, from constructs

/**
 * Resources with this ID are complete hidden from the logical ID calculation.
 */
const HIDDEN_ID = 'Default';

/**
 * Calculates the construct uid based on path components.
 *
 * Components named `Default` (case sensitive) are excluded from uid calculation
 * to allow tree refactorings.
 *

[royby-cyberark]

I was just wondering about this and tried to ask in discussions (aws/aws-cdk#22016)
and I suggested a way of side stepping the entire logical ids issues when refactoring.

My suggestion was to avoid using construct classes (avoid inheriting Construct) and instead arrange your code with regular classes which takes the scope of the stack/parent construct and use this as the scope for all child resources.

Pros:

• No logical id changes when refactoring
• get all the value of refactoring and code encapsulation

Cons:

• You lose the CloudFormation hierarchy which comes out of the box
  • you no longer get the ids of the parent concatenated
  • you can't see the nice hierarchy in CloudFormatoin new Tree view which is really nice (also other tools that might do the same things for visualization)

Let me expand on this a bit - i'm not suggesting to have everything as a flat stack, but do something which i'll call "top level domain structuring" in which i'll create top level constructs in which it is highly unlikely for things to move between such top level constructs. and in them, I will avoid using construct, lose the internal hierarchy but be able to refactor the code without issues.

What do you think about this approach? do you think it goes against the "spirit" of cdk/cloudformation, or does it seem like a legitimate approach?

Thank you.

[humanzz]

How different is this from renaming logical ids? Is the difference that relocating can happen at some intermediate construct level therefore might save multiple logical Id renames?

[jonodrew]

Was this ever merged?