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 maps/lists with sensitive members not redacted #3765

Merged
merged 5 commits into from
Jul 17, 2024

Conversation

landonxjames
Copy link
Contributor

Motivation and Context

Lists and maps with sensitive members were not being properly redacted. Closes #3757

Description

Updated Shape.shouldRedact method to properly check for list and map shapes with sensitive members. Also updated the test definition so it would actually run since previously the test code was generated in a nested function inside a no-op function and never run.

Testing

Added test cases for lists with sensitive members, maps with sensitive keys, and maps with sensitive values.

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates

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

@landonxjames landonxjames requested review from a team as code owners July 16, 2024 18:23
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Contributor

@aajtodd aajtodd left a comment

Choose a reason for hiding this comment

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

This has a lot of changes that actually seem to relax debug/display implementations. Have you verified this is the desired/correct behavior change? Were we over redacting before?

@landonxjames
Copy link
Contributor Author

This has a lot of changes that actually seem to relax debug/display implementations. Have you verified this is the desired/correct behavior change? Were we over redacting before?

Good catch! That was a bug (same one causing the CI failures) and should be fixed in 4b398c7

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@ysaito1001
Copy link
Contributor

ysaito1001 commented Jul 17, 2024

Great work! Probably this warrants a CHANGELOG entry with a bug-fix being true.

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@landonxjames landonxjames added this pull request to the merge queue Jul 17, 2024
Merged via the queue into main with commit c39792b Jul 17, 2024
44 checks passed
@landonxjames landonxjames deleted the landonxjames/sensitive-lists branch July 17, 2024 18:30
@david-perez
Copy link
Contributor

This needs to be ported over to ServerBuilderGenerator too.

It also looks like we never added Debug implementation to ServerBuilderGeneratorWithoutPublicConstrainedTypes.

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.

@sensitive doesn't work when applied to list and map targets
4 participants