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

Deprecation warnings not working for properties in a list of interfaces #3755

Closed
corymhall opened this issue Sep 19, 2022 · 1 comment · Fixed by #3756
Closed

Deprecation warnings not working for properties in a list of interfaces #3755

corymhall opened this issue Sep 19, 2022 · 1 comment · Fixed by #3756
Assignees
Labels
bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@corymhall
Copy link
Contributor

corymhall commented Sep 19, 2022

Describe the bug

When a property is deprecated, and that property is used in an interface that is part of a list, deprecation warnings (or failures) are not generated.

For example, cdk.aws_ec2.SubnetType.ISOLATED is deprecated, but if you use it as part of SubnetConfiguration like this:

new ec2.Vpc(this, 'Vpc', {
  subnetConfiguration: [
    {
      name: 'Isolated',
      subnetType: ec2.SubnetType.ISOLATED,
    },
  ],
});

warnings are not generated.

Expected Behavior

A warning should be generated:

[WARNING] @aws-cdk/aws-ec2.SubnetType#ISOLATED is deprecated.
  use `SubnetType.PRIVATE_ISOLATED`
  This API will be removed in the next major release. 

Current Behavior

No warning has been generated.

Reproduction Steps

Create a CDK v1 application.

npx aws-cdk@latest-1 init --language=typescript app

Create a VPC using a deprecated SubnetType:

import * as cdk from '@aws-cdk/core';
import * as ec2 from '@aws-cdk/aws-ec2';

export class CdkTmpV1Stack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    new ec2.NatInstanceProvider({
      instanceType: ec2.InstanceType.of(ec2.InstanceClass.A1, ec2.InstanceSize.NANO),
      allowAllTraffic: true,
    });
    new ec2.Vpc(this, 'Vpc', {
      subnetConfiguration: [
        {
          name: 'public',
          subnetType: ec2.SubnetType.PUBLIC,
        },
        {
          name: 'Isolated',
          subnetType: ec2.SubnetType.ISOLATED,
        }
      ]
    })
  }
}

Run npx cdk synth

Possible Solution

It looks like we are not correctly handling cases where we deprecate properties that exist in a list of intefaces.

The function in .warnings.jsii.js

function _aws_cdk_aws_ec2_SubnetConfiguration(p) {
    if (p == null)
        return;
    visitedObjects.add(p);
    try {
        if (!visitedObjects.has(p.subnetType))
            _aws_cdk_aws_ec2_SubnetType(p.subnetType);
    }
    finally {
        visitedObjects.delete(p);
    }
}

Should be something like

function _aws_cdk_aws_ec2_SubnetConfiguration(p) {
    if (p == null)
        return;
    visitedObjects.add(p);
    try {
        p.forEach(config => {
          if (!visitedObjects.has(config.subnetType))
              _aws_cdk_aws_ec2_SubnetType(config.subnetType);
        });
    }
    finally {
        visitedObjects.delete(p);
    }

Additional Information/Context

Original issue aws/aws-cdk#22066

SDK version used

CDK v1.172.0

Environment details (OS name and version, etc.)

Ubuntu

@corymhall corymhall added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 19, 2022
@RomainMuller RomainMuller self-assigned this Sep 19, 2022
@RomainMuller RomainMuller added effort/small Small work item – less than a day of effort p1 and removed needs-triage This issue or PR still needs to be triaged. labels Sep 19, 2022
RomainMuller added a commit that referenced this issue Sep 19, 2022
When a deprecated member was nested under a collection of values, no
deprecation warning was being generated, despite all the necessary
information being available.

There was a missing case that caused the collection to be validated as
if it was the element type, which is obviously incorrect, but thankfully
did not cause errors as it typically led to verifying `undefined`, which
silently succeeds.

Added the necessary code so that collections are deep-validated as they
should have been.

Fixes #3755
@mergify mergify bot closed this as completed in #3756 Sep 23, 2022
mergify bot pushed a commit that referenced this issue Sep 23, 2022
When a deprecated member was nested under a collection of values, no deprecation warning was being generated, despite all the necessary information being available.

There was a missing case that caused the collection to be validated as if it was the element type, which is obviously incorrect, but thankfully did not cause errors as it typically led to verifying `undefined`, which silently succeeds.

Added the necessary code so that collections are deep-validated as they should have been.

Fixes #3755



---

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

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
@github-actions
Copy link
Contributor

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants