Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(cfnspec): should be able to handle types changing from Json to type #21767

Open
6 tasks
corymhall opened this issue Aug 25, 2022 · 4 comments
Open
6 tasks

(cfnspec): should be able to handle types changing from Json to type #21767

corymhall opened this issue Aug 25, 2022 · 4 comments
Labels
@aws-cdk/cfnspec effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p2

Comments

@corymhall
Copy link
Contributor

corymhall commented Aug 25, 2022

Overview

Sometimes a property type in the cfnspec is set to Json which means that it accepts any JSON value. If a service team ever adds actual types for this property it is not a breaking change on their side, but it does lead to breaking changes for us. Our current workaround is to add a patch which keeps the value as Json which is not ideal.

Issue list

Type Change
List of Properties that have changed from Json to a type

@corymhall corymhall added the management/tracking Issues that track a subject or multiple issues label Aug 25, 2022
@corymhall corymhall changed the title Tracking: cfnspec (cfnspec): should be able to handle types changing from Json to type Aug 25, 2022
@corymhall corymhall added @aws-cdk/cfnspec p2 feature-request A feature should be added or improved. effort/large Large work item – several weeks of effort and removed management/tracking Issues that track a subject or multiple issues labels Aug 25, 2022
@madeline-k
Copy link
Contributor

madeline-k commented Dec 26, 2022

A solution we are working on (thanks to @rix0rrr for proposing this):

  • As a postprocessing step after importing a new spec, any time we encounter a property { MyProp: { PrimitiveType: Json } }, we automatically write a patch file that adds a marker field into the property, like { MyProp: { WasOnceJson: true } } .
  • In cfn2ts, for every property MyProp we encounter we look at WasOnceJson.
    • If it doesn't exist OR MyProp.PrimitiveType = 'Json', we proceed as normal.
    • If it does exist, then we emit two properties:
/**
 * MyProp (as untyped json data). The object you provide for this property will end up in your cloudformation template exactly as specified, without any capitalization alteration.
 *
 * @deprecated Use `myPropTyped` instead.
 * @default - Specify {at most|exactly} one of `myProp` and `myPropTyped`
 */
readonly myProp?: any;

/**
 * MyProp
 *
 * @default - Specify {at most|exactly} one of `myProp` and `myPropTyped`
 */
readonly myPropTyped?: MyPropType;

"At most" vs "exactly" depends on whether the original property was optional or required.

Once we have this in place, we can add the WasOnceJson field as necessary, and get rid of this patch: 4fbc182#diff-08b4822e59cb5286d16984c086cf2fe15085a336fa7a37157d923b1f408f5647

@Wurstnase
Copy link
Contributor

Wurstnase commented Jan 7, 2023

Looks like, that the new type will generate an additional dataclass. Before the patch it is PascalCase. After this patch it becomes camelCase. Would it a big problem to make the properties PascalCase again so it would be possible to use the generated dataclasses?

In that case we could benefit with typing and autocompletion.

@peterwoodworth
Copy link
Contributor

peterwoodworth commented Feb 16, 2023

We've received a number of reports since we patched the breaking changes found in spec 101, including

#23570
#23679
#23709
#24754
#25558
#25774
#25990

@Nabeelperson
Copy link

This issue also effects: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-backup-backupselection-conditionparameter.html

I use Java CDK:

            CfnBackupSelection.Builder.create(this, id + "TagSelector")
                .backupPlanId(backupPlan.getAttrBackupPlanId())
                .backupSelection(
                    BackupSelectionResourceTypeProperty.builder()
                        .selectionName("Selector")
                        .iamRoleArn(role.getRoleArn())
                        .resources(List.of("*"))
                        .conditions(
                            Map.of(
                                "StringEquals",
                                List.of(
                                    ConditionParameterProperty.builder()
                                        .conditionKey("aws:ResourceTag/TagKey")
                                        .conditionValue("TagValue")
                                        .build()
                                )
                            )
                        )
                        .build()
                )
                .build();

Results in camelCase yaml output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/cfnspec effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

No branches or pull requests

5 participants