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

Add filterSensitiveLog method to Structure namespaces #170

Merged
merged 50 commits into from
May 27, 2020

Conversation

trivikr
Copy link
Contributor

@trivikr trivikr commented May 1, 2020

Issue #, if available:
N/A

Description of changes:
This PR covers all cases for filterSensitiveLogs

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 trivikr changed the title CodeGen for filterSensitiveLog Add filterSensitiveLog method for Structure namespaces May 1, 2020
@trivikr trivikr changed the title Add filterSensitiveLog method for Structure namespaces Add filterSensitiveLog method to Structure namespaces May 1, 2020
@trivikr
Copy link
Contributor Author

trivikr commented May 1, 2020

CodeGen PR aws/aws-sdk-js-v3#1130

Copy link
Contributor

@kstich kstich left a comment

Choose a reason for hiding this comment

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

There's a lot of duplicate type checking, casting, and iteration logic in these methods. It also looks like some instances where isIterationRequired might need to be used are dropped because of this.

You can reduce this surface by implementing a ShapeVisitor.Default<Void> that writes and recurses only for shapes that sensitive iteration is required. It would look loosely similar to the SmithyIdlModelSerializer and recurse using something like memberTarget.accept(this);.

@trivikr trivikr marked this pull request as ready for review May 13, 2020 07:03
@trivikr
Copy link
Contributor Author

trivikr commented May 15, 2020

There's a lot of duplicate type checking, casting, and iteration logic in these methods.

You can reduce this surface by implementing a ShapeVisitor.Default that writes and recurses only for shapes that sensitive iteration is required

Any Shape has to be call filterSensitiveLog for its members which are not of SimpleShape type, as the work of filtering sensitive values is delegated to the members. Using a ShapeVisitor won't help in this case.

From the TypeScript playground example of array of objects, the testStructure or testList doesn't know which members of User are sensitive and call User.filterSensitiveLog

@trivikr
Copy link
Contributor Author

trivikr commented May 15, 2020

It also looks like some instances where isIterationRequired might need to be used are dropped because of this.

The purpose of isIterationRequired is to not call filterSensitiveLog function for complex shapes of SimpleShape. For example:

  • array of SimpleShapes
  • array of arrays of SimpleShapes
  • map of SimpleShapes
  • map of maps of SimpleShapes
  • array of maps of SimpleShapes
  • maps of arrays of SimpleShapes

Can you share an example where isIterationRequired might be used where not required, or not used where required?

@kstich kstich merged commit 5fb9184 into smithy-lang:master May 27, 2020
@trivikr trivikr deleted the surface-sensitive-trait branch May 27, 2020 22:02
srchase pushed a commit to srchase/smithy-typescript that referenced this pull request Mar 17, 2023
* Add basic toString function

* Add REDACTED to member if it has SensitiveTrait

* Use getMemberTrait to find SensitiveTrait

* Move code to StructuredMemberWriter

* Add SENSITIVE_STRING for optional members only if they're present

* chore: remove isRequired check as all members are either undefined or optional

* feat: call toString on Structures

* chore: rename toString to filterSensitiveLog

* feat: add filterSensitiveLog for Array

* feat: added support for list of lists

* Move map open and end outside if-else block

* Fix issues with writeFilterSensitiveLogForArray

Noticed while writing TypeScript playground https://tiny.amazon.com/f5o7auri

* Fix java.lang.StackOverflowError

* Simplify code by storing member shape in memberShape

* Use openBlock instead of write

* Pass arrayMember in writeFilterSensitiveLogForArray

* Fix bug with Array<Array<SimpleShape>>

* filterSensitiveLog for MapShape inside Structure

* Move reducer function definition outside memberShape comparison

* Remove redundant code by doing iteration only for some shapes

* Recursively call isIterationRequired to remove redundant code

* filterSensitiveLog for Map inside Collection

* Explicitly return any from filterSensitiveLog method

* filterSensitiveLog for Collection inside Map

* filterSensitiveLog for Map inside Map

* Fix bug in filterSensitiveLog for Map of Map

* chore: rename writeFilterSensitiveLogFor<X> to write<X>FilterSensitiveLog

* Simplified method isIterationRequired

* Add period at the end of comments

* Removed braces for variable writes

* Used positional parameters in write() calls

* Use destructuring in writeMapFilterSensitiveLog

* Throw error in filterSensitiveLog where the path should never reach

* Make writeX methods private

* Add writeStructureFilterSensitiveLog

* Edge case for collection with sensitive trait

* Edge case for map with Sensitive trait

* Refactor to simplify code

* Rename config -> structuredMemberWriter

* Move writeFilterSensitiveLog above private functions

* Remove shape from writeFilterSensitiveLog

* Rename memberShape to memberTarget

* Create getSanitizedMemberName method

* Fix for ./gradlew test to be successful

* Add tests for filterSensitiveLog

* Updated documentation to clarify getSanitizedMemberName

* Use structureTarget.hasTrait in writeStructureFilterSensitiveLog

* Added tests for callsFilterIn<X>WithSensitiveData

* Modify tests for sensitive List/Map/Structure

* Add tests for member pointing to sensitive structure/list/map
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants