feat(core): builtin PropertyMergeStrategys are now compatible with deferred Box values#37844
Conversation
|
👋 It looks like your PR description follows the template but is missing a valid issue number in the first section. PRs without a linked issue will receive lower priority for review and merging. Please update the description to include a reference like |
60b515f to
7701505
Compare
7701505 to
fdebe86
Compare
PropertyMergeStrategys are now compatible with deferred Box values
fdebe86 to
f2821ec
Compare
PropertyMergeStrategys are now compatible with deferred Box valuesPropertyMergeStrategys are now compatible with deferred Box values
PropertyMergeStrategys are now compatible with deferred Box valuesPropertyMergeStrategys are now compatible with deferred Box values
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
|
||
| ### Deferred Values (Boxes) | ||
|
|
||
| Property mixins support **Box-backed values**. Most L2 constructs in `aws-cdk-lib` use Boxes internally to defer property computation until synthesis time. When a mixin encounters a Box on either the target or source side, the merge is automatically deferred — the merge strategy runs once the Box resolves, ensuring it operates on final values. |
There was a problem hiding this comment.
The point of the Box refactor is that we can capture stack traces of the "moment of instruction to modify a value". This sounds like it makes Property Mixins behave like Aspects (i.e., run lazily).
Are we retaining the stack trace of where the Mixin got applied?
There was a problem hiding this comment.
No. Aspects don't magically support deferred values either, since deferred values are resolved after Aspects.
const table = new dynamodb.TableV2(this, "MinimalAppTable", {
partitionKey: { name: "pk", type: dynamodb.AttributeType.STRING },
});
cdk.Aspects.of(table).add({
visit: function (node: IConstruct): void {
if (!dynamodb.CfnGlobalTable.isCfnGlobalTable(node)) {
return;
}
if (isBox(node.replicas)) {
throw Error("Replicas is a box");
}
}
}, {
priority: cdk.AspectPriority.MUTATING
})Will result in Error: Replicas is a box.
With Aspects, we didn't really allow or encourage construct mutation. It only really supports construct tree mutation. So this wasn't really an issue before, because you only had the choice to either
- inspect
tableand fail becausetable.replicasis not a readable value - remove the table and replace it with something else
In practice, I'd wager that L2s that heavily depend on deferred values didn't get used much with Aspects at all.
The Box proposal itself is was made manipulation of deferred values possible in the first place! That's a huge win for everyone concerned. Some of our existing L2 code could be rewritten much better now as well (if we cared enough). This PR just makes something automatically available that users could write themselves today. Also the readme just shows something that ought to have worked already in the first place, if we didn't rely on Lazy's so much.
There was a problem hiding this comment.
Are we retaining the stack trace of where the Mixin got applied?
No special provisions for that at the moment. But every L1 property assignment already points to the .with call in userland code.
| // L1 prop: pointInTimeRecoverySpecification is an object | ||
| pointInTimeRecoverySpecification: { pointInTimeRecoveryEnabled: true }, | ||
| }], | ||
| }, { strategy: PropertyMergeStrategy.combine({ arrays: ArrayMergeStrategy.append() }) })); |
There was a problem hiding this comment.
Oof, this declaration 😓
(I know why but it still feels quite heavy)
There was a problem hiding this comment.
I don't have a better solution just now, but I have a vague idea of introducing a "smart" merge strategy that is aware of resource types and what they need.
The direct alternative to this syntax would be a mutex list of booleans, which I don't think is better. Do you have other suggestions?
In any case, the array change is not new in this PR. It's just documented here.
There was a problem hiding this comment.
I don't have a better suggestion. Mutex booleans and judicious choice of defaults is probably the best we've got.
| // PointInTimeRecoveryEnabled: true | ||
| ``` | ||
|
|
||
| **Lazys and Tokens are not supported.** If a property value is a `Lazy` or raw `Token` (not a Box), the merge strategy cannot inspect or defer it — the mixin will treat it as an opaque value and replace it. Most L2 constructs already use Boxes, but if you encounter one that doesn't, please [open an issue](https://github.com/aws/aws-cdk/issues/new?template=bug-report.yml) so we can migrate it. |
There was a problem hiding this comment.
Can we error on this in some way, so at least it doesn't pass unnoticed?
This feels like asking users to check for something they can't really see...
There was a problem hiding this comment.
Tokens we can probably reverse. For Lazys I was looking to investigate if we can change their internal implementation to be Boxes.
In any case, this is existing behavior. Happy to think about erroring here (though would be breaking), but would prefer to do this on a separate PR. The proposed change makes the situation strictly better by now not opaquely failing anymore in many cases.
| boxes.source = sourceValue; | ||
| } | ||
|
|
||
| (target as any)[key] = Box.combine(boxes, (resolved) => { |
There was a problem hiding this comment.
Ohhh I see. Clever.
Since this goes through the L1 setter (afaiu), this seems to ultimately desugar to:
oldBox.set(combinedBox);So: relying on the Box setter allowing a Box to be set into itself. Does that actually work?
There was a problem hiding this comment.
No, the setter doesn't call oldBox.set(...). We are replacing the whole Box with new one.
The L1 line is
this._replicas = value;with value being a new ComputedBox.
There was a problem hiding this comment.
Oh, so we poke through the L1 setter to directly access the backing field?
There was a problem hiding this comment.
Not sure what you mean by poking through.
We are transparently using the L1 getter and setter. This part has nothing to do with boxes, and all to do with capturing a stack trace when calling the setter. The original L2 construct Box has stack traces disabled anyway via @noBoxStackTraces.
There was a problem hiding this comment.
We create a brand new Combined Box from the original target and the new data source. That Combined Box will contain the stack trace of the original box if any was present. The setter call will collect the stack trace of the assignment. Both end up as aws:cdk:propertyAssignment entries.
| // PointInTimeRecoveryEnabled: true | ||
| ``` | ||
|
|
||
| **Lazys and Tokens are not supported.** If a property value is a `Lazy` or raw `Token` (not a Box), the merge strategy cannot inspect or defer it — the mixin will treat it as an opaque value and replace it. Most L2 constructs already use Boxes, but if you encounter one that doesn't, please [open an issue](https://github.com/aws/aws-cdk/issues/new?template=bug-report.yml) so we can migrate it. |
There was a problem hiding this comment.
In case of tokens, would it make sense to Tokenization.reverse them and, if the obtained value is a box, run with it?
There was a problem hiding this comment.
Good idea. Will follow-up in a second PR if that's okay.
…trategy.combine() is now wrapped in BoxSafeMergeStrategy,\nwhich defers per-key merging when either the target or source value is a\nBox. This ensures merge strategies operate on resolved values when L2\nconstructs use Boxes internally.\n\nArrayMergeStrategy.replaceByKey() is similarly wrapped in\nBoxSafeArrayStrategy to handle Box elements that need key comparison.\n\nOther array strategies (replace, append, prepend, replaceByIndex) don't\nneed box-safety since they never inspect element contents.
f2821ec to
f9950cd
Compare
f9950cd to
83325e3
Compare
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Merge Queue Status
This pull request spent 2 hours 20 minutes 11 seconds in the queue, including 47 minutes 25 seconds running CI. Required conditions to merge
|
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
N/A
Reason for this change
L2 constructs use Boxes internally to defer property values until synthesis time. When a property mixin tried to merge into a Box-backed value (e.g.
TableV2.replicas), the merge strategy would see an opaqueIResolvableobject instead of the actual array or object, causing incorrect results.Description of changes
Introduces two new wrapper classes that make merge strategies box-safe without mixing concerns into the core merge logic:
BoxSafeMergeStrategywraps anyIMergeStrategy. For each key, it checks whether the target or source value is a Box. If so, it defers the merge viaBox.combineso the delegate strategy operates on resolved values. Non-Box values are passed through to the delegate directly.BoxSafeArrayStrategywraps anyIArrayMergeStrategy. It checks whether any element in either array is a Box, and if so defers the entire array merge. This is only needed forreplaceByKey, which reads element properties to match them — other array strategies just shuffle elements positionally and don't need it.PropertyMergeStrategy.combine()is now wrapped inBoxSafeMergeStrategy, andArrayMergeStrategy.replaceByKey()is wrapped inBoxSafeArrayStrategy. The core strategy classes remain pure and unaware of Boxes.Also documents Box support in the public API JSDoc and adds a "Deferred Values" section to the
@aws-cdk/cfn-property-mixinsREADME explaining what is and isn't supported.Describe any new or updated permissions being added
N/A
Description of how you validated changes
Unit tests in
core/test/mixins/property-merge-strategy.box-safety.test.tscovering target-is-Box, source-is-Box, both-are-Boxes, Box elements inside arrays, deferred mutations, and derived boxes. Integration tests in@aws-cdk/cfn-property-mixinsusingTableV2with all array merge strategies against Box-backed replicas.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license