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

(assertions): absentProperty() and not() matchers are incompatible with hasResourceProperties #16626

Closed
nija-at opened this issue Sep 23, 2021 · 6 comments · Fixed by #16678
Closed
Labels
@aws-cdk/assertions Related to the @aws-cdk/assertv2 package bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@nija-at
Copy link
Contributor

nija-at commented Sep 23, 2021

Given a template that has a resource with no Properties (just Type)

{
  "Resources": {
    "IamUser1": {
      "Type": "AWS::IAM::User"
    }
  }
}

The following assertion should pass but throws the error Template has 1 resources with type AWS::IAM::User, but none match as expected.

Template.fromStack(stack).hasResourceProperties('AWS::IAM::User', {
  UserName: Match.absentProperty(),
});

Similarly, the assertion -

Template.fromStack(stack).hasResourceProperties('AWS::IAM::User', Match.not({
  UserName: 'MyUser',
}));

The workaround is to do -

Templte.fromStack(stack).hasResource('AWS::IAM::User', {
  Properties: Match.absentProperty(),
});

This is 🐛 Bug Report

@nija-at nija-at added bug This issue is a bug. p1 needs-triage This issue or PR still needs to be triaged. @aws-cdk/assertions Related to the @aws-cdk/assertv2 package effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Sep 23, 2021
@kaizencc
Copy link
Contributor

Related: #16653. If that PR is accepted, the workaround for this issue would become:

Template.fromStack(stack).hasResourceProperties('AWS::IAM::User', Match.absentProperty());

With this in mind, is it reasonable to say that hasResourceProperties() assumes that Properties exists on the template? Personally, I see value in fixing this bug as described, but I don't think its life or death.

@nija-at
Copy link
Contributor Author

nija-at commented Sep 27, 2021

#16653. If that PR is accepted, the workaround for this issue would become:

This is not the correct solution for this issue. As stated in the docs, Match.absentProperty() has a specific intent.

Template.fromStack(stack).hasResourceProperties('AWS::IAM::User', {
  UserName: Match.absentProperty(),
});

must automatically pass for both -

"IamUser1": {
  "Type": "AWS::IAM::User"
  "Properties": {}
}

and

"IamUser1": {
  "Type": "AWS::IAM::User"
}

This is because the API is called hasResourceProperties().

@nija-at
Copy link
Contributor Author

nija-at commented Sep 27, 2021

There are a couple of ways to approach this issue -

  1. special case hasResourceProperties() such that the template is modified where all Resource blocks for the format "MyLogicalId": { "Type": "AWS::Foo::Bar" } gets converted into "MyLogicalId": { "Type": "AWS::Foo::Bar", "Properties": {} }

  2. have a special (private) matcher than handles this case and use that in hasResourceProperties().

@kaizencc
Copy link
Contributor

Reading your comments, I agree that we should do something about this bug :). And I'll look further into the two approaches you've outlined in the second comment. However -

#16653 is not trying to fix this bug. Rather, I think that Match.absentProperty() should be of type Matcher as I imagine that's what the customer expects. My arguments for this change below -

  • Would make Matcher.isMatcher(Match.absentProperty()) === true. I believe this to be the sensible result (currently it returns false).
  • For: template.hasResourceProperties('Foo::Bar', Match.absentProperty());. I understand that this is confusing so I will do my best. I am arguing that this assertion should return true. It currently does not. The docs say that for Match.absentProperty(), we should "Use this matcher in the place of a field's value, if the field must not be present." I see the second argument in hasResourceProperties() to be the value of the Properties field and thus is a valid place to put Match.absentProperty() (and expect the library to assert that Properties is absent).
  • I think it generally Improves code readability by eliminating the special case ABSENT, but that's not as motivating as the above 2 points.

@nija-at
Copy link
Contributor Author

nija-at commented Sep 27, 2021

Ah, my bad! I thought they were related.

@mergify mergify bot closed this as completed in #16678 Oct 6, 2021
mergify bot pushed a commit that referenced this issue Oct 6, 2021
…not` and `Match.absent` (#16678)

Fixes #16626.

This PR modifies `hasResourceProperties()` so that it accounts for the special case where `Properties` does not exist on the template. The following assertions previously behaved incorrectly when `Properties` were not in the template and are now fixed:

```ts
template.fromStack(stack); // some template with no `Properties`.

// assert that `Properties` does not exist in the template. Returns true.
template.hasResourceProperties('Foo::Bar', Match.absent());

// assert that `baz` is not a `Property` of 'Foo::Bar'. Returns true.
template.hasResourceProperties('Foo::Bar', {
   baz: Match.absent(),
};

// assert that `baz` does not have a value of `qux` in the `Properties` object. Returns true.
template.hasResourceProperties('Foo::Bar', Match.not({baz: 'qux'});
```

It also moves `AbsentMatch` into the `private` folder so that it can be exposed internally as a special case for `hasResourceProperties()`.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Oct 6, 2021

⚠️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.

njlynch pushed a commit that referenced this issue Oct 11, 2021
…not` and `Match.absent` (#16678)

Fixes #16626.

This PR modifies `hasResourceProperties()` so that it accounts for the special case where `Properties` does not exist on the template. The following assertions previously behaved incorrectly when `Properties` were not in the template and are now fixed:

```ts
template.fromStack(stack); // some template with no `Properties`.

// assert that `Properties` does not exist in the template. Returns true.
template.hasResourceProperties('Foo::Bar', Match.absent());

// assert that `baz` is not a `Property` of 'Foo::Bar'. Returns true.
template.hasResourceProperties('Foo::Bar', {
   baz: Match.absent(),
};

// assert that `baz` does not have a value of `qux` in the `Properties` object. Returns true.
template.hasResourceProperties('Foo::Bar', Match.not({baz: 'qux'});
```

It also moves `AbsentMatch` into the `private` folder so that it can be exposed internally as a special case for `hasResourceProperties()`.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
…not` and `Match.absent` (aws#16678)

Fixes aws#16626.

This PR modifies `hasResourceProperties()` so that it accounts for the special case where `Properties` does not exist on the template. The following assertions previously behaved incorrectly when `Properties` were not in the template and are now fixed:

```ts
template.fromStack(stack); // some template with no `Properties`.

// assert that `Properties` does not exist in the template. Returns true.
template.hasResourceProperties('Foo::Bar', Match.absent());

// assert that `baz` is not a `Property` of 'Foo::Bar'. Returns true.
template.hasResourceProperties('Foo::Bar', {
   baz: Match.absent(),
};

// assert that `baz` does not have a value of `qux` in the `Properties` object. Returns true.
template.hasResourceProperties('Foo::Bar', Match.not({baz: 'qux'});
```

It also moves `AbsentMatch` into the `private` folder so that it can be exposed internally as a special case for `hasResourceProperties()`.

----

*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
@aws-cdk/assertions Related to the @aws-cdk/assertv2 package 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