This repository was archived by the owner on Feb 17, 2025. It is now read-only.
fix: primitive metadata values cannot be loaded in jsii languages #121
Conversation
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
iliapolo
commented
Jan 8, 2025
| */ | ||
| export type StackTagsMetadataEntry = Tag[]; | ||
|
|
||
| export type PrimitiveType = boolean | number | string; |
There was a problem hiding this comment.
I really wanted to put a comment here, but its just too nasty to explain. An entire PR is needed to understand why this is necessary.
There was a problem hiding this comment.
Link the PR as a code comment? Just an idea.
iliapolo
changed the title
rix0rrr
reviewed
Jan 8, 2025
schema/version.json
Outdated
| "schemaHash": "f0ccc3212331af8a1c75830878d218fd667e5957063831be3b5bfd803b25f0cc", | ||
| "revision": 39 | ||
| "schemaHash": "bf762e5da559c685e06892ef65fdfc33d1c4bd53d4eb07d4a326d476ac58ae25", | ||
| "revision": 40 |
There was a problem hiding this comment.
Should(n't) we release this as a patch on 39? It's not actually adding functionality, it's just more accurately describing current behavior.
rix0rrr
approved these changes
Jan 8, 2025
Signed-off-by: github-actions <[email protected]>
1 task
mergify bot
pushed a commit
to aws/aws-cdk
that referenced
this pull request
Jan 21, 2025
…32998) ### Reason for this change We are publishing the `cx-api` package twice: Once as a standalone package `@aws-cdk/cx-api` and once as part of the construct library under `aws-cdk-lib/cx-api`. The code is copied during the release and the same versions of the packages will have the same code. However this makes it difficult for other packages to take a type dependency on types from this package. The most common class that's used from `cx-api` is `CloudAssembly` - the result of `app.synth()`. Previously a package had to take a dependency on the very large `aws-cdk-lib` just to use a single type. It would be better if other packages could instead depend on the smaller, much more focused `@aws-cdk/cx-api` package. ### Description of changes This adds the same mechanism to `CloudAssembly` to detect cross-library compatibility, that we already use for constructs like `Stack` or `App`. In TypeScript, it's now possible for a consuming package to receive an object from either package and check at runtime if it satisfies the requirements. We cannot get around type checking with this. Instead we introduce a new type `ICloudAssembly` into the Cloud Assembly Schema package (cdklabs/cloud-assembly-schema#133). This interface only declares a single property: `directory`. Consumers can use this type to indicate where they would like to receive a `CloudAssembly`. They can then use runtime code to either confirm a provided object already satisfies the requirements or fallback to creating a new `CloudAssembly` from the directory. Because the `CloudAssembly` in `cxapi` implements the new interface, this approach will work in all jsii languages. In TypeScript it's even compatible with older version of `aws-cdk-lib`. Jsii language will only support this going forward. #### Allowed breaking changes ``` weakened:aws-cdk-lib.cloud_assembly_schema.MetadataEntry weakened:aws-cdk-lib.cx_api.MetadataEntryResult ``` This PR updates the version of `@aws-cdk/cloud-assembly-schema` to make new of the new interface. However the update also includes a change to `MetadataEntry` which was introduced in cdklabs/cloud-assembly-schema#121. That change is weakening a type, because in #31041, the CDK started emitting booleans and numbers as metadata values. But since these types weren't officially declared in the schema, jsii runtime type checking failed to load them. The fix was to officially extend the type union to include `boolean` and `number` primitive values. This is considered breaking, because when used as an output any consuming code will now need to account for the possibility of the value being a `boolean` or `number`. In static languages, the type would already have been treated as a generic Object with required runtime checks. ### Describe any new or updated permissions being added n/a ### Description of how you validated changes Unit tests. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
moelasmar
pushed a commit
to aws/aws-cdk
that referenced
this pull request
Jan 24, 2025
…32998) ### Reason for this change We are publishing the `cx-api` package twice: Once as a standalone package `@aws-cdk/cx-api` and once as part of the construct library under `aws-cdk-lib/cx-api`. The code is copied during the release and the same versions of the packages will have the same code. However this makes it difficult for other packages to take a type dependency on types from this package. The most common class that's used from `cx-api` is `CloudAssembly` - the result of `app.synth()`. Previously a package had to take a dependency on the very large `aws-cdk-lib` just to use a single type. It would be better if other packages could instead depend on the smaller, much more focused `@aws-cdk/cx-api` package. ### Description of changes This adds the same mechanism to `CloudAssembly` to detect cross-library compatibility, that we already use for constructs like `Stack` or `App`. In TypeScript, it's now possible for a consuming package to receive an object from either package and check at runtime if it satisfies the requirements. We cannot get around type checking with this. Instead we introduce a new type `ICloudAssembly` into the Cloud Assembly Schema package (cdklabs/cloud-assembly-schema#133). This interface only declares a single property: `directory`. Consumers can use this type to indicate where they would like to receive a `CloudAssembly`. They can then use runtime code to either confirm a provided object already satisfies the requirements or fallback to creating a new `CloudAssembly` from the directory. Because the `CloudAssembly` in `cxapi` implements the new interface, this approach will work in all jsii languages. In TypeScript it's even compatible with older version of `aws-cdk-lib`. Jsii language will only support this going forward. #### Allowed breaking changes ``` weakened:aws-cdk-lib.cloud_assembly_schema.MetadataEntry weakened:aws-cdk-lib.cx_api.MetadataEntryResult ``` This PR updates the version of `@aws-cdk/cloud-assembly-schema` to make new of the new interface. However the update also includes a change to `MetadataEntry` which was introduced in cdklabs/cloud-assembly-schema#121. That change is weakening a type, because in #31041, the CDK started emitting booleans and numbers as metadata values. But since these types weren't officially declared in the schema, jsii runtime type checking failed to load them. The fix was to officially extend the type union to include `boolean` and `number` primitive values. This is considered breaking, because when used as an output any consuming code will now need to account for the possibility of the value being a `boolean` or `number`. In static languages, the type would already have been treated as a generic Object with required runtime checks. ### Describe any new or updated permissions being added n/a ### Description of how you validated changes Unit tests. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* (cherry picked from commit 94ba772)
moelasmar
pushed a commit
to aws/aws-cdk
that referenced
this pull request
Jan 24, 2025
…32998) ### Reason for this change We are publishing the `cx-api` package twice: Once as a standalone package `@aws-cdk/cx-api` and once as part of the construct library under `aws-cdk-lib/cx-api`. The code is copied during the release and the same versions of the packages will have the same code. However this makes it difficult for other packages to take a type dependency on types from this package. The most common class that's used from `cx-api` is `CloudAssembly` - the result of `app.synth()`. Previously a package had to take a dependency on the very large `aws-cdk-lib` just to use a single type. It would be better if other packages could instead depend on the smaller, much more focused `@aws-cdk/cx-api` package. ### Description of changes This adds the same mechanism to `CloudAssembly` to detect cross-library compatibility, that we already use for constructs like `Stack` or `App`. In TypeScript, it's now possible for a consuming package to receive an object from either package and check at runtime if it satisfies the requirements. We cannot get around type checking with this. Instead we introduce a new type `ICloudAssembly` into the Cloud Assembly Schema package (cdklabs/cloud-assembly-schema#133). This interface only declares a single property: `directory`. Consumers can use this type to indicate where they would like to receive a `CloudAssembly`. They can then use runtime code to either confirm a provided object already satisfies the requirements or fallback to creating a new `CloudAssembly` from the directory. Because the `CloudAssembly` in `cxapi` implements the new interface, this approach will work in all jsii languages. In TypeScript it's even compatible with older version of `aws-cdk-lib`. Jsii language will only support this going forward. #### Allowed breaking changes ``` weakened:aws-cdk-lib.cloud_assembly_schema.MetadataEntry weakened:aws-cdk-lib.cx_api.MetadataEntryResult ``` This PR updates the version of `@aws-cdk/cloud-assembly-schema` to make new of the new interface. However the update also includes a change to `MetadataEntry` which was introduced in cdklabs/cloud-assembly-schema#121. That change is weakening a type, because in #31041, the CDK started emitting booleans and numbers as metadata values. But since these types weren't officially declared in the schema, jsii runtime type checking failed to load them. The fix was to officially extend the type union to include `boolean` and `number` primitive values. This is considered breaking, because when used as an output any consuming code will now need to account for the possibility of the value being a `boolean` or `number`. In static languages, the type would already have been treated as a generic Object with required runtime checks. ### Describe any new or updated permissions being added n/a ### Description of how you validated changes Unit tests. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* (cherry picked from commit 94ba772)
moelasmar
pushed a commit
to aws/aws-cdk
that referenced
this pull request
Jan 24, 2025
…32998) ### Reason for this change We are publishing the `cx-api` package twice: Once as a standalone package `@aws-cdk/cx-api` and once as part of the construct library under `aws-cdk-lib/cx-api`. The code is copied during the release and the same versions of the packages will have the same code. However this makes it difficult for other packages to take a type dependency on types from this package. The most common class that's used from `cx-api` is `CloudAssembly` - the result of `app.synth()`. Previously a package had to take a dependency on the very large `aws-cdk-lib` just to use a single type. It would be better if other packages could instead depend on the smaller, much more focused `@aws-cdk/cx-api` package. ### Description of changes This adds the same mechanism to `CloudAssembly` to detect cross-library compatibility, that we already use for constructs like `Stack` or `App`. In TypeScript, it's now possible for a consuming package to receive an object from either package and check at runtime if it satisfies the requirements. We cannot get around type checking with this. Instead we introduce a new type `ICloudAssembly` into the Cloud Assembly Schema package (cdklabs/cloud-assembly-schema#133). This interface only declares a single property: `directory`. Consumers can use this type to indicate where they would like to receive a `CloudAssembly`. They can then use runtime code to either confirm a provided object already satisfies the requirements or fallback to creating a new `CloudAssembly` from the directory. Because the `CloudAssembly` in `cxapi` implements the new interface, this approach will work in all jsii languages. In TypeScript it's even compatible with older version of `aws-cdk-lib`. Jsii language will only support this going forward. #### Allowed breaking changes ``` weakened:aws-cdk-lib.cloud_assembly_schema.MetadataEntry weakened:aws-cdk-lib.cx_api.MetadataEntryResult ``` This PR updates the version of `@aws-cdk/cloud-assembly-schema` to make new of the new interface. However the update also includes a change to `MetadataEntry` which was introduced in cdklabs/cloud-assembly-schema#121. That change is weakening a type, because in #31041, the CDK started emitting booleans and numbers as metadata values. But since these types weren't officially declared in the schema, jsii runtime type checking failed to load them. The fix was to officially extend the type union to include `boolean` and `number` primitive values. This is considered breaking, because when used as an output any consuming code will now need to account for the possibility of the value being a `boolean` or `number`. In static languages, the type would already have been treated as a generic Object with required runtime checks. ### Describe any new or updated permissions being added n/a ### Description of how you validated changes Unit tests. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* (cherry picked from commit 94ba772)
moelasmar
pushed a commit
to aws/aws-cdk
that referenced
this pull request
Jan 24, 2025
…32998) ### Reason for this change We are publishing the `cx-api` package twice: Once as a standalone package `@aws-cdk/cx-api` and once as part of the construct library under `aws-cdk-lib/cx-api`. The code is copied during the release and the same versions of the packages will have the same code. However this makes it difficult for other packages to take a type dependency on types from this package. The most common class that's used from `cx-api` is `CloudAssembly` - the result of `app.synth()`. Previously a package had to take a dependency on the very large `aws-cdk-lib` just to use a single type. It would be better if other packages could instead depend on the smaller, much more focused `@aws-cdk/cx-api` package. ### Description of changes This adds the same mechanism to `CloudAssembly` to detect cross-library compatibility, that we already use for constructs like `Stack` or `App`. In TypeScript, it's now possible for a consuming package to receive an object from either package and check at runtime if it satisfies the requirements. We cannot get around type checking with this. Instead we introduce a new type `ICloudAssembly` into the Cloud Assembly Schema package (cdklabs/cloud-assembly-schema#133). This interface only declares a single property: `directory`. Consumers can use this type to indicate where they would like to receive a `CloudAssembly`. They can then use runtime code to either confirm a provided object already satisfies the requirements or fallback to creating a new `CloudAssembly` from the directory. Because the `CloudAssembly` in `cxapi` implements the new interface, this approach will work in all jsii languages. In TypeScript it's even compatible with older version of `aws-cdk-lib`. Jsii language will only support this going forward. #### Allowed breaking changes ``` weakened:aws-cdk-lib.cloud_assembly_schema.MetadataEntry weakened:aws-cdk-lib.cx_api.MetadataEntryResult ``` This PR updates the version of `@aws-cdk/cloud-assembly-schema` to make new of the new interface. However the update also includes a change to `MetadataEntry` which was introduced in cdklabs/cloud-assembly-schema#121. That change is weakening a type, because in #31041, the CDK started emitting booleans and numbers as metadata values. But since these types weren't officially declared in the schema, jsii runtime type checking failed to load them. The fix was to officially extend the type union to include `boolean` and `number` primitive values. This is considered breaking, because when used as an output any consuming code will now need to account for the possibility of the value being a `boolean` or `number`. In static languages, the type would already have been treated as a generic Object with required runtime checks. ### Describe any new or updated permissions being added n/a ### Description of how you validated changes Unit tests. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* (cherry picked from commit 94ba772)
moelasmar
pushed a commit
to aws/aws-cdk
that referenced
this pull request
Jan 24, 2025
…32998) ### Reason for this change We are publishing the `cx-api` package twice: Once as a standalone package `@aws-cdk/cx-api` and once as part of the construct library under `aws-cdk-lib/cx-api`. The code is copied during the release and the same versions of the packages will have the same code. However this makes it difficult for other packages to take a type dependency on types from this package. The most common class that's used from `cx-api` is `CloudAssembly` - the result of `app.synth()`. Previously a package had to take a dependency on the very large `aws-cdk-lib` just to use a single type. It would be better if other packages could instead depend on the smaller, much more focused `@aws-cdk/cx-api` package. ### Description of changes This adds the same mechanism to `CloudAssembly` to detect cross-library compatibility, that we already use for constructs like `Stack` or `App`. In TypeScript, it's now possible for a consuming package to receive an object from either package and check at runtime if it satisfies the requirements. We cannot get around type checking with this. Instead we introduce a new type `ICloudAssembly` into the Cloud Assembly Schema package (cdklabs/cloud-assembly-schema#133). This interface only declares a single property: `directory`. Consumers can use this type to indicate where they would like to receive a `CloudAssembly`. They can then use runtime code to either confirm a provided object already satisfies the requirements or fallback to creating a new `CloudAssembly` from the directory. Because the `CloudAssembly` in `cxapi` implements the new interface, this approach will work in all jsii languages. In TypeScript it's even compatible with older version of `aws-cdk-lib`. Jsii language will only support this going forward. #### Allowed breaking changes ``` weakened:aws-cdk-lib.cloud_assembly_schema.MetadataEntry weakened:aws-cdk-lib.cx_api.MetadataEntryResult ``` This PR updates the version of `@aws-cdk/cloud-assembly-schema` to make new of the new interface. However the update also includes a change to `MetadataEntry` which was introduced in cdklabs/cloud-assembly-schema#121. That change is weakening a type, because in #31041, the CDK started emitting booleans and numbers as metadata values. But since these types weren't officially declared in the schema, jsii runtime type checking failed to load them. The fix was to officially extend the type union to include `boolean` and `number` primitive values. This is considered breaking, because when used as an output any consuming code will now need to account for the possibility of the value being a `boolean` or `number`. In static languages, the type would already have been treated as a generic Object with required runtime checks. ### Describe any new or updated permissions being added n/a ### Description of how you validated changes Unit tests. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* (cherry picked from commit 94ba772)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
In aws/aws-cdk#31041, the CDK started emitting booleans and numbers as metadata values.
However, since these types are not officially declared in the schema, jsii runtime type checking prevents loading them:
Fixes aws/jsii#4742
FAQ
If the types are not allowed in the schema, how can a manifest with them be written?
Actually, these types are allowed in the schema, in the json schema that is:
cloud-assembly-schema/schema/cloud-assembly.schema.json
Lines 131 to 133 in f5ae537
This free form data is added to the json schema in order to support calls to
node.addMetadata(key: string, value: any), as defined in theconstructsAPI. However, this type was intentionally left out of the type declaration:cloud-assembly-schema/projenrc/update-schema.ts
Lines 79 to 89 in f5ae537
I'm fairly certain the comment is not applicable anymore, because we don't run jsii compatibility checks on the schema (we just bump it on every change). In practice, I think we can probably get rid of this patching and just type metadata entry values as
any- but that seems like a bigger change I don't want to go into right now.If such manifests cannot be loaded, how does synth work?
Because during synth, loading the assembly using the
Manifestclass happens solely inside the javascript space, it doesn't callback to java land. This problem presents itself only when directly using the javaManifestclass and invoking theloadAssemblyManifestfunction on a manifest produced byapp.synth().