Add filterSensitiveLog for Union types #190

trivikr · 2020-07-01T05:45:39Z

Issue #, if available:
Follow up #170

Description of changes:
This PR adds filterSensitiveLog for Union types

Testing done prior to code change on TypeScript playground

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

trivikr · 2020-07-06T21:02:05Z

Converted to draft as it's blocked on #191

...-codegen/src/main/java/software/amazon/smithy/typescript/codegen/StructuredMemberWriter.java

kstich · 2020-08-21T00:42:40Z

...pescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/UnionGenerator.java

+                        writer.write("};");
+                    }
+                }
+                writer.write("if (${1L}.$$unknown !== undefined) return {[${1L}.$$unknown[0]]: ${1L}.$$unknown[1]};",


You may want to not log the value of an unknown union member, as there's no way to know if it is sensitive or not.

Logged the value of an unknown union member as 'UNKNOWN' in 0382f0e

...pescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/UnionGenerator.java

kstich · 2020-08-21T01:03:13Z

...pescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/UnionGenerator.java

+                    Shape memberTarget = model.expectShape(member.getTarget());
+                    String memberName = symbolProvider.toMemberName(member);
+
+                    if (member.getMemberTrait(model, SensitiveTrait.class).isPresent()) {


A lot of this code, specifically the branching, looks similar to that of writing this function for structures (StructuredMemberWriter.writeFilterSensitiveLog.) Can it be consolidated cleanly at all?

Can it be consolidated cleanly at all?

I'm open to suggestions for modifications.

The branching can't be consolidated between Union and Structure, as there's a check for SimpleShape in case of CollectionShape/MapShape members in case of Union which doesn't exist in latter.

Consolidated by creating new method writeMemberFilterSensitiveLog in e1fd23c, and calling it from Union in e5361be 🎉

It was done by moving SimpleShape check inside write<Collection|Map>FilterSensitiveLog
It's redundant for Structure as it'll never reach because of isMemberOverwriteRequired call, but is required for Union.

kstich · 2020-08-21T01:08:41Z

...es/software/amazon/smithy/typescript/codegen/test-list-with-union-with-sensitive-data.smithy

+
+    @sensitive
+    sensitiveBar: String
+}


Can you please add newlines to the end of these files? There are quite a few without them.

If you use IntelliJ, you can set this automatically under Preferences -> Editor -> General -> Ensure an empty line at the end of a file on Save

Added in commit 481a425

kstich · 2020-08-25T00:18:33Z

...-codegen/src/main/java/software/amazon/smithy/typescript/codegen/StructuredMemberWriter.java

+                String itemParam = "item";
+                Shape collectionMemberTarget = model.expectShape(collectionMember.getTarget());
+                writer.write("$L => ", itemParam);
+                if (collectionMemberTarget.isStructureShape() || collectionMemberTarget.isUnionShape()) {


This looks really similar to the code in writeMemberFilterSensitiveLog, is it possible to refactor this chunk of if/else to instead use that? I might be missing something, but the final else is the only thing that looks different to me here.

The purpose of the else block is to throw CodegenException in case redundant code is being written for provided models at Runtime for SimpleShape.

But let me verify that in a Unit test instead, and reuse code from writeMemberFilterSensitiveLog

But let me verify that in a Unit test instead, and reuse code from writeMemberFilterSensitiveLog

Code reused in commit 4d94416
Unit test test-insensitive-list already checks for redundant code not being present.

kstich · 2020-08-25T00:19:25Z

...-codegen/src/main/java/software/amazon/smithy/typescript/codegen/StructuredMemberWriter.java

+                    writer.write("...$L,", accParam);
+                    Shape mapMemberTarget = model.expectShape(mapMember.getTarget());
+                    writer.openBlock("[$L]: ", ",", keyParam, () -> {
+                        if (mapMemberTarget.isStructureShape() || mapMemberTarget.isUnionShape()) {


This looks really similar to the code in writeMemberFilterSensitiveLog, is it possible to refactor this chunk of if/else to instead use that? I might be missing something, but the final else is the only thing that looks different to me here.

Done in commit 4d94416

…e member

Can be fixed by adding es2017 in lib https://stackoverflow.com/questions/45422573 But waiting for root TSConfig to be pushed

…eLog

* Add writeFilterSensitiveLog to UnionGenerator with check for sensitive member * Return $undefined if no value matched in filterSensitiveLog * Remove fromEntries as TSConfig doesn't have it on type ObjectConstructor Can be fixed by adding es2017 in lib https://stackoverflow.com/questions/45422573 But waiting for root TSConfig to be pushed * Non-sensitive SimpleShape in Union.filterSensitiveLog * Add check for obj.$unknown defined in Union.filterSensitiveLog * Call writeStructureFilterSensitiveLog from Union * Union.filterSensitiveLog for complex members * Add check for UnionShape in filterSensitiveLog * Update documentation for UnionGenerator * Add newlines within Union components * Fix checkstyle errors * Test callsFilterForUnionWithSensitiveData * Test callsFilterInUnionWithSensitiveData * Test filtersSensitiveUnion * Test callsFilterForUnionWithoutSensitiveData * Test callsFilterInUnionWithoutSensitiveData * rename test-insensitive-structure * Test filtersSensitiveMemberPointingToUnion * Test callsFilterForListWithUnionWithSensitiveData * Test callsFilterInListWithUnionWithSensitiveData * Test callsFilterForMapWithUnionWithSensitiveData * Test callsFilterInMapWithUnionWithSensitiveData * Fix ordering of objects in callsFilterInUnionWithoutSensitiveData * Remove unused memberTarget instanceof UnionShape * Disable filterSensitiveLog for event stream * Rename STREAMING_TRAIT to STREAMING_CONTENT * Log unknown union member as 'UNKNOWN' * Use isStructureShape() and isUnionShape() instead of instanceof * Added newline at the end of .smithy test files * Add tests for complex shapes inside Union * Create new function writeMemberFilterSensitiveLog * Call writeMemberFilterSensitiveLog from Union * Call writeMemberFilterSensitiveLog from Collection/Member * Throw CodegenException in the else block of writeMemberFilterSensitiveLog

trivikr mentioned this pull request Jul 1, 2020

feat: [old] add filterSensitiveLog for Union types aws/aws-sdk-js-v3#1314

Closed

trivikr marked this pull request as ready for review July 6, 2020 16:55

trivikr marked this pull request as draft July 6, 2020 21:01

kstich self-requested a review July 13, 2020 18:58

trivikr force-pushed the union-filter-sensitive-log branch from 7b70ddf to 2f8681f Compare August 18, 2020 22:56

trivikr mentioned this pull request Aug 18, 2020

feat: add filterSensitiveLog for Union types aws/aws-sdk-js-v3#1447

Merged

trivikr marked this pull request as ready for review August 19, 2020 00:40

kstich requested changes Aug 21, 2020

View reviewed changes

trivikr force-pushed the union-filter-sensitive-log branch from b67715f to 0382f0e Compare August 21, 2020 16:09

kstich reviewed Aug 25, 2020

View reviewed changes

trivikr added 19 commits August 25, 2020 02:36

Add writeFilterSensitiveLog to UnionGenerator with check for sensitiv…

b897f8d

…e member

Return $undefined if no value matched in filterSensitiveLog

561f40a

Remove fromEntries as TSConfig doesn't have it on type ObjectConstructor

002a84b

Can be fixed by adding es2017 in lib https://stackoverflow.com/questions/45422573 But waiting for root TSConfig to be pushed

Non-sensitive SimpleShape in Union.filterSensitiveLog

4b7cecd

Add check for obj.$unknown defined in Union.filterSensitiveLog

22d6bc2

Call writeStructureFilterSensitiveLog from Union

6189a58

Union.filterSensitiveLog for complex members

c648d9b

Add check for UnionShape in filterSensitiveLog

97412a4

Update documentation for UnionGenerator

4d13d52

Add newlines within Union components

7a06e94

Fix checkstyle errors

b99b1e6

Test callsFilterForUnionWithSensitiveData

72662ea

Test callsFilterInUnionWithSensitiveData

4a19728

Test filtersSensitiveUnion

424ee10

Test callsFilterForUnionWithoutSensitiveData

91fa099

Test callsFilterInUnionWithoutSensitiveData

6bd3830

rename test-insensitive-structure

dfb9d1c

Test filtersSensitiveMemberPointingToUnion

8d165e4

Test callsFilterForListWithUnionWithSensitiveData

b800fa3

trivikr added 14 commits August 25, 2020 02:36

Test callsFilterInListWithUnionWithSensitiveData

69a8c46

Test callsFilterForMapWithUnionWithSensitiveData

4eb95bf

Test callsFilterInMapWithUnionWithSensitiveData

967e673

Fix ordering of objects in callsFilterInUnionWithoutSensitiveData

7ad784f

Remove unused memberTarget instanceof UnionShape

bd2893b

Disable filterSensitiveLog for event stream

1c8f28a

Rename STREAMING_TRAIT to STREAMING_CONTENT

016b071

Log unknown union member as 'UNKNOWN'

dce7317

Use isStructureShape() and isUnionShape() instead of instanceof

62fa62d

Added newline at the end of .smithy test files

4fbbd93

Add tests for complex shapes inside Union

7e62b08

Create new function writeMemberFilterSensitiveLog

d83f565

Call writeMemberFilterSensitiveLog from Union

ca63ff5

Call writeMemberFilterSensitiveLog from Collection/Member

4d94416

trivikr force-pushed the union-filter-sensitive-log branch from e5361be to 4d94416 Compare August 25, 2020 02:37

Throw CodegenException in the else block of writeMemberFilterSensitiv…

17b63af

…eLog

kstich approved these changes Aug 25, 2020

View reviewed changes

kstich merged commit 4b001eb into smithy-lang:master Aug 25, 2020

trivikr deleted the union-filter-sensitive-log branch October 11, 2022 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add filterSensitiveLog for Union types #190

Add filterSensitiveLog for Union types #190

Uh oh!

trivikr commented Jul 1, 2020

Uh oh!

trivikr commented Jul 6, 2020

Uh oh!

Uh oh!

kstich Aug 21, 2020

Uh oh!

trivikr Aug 21, 2020

Uh oh!

Uh oh!

Uh oh!

kstich Aug 21, 2020

Uh oh!

trivikr Aug 21, 2020 •

edited

Loading

Uh oh!

trivikr Aug 21, 2020 •

edited

Loading

Uh oh!

kstich Aug 21, 2020

Uh oh!

trivikr Aug 21, 2020

Uh oh!

kstich Aug 25, 2020

Uh oh!

trivikr Aug 25, 2020

Uh oh!

trivikr Aug 25, 2020

Uh oh!

kstich Aug 25, 2020

Uh oh!

trivikr Aug 25, 2020

Uh oh!

Uh oh!

Add filterSensitiveLog for Union types #190

Add filterSensitiveLog for Union types #190

Uh oh!

Conversation

trivikr commented Jul 1, 2020

Uh oh!

trivikr commented Jul 6, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trivikr Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trivikr Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

trivikr Aug 21, 2020 •

edited

Loading

trivikr Aug 21, 2020 •

edited

Loading