fix(apigatewayv2): HttpApi and Route in different stacks creates cycles

[hoegertn]

When the Route construct with an integration is defined
in a separate stack from the one defining the HttpApi
construct, a circular dependency error is thrown.

The bug was introduced in - 855ce59 - where the
refactor of moving the creation of Integration resulted in
using the HttpApi as the scope of the Integration
construct, rather than the Route.

fixes #13021

---

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

[gitpod-io]

[mergify]

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

[hoegertn]

@eladb @nija-at I modified the code to keep the old semantic of using the route scope for the integration.

But I see the benefit of the Integration being tied to the API scope.

I think ideally the integration should be scoped depending on the integration type and for Lambda integrations it should be in the same scope as the function, as this causes the cyclic dependency.

[nija-at]

But I see the benefit of the Integration being tied to the API scope.

I'm curious what the benefit is?

I think ideally the integration should be scoped depending on the integration type and for Lambda integrations it should be in the same scope as the function, as this causes the cyclic dependency.

Do you mean when the lambda is in the API scope or just any other stack?

It feels reasonable to assume that the lambda function will be defined closer to the integration rather than the API.

[hoegertn]

But I see the benefit of the Integration being tied to the API scope.

I'm curious what the benefit is?

The set of integrations for deduplication is located at the API and integrations are possibly reused, so they should live at the top level.

I think ideally the integration should be scoped depending on the integration type and for Lambda integrations it should be in the same scope as the function, as this causes the cyclic dependency.

Do you mean when the lambda is in the API scope or just any other stack?

It feels reasonable to assume that the lambda function will be defined closer to the integration rather than the API.

The integration should live in the same stack as the Lambda function at any time. Just not sure about other integrations that are not Lambda.

But I am totally happy to have integrations located at the route as in my PR, I am just not sure as I never reused integrations and want to see if this breaks other peoples workflows.

[nija-at]

The integration should live in the same stack as the Lambda function at any time.

It could also live in another stack, no?

How about taking scope as an optional parameter as part of integration's props and defaulting to the Route's scope if not provided?

[hoegertn]

The integration should live in the same stack as the Lambda function at any time.

It could also live in another stack, no?

It references the Lambda function so it could only live in the same stack or downstream but not more to the top.

Api -> Lambda -> Integ

[nija-at]

The integration should live in the same stack as the Lambda function at any time.

It could also live in another stack, no?

It references the Lambda function so it could only live in the same stack or downstream but not more to the top.

Api -> Lambda -> Integ

Agreed.

[ayush987goyal]

@hoegertn Thanks for this change! I did not realise this flow while making the change.

I am just curious on why have you decided to make the scope as optional and defaulted to this in the _addIntegration helper? I think we should always make it scoped in the Route?

[nija-at]

@hoegertn Thanks for this change! I did not realise this flow while making the change.

I am just curious on why have you decided to make the scope as optional and defaulted to this in the _addIntegration helper? I think we should always make it scoped in the Route?

Good point. And we should make scope the first argument on the method, just like in any Construct.

[hoegertn]

@hoegertn Thanks for this change! I did not realise this flow while making the change.

I am just curious on why have you decided to make the scope as optional and defaulted to this in the _addIntegration helper? I think we should always make it scoped in the Route?

The _addIntegration could be called from another place outside of the scope of a route. So I thought a default of the API as the parent might be a good idea.

[ayush987goyal]

The _addIntegration could be called from another place outside of the scope of a route. So I thought a default of the API as the parent might be a good idea.

I am just thinking if this problem would then occur in that context? Better we force the callers to pass the scope. Thoughts?

[nija-at]

The _addIntegration could be called from another place outside of the scope of a route. So I thought a default of the API as the parent might be a good idea.

I am just thinking if this problem would then occur in that context? Better we force the callers to pass the scope. Thoughts?

Let's make scope mandatory and make it the first parameter of the method (to keep it consistent with our existing patterns). Better to be explicit than implicit. This is an internal API, so we can change it later when we learn more.

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: 4028e0c
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).