Skip to content

Commit

Permalink
fix(core): isTaggable function can return undefined instead of false (
Browse files Browse the repository at this point in the history
#31600)

Closes #26495 

### Reason for this change

The `isTaggable` function of the `TagManager` class is currently broken in Python, as it can return `undefined` instead of `true` or `false`.

### Description of changes

In JS/TS, the logical AND operator (`&&`) returns the first falsy value it encounters, even if that value is `undefined` instead of `false` - so the current implementation of `isTaggable` allows for `undefined` to be returned if `tags` is undefined:

```ts
public static isTaggable(construct: any): construct is ITaggable {
  const tags = (construct as any).tags;
  return tags && typeof tags === 'object' && (tags as any)[TAG_MANAGER_SYM];
}
```

The fix is simply changing the return line to the following to handle cases where tags is `null` or `undefined`:

```ts
return tags !== undefined && tags !== null && typeof tags === 'object' && (tags as any)[TAG_MANAGER_SYM];
```

### Description of how you validated changes

Added a unit test to assert that `isTaggable` returns `false`, not `undefined` for a non-taggable Construct (and still returns `true` for a taggable construct).

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
sumupitchayan authored Oct 2, 2024
1 parent 0755561 commit be70c82
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 1 deletion.
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/core/lib/tag-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ export class TagManager {
*/
public static isTaggable(construct: any): construct is ITaggable {
const tags = (construct as any).tags;
return tags && typeof tags === 'object' && (tags as any)[TAG_MANAGER_SYM];
return tags !== undefined && tags !== null && typeof tags === 'object' && (tags as any)[TAG_MANAGER_SYM];
}

/**
Expand Down
37 changes: 37 additions & 0 deletions packages/aws-cdk-lib/core/test/tag-manager.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { Construct } from 'constructs';
import * as cdk from '../../../aws-cdk-lib';
import { TagType } from '../lib/cfn-resource';
import { TagManager } from '../lib/tag-manager';

Expand Down Expand Up @@ -225,4 +227,39 @@ describe('tag manager', () => {
expect(true).toEqual(mgr.applyTagAspectHere(['AWS::Fake::Resource'], []));
expect(false).toEqual(mgr.applyTagAspectHere(['AWS::Wrong::Resource'], []));
});

test('isTaggable function works as expected', () => {
const app = new cdk.App();
const taggableConstruct = new TaggableConstruct(app, 'MyConstruct');
const nonTaggableConstruct = new NonTaggableConstruct(app, 'NonTaggableConstruct');

// Assert that isTaggable returns true for a taggable construct
expect(TagManager.isTaggable(taggableConstruct)).toEqual(true);

// Assert that isTaggable returns false (not undefined) for a non-taggable construct
expect(TagManager.isTaggable(nonTaggableConstruct)).not.toEqual(undefined);
expect(TagManager.isTaggable(nonTaggableConstruct)).toEqual(false);
});
});

// `Stack` extends ITaggable so this construct is Taggable by default
class TaggableConstruct extends cdk.Stack {
constructor(scope: Construct, id: string) {
super(scope, id);
new cdk.CfnResource(this, 'Resource', {
type: 'Whatever::The::Type',
properties: {
Tags: this.tags.renderedTags,
},
});
}
}

// Simple Construct that does not extend ITaggable
class NonTaggableConstruct extends Construct {
public readonly id: string;
constructor(scope: Construct, id: string) {
super(scope, id);
this.id = id;
}
}

0 comments on commit be70c82

Please sign in to comment.