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

fix(core): property overrides sometimes don't work with intrinsics #20608

Merged
merged 2 commits into from
Jun 3, 2022
Merged

fix(core): property overrides sometimes don't work with intrinsics #20608

merged 2 commits into from
Jun 3, 2022

Conversation

corymhall
Copy link
Contributor

When you add a property override, we merge it with any properties set
when the construct is initialized. The merge happens after tokens are
resolved. For example, if we created a resource:

const resource = new CfnResource(this, 'Resource', {
  type: 'MyResourceType',
  properties: {
    prop1: Fn.ref('Param'),
  },
});

And the added an override for prop1:

resource.addPropertyOverride('prop1', Fn.ref('OtherParam'));

We would then perform a deep merge on the properties with a priority on
the overrides. The above example would work fine, eventually the merge
would get to the point where it was comparing two objects:

target = { prop1: { Ref: 'Param' } }
source = { prop1: { Ref: 'OtherParam' } }
result = { prop1: { Ref: 'OtherParam' } }

If we instead added an override using a different intrinsic:

resource.addPropertyOverride('prop1', Fn.join('-', ['hello',
Fn.ref('Param')]));

Then we would get to the point in the merge where we were comparing two
different objects:

target = { prop1: { Ref: 'Param' } }
source = { prop1: { 'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] } }
result = { prop1: {
  Ref: 'Param',
  'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] },
}}

This PR solves this issue by filtering out any CloudFormation
intrinsics. If the merge finds an intrinsic key in the target it "drops" the
intrinsic and takes whatever value is in the source (override).

fix #19971, #19447


All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

When you add a property override, we merge it with any properties set
when the construct is initialized. The merge happens _after_ tokens are
resolved. For example, if we created a resource:

```ts
const resource = new CfnResource(this, 'Resource', {
  type: 'MyResourceType',
  properties: {
    prop1: Fn.ref('Param'),
  },
});
```

And the added an override for `prop1`:

```ts
resource.addPropertyOverride('prop1', Fn.ref('OtherParam'));
```

We would then perform a deep merge on the properties with a priority on
the overrides. The above example would work fine, eventually the merge
would get to the point where it was comparing two objects:

```
target = { prop1: { Ref: 'Param' } }
source = { prop1: { Ref: 'OtherParam' } }
result = { prop1: { Ref: 'OtherParam' } }
```

If we instead added an override using a different intrinsic:

```ts
resource.addPropertyOverride('prop1', Fn.join('-', ['hello',
Fn.ref('Param')]));
```

Then we would get to the point in the merge where we were comparing two
different objects:

```
target = { prop1: { Ref: 'Param' } }
source = { prop1: { 'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] } }
result = { prop1: {
  Ref: 'Param',
  'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] },
}}
```

This PR solves this issue by filtering out any CloudFormation
intrinsics. If the merge finds an intrinsic key in the target it "drops" the
intrinsic and takes whatever value is in the source (override).

fix #19971, #19447
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jun 3, 2022
@gitpod-io
Copy link

gitpod-io bot commented Jun 3, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team June 3, 2022 12:50
@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Jun 3, 2022
* Eventually we will get to the point where we have
*
* target: { prop1: { Ref: 'Param' } }
* sources: [ { prop1: { 'Fn::Join': ['-', 'hello', 'world'] } } ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Are the { and [ reversed from L#347 to L#553?

@mergify
Copy link
Contributor

mergify bot commented Jun 3, 2022

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: d81d48e
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 49b397c into aws:main Jun 3, 2022
@mergify
Copy link
Contributor

mergify bot commented Jun 3, 2022

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

daschaa pushed a commit to daschaa/aws-cdk that referenced this pull request Jul 9, 2022
…ws#20608)

When you add a property override, we merge it with any properties set
when the construct is initialized. The merge happens _after_ tokens are
resolved. For example, if we created a resource:

```ts
const resource = new CfnResource(this, 'Resource', {
  type: 'MyResourceType',
  properties: {
    prop1: Fn.ref('Param'),
  },
});
```

And the added an override for `prop1`:

```ts
resource.addPropertyOverride('prop1', Fn.ref('OtherParam'));
```

We would then perform a deep merge on the properties with a priority on
the overrides. The above example would work fine, eventually the merge
would get to the point where it was comparing two objects:

```
target = { prop1: { Ref: 'Param' } }
source = { prop1: { Ref: 'OtherParam' } }
result = { prop1: { Ref: 'OtherParam' } }
```

If we instead added an override using a different intrinsic:

```ts
resource.addPropertyOverride('prop1', Fn.join('-', ['hello',
Fn.ref('Param')]));
```

Then we would get to the point in the merge where we were comparing two
different objects:

```
target = { prop1: { Ref: 'Param' } }
source = { prop1: { 'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] } }
result = { prop1: {
  Ref: 'Param',
  'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] },
}}
```

This PR solves this issue by filtering out any CloudFormation
intrinsics. If the merge finds an intrinsic key in the target it "drops" the
intrinsic and takes whatever value is in the source (override).

fix aws#19971, aws#19447


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mergify bot pushed a commit that referenced this pull request Sep 30, 2022
There was a previous fix in #20608 that attempted to fix addPropertyOverride when intrinisics were involved. This PR fixes an edge case where overrides do not work correctly an object is being replaced with an intrinsic.

For example, there might be the case where the override is an intrinsic and it is overriding an object, not a value.

```
   original: {
     Type: 'MyResourceType',
     Properties: {
       prop1: { subprop: { name: { 'Fn::GetAtt': 'abc' } } }
     }
   }
   override: {
     Properties: {
       prop1: { subprop: { 'Fn::If': ['SomeCondition', {...}, {...}] }}
     }
   }
```

The previous fix only handled cases where the original had an intrinsic, but in the above example the override is the first to hit an intrinsic. This PR adds logic to handle cases where we hit an intrinsic in the original _or_ the override.

fixes #19971


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
arewa pushed a commit to arewa/aws-cdk that referenced this pull request Oct 8, 2022
…2294)

There was a previous fix in aws#20608 that attempted to fix addPropertyOverride when intrinisics were involved. This PR fixes an edge case where overrides do not work correctly an object is being replaced with an intrinsic.

For example, there might be the case where the override is an intrinsic and it is overriding an object, not a value.

```
   original: {
     Type: 'MyResourceType',
     Properties: {
       prop1: { subprop: { name: { 'Fn::GetAtt': 'abc' } } }
     }
   }
   override: {
     Properties: {
       prop1: { subprop: { 'Fn::If': ['SomeCondition', {...}, {...}] }}
     }
   }
```

The previous fix only handled cases where the original had an intrinsic, but in the above example the override is the first to hit an intrinsic. This PR adds logic to handle cases where we hit an intrinsic in the original _or_ the override.

fixes aws#19971


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
homakk pushed a commit to homakk/aws-cdk that referenced this pull request Dec 1, 2022
…2294)

There was a previous fix in aws#20608 that attempted to fix addPropertyOverride when intrinisics were involved. This PR fixes an edge case where overrides do not work correctly an object is being replaced with an intrinsic.

For example, there might be the case where the override is an intrinsic and it is overriding an object, not a value.

```
   original: {
     Type: 'MyResourceType',
     Properties: {
       prop1: { subprop: { name: { 'Fn::GetAtt': 'abc' } } }
     }
   }
   override: {
     Properties: {
       prop1: { subprop: { 'Fn::If': ['SomeCondition', {...}, {...}] }}
     }
   }
```

The previous fix only handled cases where the original had an intrinsic, but in the above example the override is the first to hit an intrinsic. This PR adds logic to handle cases where we hit an intrinsic in the original _or_ the override.

fixes aws#19971


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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. contribution/core This is a PR that came from AWS. effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Core]: addPropertyOverride not producing correct result after upgrade
3 participants