Skip to content

chore(s3): use BucketReflection for L1-backed property access#37210

Merged
mergify[bot] merged 2 commits intomainfrom
mrgrain/feat/aws-s3/bucket-reflection
Mar 25, 2026
Merged

chore(s3): use BucketReflection for L1-backed property access#37210
mergify[bot] merged 2 commits intomainfrom
mrgrain/feat/aws-s3/bucket-reflection

Conversation

@mrgrain
Copy link
Copy Markdown
Contributor

@mrgrain mrgrain commented Mar 10, 2026

Reason for this change

Properties like isWebsite, disallowPublicAccess, and bucketWebsiteDomainName on the Bucket class are computed eagerly from constructor props and cached as plain fields. This means the values are frozen at construction time and won't reflect any mutations made to the underlying CfnBucket resource afterwards. It also means each of these properties has its own bespoke derivation logic scattered across the constructor, making it harder to reason about where the source of truth lives.

Description of changes

This introduces BucketReflection, a new class that reads bucket configuration directly from the L1 CfnBucket resource at access time rather than caching prop-derived values. By reading from the L1, the reflection getters provide a single source of truth that stays consistent regardless of whether the bucket was created as an L2 construct, imported, or built directly as a CfnBucket.

The Bucket class is refactored to delegate bucketWebsiteDomainName, isWebsite, and disallowPublicAccess through BucketReflection getters. The disallowPublicAccess setter is now a no-op since the value is derived from the CfnBucket resource via reflection.

BucketReflection is deliberately kept as a non-exported internal class while we iterate on the API shape. Once the pattern stabilizes, it can be promoted to a public API.

Beyond the initial three properties, BucketReflection also exposes policy and encryptionKey getters that search the construct tree for the associated CfnBucketPolicy and CfnKey resources. The previous private helper functions (tryFindBucketPolicyForBucket, tryFindKmsKeyforBucket, tryFindBucketConstruct) in lib/private/reflections.ts are consolidated into BucketReflection and the file is deleted. Internal consumers in default-traits.ts are updated to use BucketReflection instead.

BucketReflection.of() gracefully handles custom IBucketRef implementations that don't have an underlying CfnBucket in the construct tree. The bucket getter throws lazily on access when no L1 is found, while policy falls back to searching from the original IBucketRef, and encryptionKey/disallowPublicAccess return safe defaults. This ensures the class works across L1, L2, and custom bucket implementations.

To support safe property traversal on L1 resources — where any nested value might be an unresolved CDK token — this adds a resolvedGet utility to core/lib/helpers-internal. It walks a dot-separated property path and returns a caller-specified fallback whenever it encounters an IResolvable, rather than silently returning a token object that would be misinterpreted as a truthy value. This distinction between "not configured" (undefined) and "configured but unreadable" (fallback) is important for reflection getters that need to make boolean decisions based on L1 property values.

Describe any new or updated permissions being added

No new permissions.

Description of how you validated changes

Existing tests in bucket-reflection.test.ts (renamed from reflections.test.ts) are updated to use the BucketReflection public API instead of the deleted private helpers. Additional tests verify behavior with custom IBucketRef implementations that lack a CfnBucket: the bucket getter throws, policy still finds associated policies, and encryptionKey returns undefined. New unit tests for resolvedGet cover simple paths, numeric indexing, missing segments, null segments, and resolvable token handling at root, intermediate, and leaf positions.

Checklist


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

@mrgrain mrgrain requested a review from a team as a code owner March 10, 2026 08:50
@mrgrain mrgrain marked this pull request as draft March 10, 2026 08:50
@aws-cdk-automation aws-cdk-automation requested a review from a team March 10, 2026 08:50
@github-actions github-actions bot added the p2 label Mar 10, 2026
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Mar 10, 2026
Copy link
Copy Markdown
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This review is outdated)

@mrgrain mrgrain changed the title feat(aws-s3): add BucketReflection for L1-backed property access feat(s3): add BucketReflection for L1-backed property access Mar 10, 2026
@mrgrain mrgrain force-pushed the mrgrain/feat/aws-s3/bucket-reflection branch from 7eb72f9 to f4b5e6a Compare March 11, 2026 10:30
@mrgrain mrgrain force-pushed the mrgrain/feat/aws-s3/bucket-reflection branch from f4b5e6a to 588239b Compare March 11, 2026 10:42
@mrgrain mrgrain force-pushed the mrgrain/feat/aws-s3/bucket-reflection branch from 588239b to 0518552 Compare March 11, 2026 10:56
@mrgrain mrgrain changed the title feat(s3): add BucketReflection for L1-backed property access feat(s3): BucketReflection for L1-backed property access Mar 11, 2026
@mrgrain mrgrain marked this pull request as ready for review March 11, 2026 10:56
@mrgrain mrgrain force-pushed the mrgrain/feat/aws-s3/bucket-reflection branch from 0518552 to 3b4e815 Compare March 11, 2026 10:57
@mrgrain mrgrain changed the title feat(s3): BucketReflection for L1-backed property access chore(s3): use BucketReflection for L1-backed property access Mar 11, 2026
Comment on lines +67 to +69
get isWebsite(): boolean {
return this._bucket?.websiteConfiguration !== undefined;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to do make a decision about CFN intrinsics and tokens.

techincally speaking, the value of websiteConfiguration could be Fn.if(someCondition, {}, Aws.NO_VALUE).

In that case, we would return true, even though the correct answer is false (but the correct answer from CDK's point of view is "don't know").

How about returning a

class Answer {
  public readonly isTrue: boolean;
  public readonly isFalse: boolean;
  public readonly isUnknown: boolean;
}

Value?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean we already have that with true false and undefined. The issue here is more that the existing code used this check, but never allowed it to be a Fn. Will fix.

@mrgrain mrgrain force-pushed the mrgrain/feat/aws-s3/bucket-reflection branch from b64b5d7 to b7f9d5d Compare March 25, 2026 09:55
@mrgrain mrgrain force-pushed the mrgrain/feat/aws-s3/bucket-reflection branch from b7f9d5d to ee59eb6 Compare March 25, 2026 10:08
@mrgrain mrgrain force-pushed the mrgrain/feat/aws-s3/bucket-reflection branch from ee59eb6 to 2833876 Compare March 25, 2026 10:36
@mrgrain mrgrain requested a review from rix0rrr March 25, 2026 11:14
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Mar 25, 2026

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).

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Mar 25, 2026

Merge Queue Status

  • Entered queue2026-03-25 11:22 UTC · Rule: default-squash
  • Checks passed · in-place
  • Merged2026-03-25 11:53 UTC · at dcb90b507f2dad819e12f617dcc4f9d5c9cc6806

This pull request spent 31 minutes 8 seconds in the queue, including 30 minutes 58 seconds running CI.

Required conditions to merge

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Mar 25, 2026

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).

@mergify mergify bot merged commit 55322ba into main Mar 25, 2026
18 of 19 checks passed
@mergify mergify bot deleted the mrgrain/feat/aws-s3/bucket-reflection branch March 25, 2026 11:53
@github-actions
Copy link
Copy Markdown
Contributor

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2026
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Mar 25, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

contribution/core This is a PR that came from AWS. p2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants