feat(glue-alpha): opaque Schema Type with Schema.custom, and stronger StorageParameter types - #38592
Open
otaviomacedo wants to merge 4 commits into
Open
feat(glue-alpha): opaque Schema Type with Schema.custom, and stronger StorageParameter types#38592otaviomacedo wants to merge 4 commits into
otaviomacedo wants to merge 4 commits into
Conversation
… StorageParameter types
Table schema `Type` was an open public interface, so a column type could be
hand-built as `{ isPrimitive, inputString }` — an uncontrolled escape hatch that
bypassed the `Schema` factories entirely, with no sanctioned way to express a
custom type. `Type` is now an opaque class produced only by `Schema` factories,
and `Schema.custom(inputString, isPrimitive?)` is the explicit escape hatch for
types the factories don't model. `Schema.decimal` now validates precision (1-38)
and scale (0..precision), matching the bounds `char`/`varchar` already enforce.
`StorageParameter` weak primitives are tightened: `custom(key, value)` takes a
`string` value instead of `any`, and `writeKmsKeyId` takes a `kms.IKey` instead of
a raw key-id string (rendering `key.keyId`).
Addresses the Type/Schema.custom, Schema.decimal-bounds, and
StorageParameter-weak-primitives findings from the aws-glue-alpha pre-GA API
review. (The overlapping `StorageParameters`-enum-vs-factory dedup is deferred —
it is entangled with the deprecated-`Table` deletion still in flight.)
BREAKING CHANGE: schema `Type` is now an opaque class; construct column types via
the `Schema` factories or `Schema.custom(...)` rather than `{ isPrimitive,
inputString }` literals. `StorageParameter.custom(key, value)` requires a `string`
value, and `StorageParameter.writeKmsKeyId` takes a `kms.IKey` instead of a string.
Contributor
|
PRs without a linked issue will receive lower priority for review and merging. Please update the description to follow the PR template and include a line like |
aws-cdk-automation
previously requested changes
Aug 18, 2026
otaviomacedo
added
the
pr-linter/exempt-integ-test
aws-cdk-automation
dismissed
their stale review
August 18, 2026 09:01
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
aws-cdk-automation
added
the
pr/needs-maintainer-review
gudipati
reviewed
Aug 20, 2026
Comment on lines
+114
to
+119
| if (precision < 1 || precision > 38 || precision % 1 !== 0) { | ||
| throw new UnscopedValidationError(lit`DecimalPrecisionOutOfRange`, `decimal precision must be a positive integer between 1 and 38, got ${precision}`); | ||
| } | ||
| if (scale !== undefined && (scale < 0 || scale > 38 || scale % 1 !== 0)) { | ||
| throw new UnscopedValidationError(lit`DecimalScaleOutOfRange`, `decimal scale must be an integer between 0 and 38, got ${scale}`); | ||
| } |
Contributor
There was a problem hiding this comment.
do these need any token resolution check (Token.isUnresolved) ?
| public static writeKmsKeyId(value: string): StorageParameter { | ||
| return new StorageParameter('write.kms.key.id', value); | ||
| public static writeKmsKeyId(key: kms.IKeyRef): StorageParameter { | ||
| return new StorageParameter('write.kms.key.id', key.keyRef.keyId); |
Contributor
There was a problem hiding this comment.
can keyArn be used here instead of keyId ?
keyId drops the account and breaks cross-account imported keys -keyArn works for both same and cross-account
Contributor
There was a problem hiding this comment.
Were you able to verify this works while deploying to CFN ?
| * | ||
| * @param inputString the Glue input string for the type (for example `interval_day_to_second`). | ||
| * @param isPrimitive whether the type is a primitive (non-nested) data type. | ||
| * @default isPrimitive - true |
Contributor
There was a problem hiding this comment.
nit: I think @default is not needed here - You can mention the default behaviour on @param itself -
* @param isPrimitive whether the type is a primitive (non-nested) data type. Defaults to true.
|
|
||
| test('decimal rejects scale greater than 38', () => { | ||
| expect(() => Schema.decimal(38, 39)).toThrow(/decimal scale must be an integer between 0 and 38/); | ||
| }); |
Contributor
There was a problem hiding this comment.
nit: should we add a test for non-integer precision/scale ?
test('decimal rejects non-integer precision or scale', () => {
expect(() => Schema.decimal(5.5)).toThrow(/.../);
expect(() => Schema.decimal(10, 2.5)).toThrow(/.../);
});
Contributor
Author
There was a problem hiding this comment.
Yeah, good point.
gudipati
reviewed
Aug 20, 2026
| isPrimitive: true, | ||
| inputString: scale !== undefined ? `decimal(${precision},${scale})` : `decimal(${precision})`, | ||
| }; | ||
| if (Token.isResolved(precision) && Token.isResolved(scale)) { |
Contributor
There was a problem hiding this comment.
if scale is not provided and precision is not a token, then precision check will not be validated.
I think you will need to do the following instead -
if (Token.isResolved(precision) && (precision < 1 || precision > 38 || precision % 1 !== 0)) {
throw new UnscopedValidationError(lit`DecimalPrecisionOutOfRange`, `decimal precision must be a positive integer between 1 and 38, got ${precision}`);
}
if (scale !== undefined && Token.isResolved(scale) && (scale < 0 || scale > 38 || scale % 1 !== 0)) {
throw new UnscopedValidationError(lit`DecimalScaleOutOfRange`, `decimal scale must be an integer between 0 and 38, got ${scale}`);
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Table schema
Typewas an open public interface, so a column type could be hand-built as{ isPrimitive, inputString }— an uncontrolled escape hatch that bypassed theSchemafactories entirely, with no sanctioned way to express a custom type.Typeis now an opaque class produced only bySchemafactories, andSchema.custom(inputString, isPrimitive?)is the explicit escape hatch for types the factories don't model.Schema.decimalnow validates precision (1-38) and scale (0..precision), matching the boundschar/varcharalready enforce.StorageParameterweak primitives are tightened:custom(key, value)takes astringvalue instead ofany, andwriteKmsKeyIdtakes akms.IKeyinstead of a raw key-id string (renderingkey.keyId).BREAKING CHANGE: schema
Typeis now an opaque class; construct column types via theSchemafactories orSchema.custom(...)rather than{ isPrimitive, inputString }literals.StorageParameter.custom(key, value)requires astringvalue, andStorageParameter.writeKmsKeyIdtakes akms.IKeyinstead of a string.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license