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

applyAspect visits same node twice in Version 1.58 #9706

Closed
lanefelker opened this issue Aug 14, 2020 · 4 comments
Closed

applyAspect visits same node twice in Version 1.58 #9706

lanefelker opened this issue Aug 14, 2020 · 4 comments
Assignees
Labels
@aws-cdk/core Related to core CDK functionality closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.

Comments

@lanefelker
Copy link

lanefelker commented Aug 14, 2020

When upgrading to version 1.58 we started to get an error where apply aspect visited the same node twice causing the second operation to fail

Reproduction Steps

An apply aspect on an api as follows:

const optionsIntegration = lambdaIntegration(api, lambda)
api.node.applyAspect({
    visit: (node: cdk.IConstruct): void => {
      // Add OPTIONS to all resources
      if (node instanceof apiGateway.Resource) {
        node.addMethod('OPTIONS', optionsIntegration)
      }
    }
})

and an apigateway with one resource. For example:

const resource = api.root.addResource('resource')
resource.addMethod('GET', new apigateway.MockIntegration())

What did you expect to happen?

apply aspect only visits each node once

What actually happened?

An error occurred "Error: There is already a Construct with name 'OPTIONS' in Resource [resource]"

Environment

  • **CLI Version: ** 1.58.0
  • Framework Version: 1.58.0
  • Node.js Version: v11.10.1
  • OS: Mac OS X Catalina
  • Language (Version): 3.9.2

Other


This is 🐛 Bug Report

@lanefelker lanefelker added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 14, 2020
@SomayaB SomayaB added the @aws-cdk/core Related to core CDK functionality label Aug 14, 2020
@Chriscbr
Copy link
Contributor

I wasn't able to reproduce this issue... here is the code I tried on v1.58 which synthesized and deployed without warnings:

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

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

    const func = new lambda.Function(this, 'MyFunction', {
      runtime: lambda.Runtime.NODEJS_12_X,
      code: new lambda.InlineCode('exports.handler = function() { console.log("Hello!") }'),
      handler: 'index.handler',
    });

    const api = new apigateway.RestApi(this, 'MyApi');
    const resource = api.root.addResource('resource');
    resource.addMethod('GET', new apigateway.MockIntegration());

    const optionsIntegration = new apigateway.LambdaIntegration(func);
    api.node.applyAspect({
      visit: (node: cdk.IConstruct): void => {
        if (node instanceof apigateway.Resource) {
          node.addMethod('OPTIONS', optionsIntegration)
        }
      }
    });
  }
}

Maybe the behavior has something to do with interaction with other constructs/aspects in your stack? There was a change to aspects introduced in #9558, so it's possible there's a regression, but it's hard for me to pinpoint.

@eladb
Copy link
Contributor

eladb commented Aug 17, 2020

@NetaNir can you take a look?

@eladb eladb assigned NetaNir and unassigned eladb Aug 17, 2020
@eladb eladb added p1 effort/small Small work item – less than a day of effort labels Aug 17, 2020
@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Aug 18, 2020
@NetaNir
Copy link
Contributor

NetaNir commented Aug 20, 2020

Which version did you upgrade from? It's hard to tell what could be the problem without seeing how is api created.
Can you try and log the node path from the visit method:

api.node.applyAspect({
    visit: (node: cdk.IConstruct): void => {
      // Add OPTIONS to all resources
      if (node instanceof apiGateway.Resource) {
       console.log(node.node.path);
        node.addMethod('OPTIONS', optionsIntegration)
      }
    }
})

Might give some clue as to what is hapnning

@NetaNir NetaNir added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Aug 20, 2020
@github-actions
Copy link

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Aug 27, 2020
@github-actions github-actions bot closed this as completed Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/core Related to core CDK functionality closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.
Projects
None yet
Development

No branches or pull requests

5 participants