Skip to content

Conversation

@lucix-aws
Copy link
Contributor

@lucix-aws lucix-aws commented May 9, 2022

Description of changes

Implements recursion detection SEP.

Companion PR in smithy-kotlin: smithy-lang/smithy-kotlin#636

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 May 9, 2022

A new generated diff is ready to view: __generated-main...__generated-feat-recursion-detection

@github-actions
Copy link

github-actions bot commented May 9, 2022

A new generated diff is ready to view: __generated-main...__generated-feat-recursion-detection

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.

Looks great overall

*/
@InternalSdkApi
public class RecursionDetection(
private val env: EnvironmentProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

this should just default to the real platform environment provider

}

override fun renderProperties(writer: KotlinWriter) {
writer.addImport(middlewareSymbol)
Copy link
Contributor

Choose a reason for hiding this comment

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

these addImport calls should no longer be necessary. The #T formatter will import symbols automatically and setting a default for the environment provider will make the platform symbol unnecessary.

@lucix-aws lucix-aws requested a review from aajtodd May 9, 2022 20:20
@github-actions
Copy link

github-actions bot commented May 9, 2022

A new generated diff is ready to view: __generated-main...__generated-feat-recursion-detection

@github-actions
Copy link

github-actions bot commented May 9, 2022

A new generated diff is ready to view: __generated-main...__generated-feat-recursion-detection

Copy link
Contributor

@ianbotsf ianbotsf left a comment

Choose a reason for hiding this comment

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

Looks like we're missing a changelog entry here.

Comment on lines +16 to +19
/**
* HTTP middleware to add the recursion detection header where required.
*/
class RecursionDetectionMiddleware : ProtocolMiddleware {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: This middleware doesn't override isEnabledFor. Just confirming...we want this to run for every operation in every AWS service?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thats a good question, it could be limited to only services that have an integration that causes recursion if we know that set. Otherwise it would have to be every service I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per post-standup discussion, we'll leave it on every request since

  1. defining the set of services that could be part of a recursive loop is non-trivial, and is subject to constant change
  2. the risk of causing a runaway workload for a user (large bill, eroded trust) is not worth selectively including the header

@lucix-aws lucix-aws requested a review from ianbotsf May 10, 2022 15:48
@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-feat-recursion-detection

@lucix-aws lucix-aws requested a review from ianbotsf May 10, 2022 16:56
@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-feat-recursion-detection

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 8 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-feat-recursion-detection

@lucix-aws lucix-aws merged commit 4227caf into main May 10, 2022
@lucix-aws lucix-aws deleted the feat-recursion-detection branch May 10, 2022 18:47
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