Conversation
The AWS Cognito API only accepts a domain prefix that starts and ends with a lowercase alphanumeric character, contains only lowercase alphanumeric characters and hyphens (not leading or trailing), and is 1-63 characters in length. The previous validation accepted any string of lowercase alphanumerics and hyphens, which allowed invalid values that fail at deploy time. Fixes aws#38531
…Object] The out-of-range error interpolated the Size object directly into the message. Size has no toString(), so the value rendered as [object Object] and gave no indication of what was actually passed. Convert to mebibytes in the message, matching the conversion already performed by the range check on the line above. Fixes aws#38621
github-actions
Bot
added
p2
beginning-contributor
…efix-validation Snapshot had a leftover CloudFrontEndpoint output that the integ test source no longer produces, causing integ-runner to flag it as a destructive change in CI.
0276a98 to
26599a9
Compare
aws-cdk-automation
added
the
pr/needs-community-review
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Automated review A maintainer will still review this — treat the notes below as a starting point. This PR is titled as a Lambda fix — replacing an error message that rendered The two code changes themselves are sound: the Lambda message fix is safe and its unit test is tightened to assert the full message, and the Cognito regex aligns exactly with the documented Cognito domain-prefix constraints, with good unit-test coverage of the new boundaries. The issues found are in the new Cognito integration test, which is not registered as a deployable test case and adds little beyond existing happy-path coverage. One item worth reconciling before merge, though outside the scope of the findings above: the title and body describe only the Lambda fix while the diff ships a substantive Cognito change with no linked issue, so the two areas may be better split into separate PRs or the description updated to cover both. 🔴 0 blocking · 🟡 1 recommended · ⚪ 1 optional Files with findings (1)
Generated automatically. React 👍 or 👎 to tell us whether this review helped, so we can improve these reviews. |
![maintainer-for-aws[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
|
|
||
| const app = new App({ | ||
| postCliContext: { | ||
| '@aws-cdk/aws-lambda:useCdkManagedLogGroup': false, | ||
| }, | ||
| }); | ||
| const stack = new Stack(app, 'integ-user-pool-domain-prefix-validation'); | ||
|
|
||
| const userpool = new UserPool(stack, 'UserPool', { | ||
| removalPolicy: RemovalPolicy.DESTROY, | ||
| }); | ||
|
|
||
| // A prefix that exercises the tightened validation: hyphens in the middle are | ||
| // allowed, but the prefix must start and end with a lowercase alphanumeric. | ||
| const domain = userpool.addDomain('Domain', { | ||
| cognitoDomain: { | ||
| domainPrefix: 'cdk-integ-user-pool-domain', | ||
| }, | ||
| }); | ||
|
|
||
| new CfnOutput(stack, 'Domain', { | ||
| value: domain.domainName, | ||
| }); |
There was a problem hiding this comment.
🟡 Recommended — This new integration test builds an App, Stack, and CfnOutput but never instantiates the IntegTest construct from @aws-cdk/integ-tests-alpha. The integ-runner only treats a file as a deployable test case when it registers one, so as written this file is never deployed by yarn integ and degrades to a bare synth snapshot — the sibling integ.user-pool-domain-managed-login.ts registers a test case (its snapshot contains a DeployAssert stack), while this PR's snapshot template contains only the UserPool and UserPoolDomain and no assertion stack, confirming the omission (INTEGRATION_TESTS.md).
Suggested change: import * as integ from '@aws-cdk/integ-tests-alpha';
// ...
new integ.IntegTest(app, 'IntegTest', { testCases: [stack] });
// then regenerate the snapshot with yarn integ --update-on-failed
| // allowed, but the prefix must start and end with a lowercase alphanumeric. | ||
| const domain = userpool.addDomain('Domain', { | ||
| cognitoDomain: { | ||
| domainPrefix: 'cdk-integ-user-pool-domain', | ||
| }, |
There was a problem hiding this comment.
⚪ Optional — The behavior this PR changes is a synth-time validation tightening of domainPrefix, and a deploy-time integration test cannot assert the rejections (invalid prefixes throw at synth, before any deploy). This test therefore only exercises the happy path of deploying a UserPoolDomain with a valid prefix, which the existing integ.user-pool-domain-managed-login.ts already covers, while the negative and boundary behavior is fully exercised by the new unit tests. Consider whether a separate integ test earns its place here.
This branch was successfully deployed
3 participants
Issue # (if applicable)
Closes #38621.
Reason for this change
When
ephemeralStorageSizeis outside the allowed range, the validation error interpolated theSizeobject directly into the message.Sizehas notoString(), so the value rendered as[object Object]and gave no indication of what was actually passed.Description of changes
Convert the size to mebibytes in the error message, matching the conversion the range check on the line above already performs. The value is guaranteed to be resolved (guarded by
!isUnresolved()) and convertible (a non-convertibleSizealready throws in the condition), so this introduces no new failure path.Updated the existing unit test to assert the full message including the received value (
received 511 MiB.).Describe any new or updated prerequisites
None.
Any behavior changes
Only the error message text — it now states the received size instead of
[object Object].Is this a breaking change to the CDK CLI?
No
Is this a breaking change to the library or any generated client interfaces?
No