From 0200a0b2478c5913b7a88cfecf9d23972e0632f8 Mon Sep 17 00:00:00 2001 From: Zoe Wang <33073555+zoewangg@users.noreply.github.com> Date: Fri, 18 Jul 2025 15:04:10 -0700 Subject: [PATCH] Refactor guidelines and add kiro steering docs --- .gitignore | 1 + .../steering/async-programming-guidelines.md | 9 + .kiro/steering/aws-sdk-java-v2-general.md | 38 ++++ .../client-configuration-guidelines.md | 9 + .kiro/steering/code-generation-guidelines.md | 9 + .kiro/steering/javadoc-guidelines.md | 9 + .kiro/steering/logging-guidelines.md | 9 + .kiro/steering/reactive-streams-guidelines.md | 9 + .kiro/steering/testing-guidelines.md | 9 + docs/design/APIReference.md | 28 --- docs/design/UseOfCompletableFuture.md | 20 -- .../ClientConfiguration.md | 8 +- .../FavorStaticFactoryMethods.md | 8 +- .../NamingConventions.md | 14 +- docs/guidelines/README.md | 38 ++++ docs/{design => guidelines}/UseOfOptional.md | 0 .../async-programming-guidelines.md | 88 ++++++++ docs/guidelines/aws-sdk-java-v2-general.md | 192 ++++++++++++++++ docs/guidelines/code-generation-guidelines.md | 211 ++++++++++++++++++ docs/guidelines/javadoc-guidelines.md | 149 +++++++++++++ docs/guidelines/logging-guidelines.md | 205 +++++++++++++++++ .../guidelines/reactive-streams-guidelines.md | 40 ++++ docs/guidelines/testing-guidelines.md | 170 ++++++++++++++ 23 files changed, 1208 insertions(+), 65 deletions(-) create mode 100644 .kiro/steering/async-programming-guidelines.md create mode 100644 .kiro/steering/aws-sdk-java-v2-general.md create mode 100644 .kiro/steering/client-configuration-guidelines.md create mode 100644 .kiro/steering/code-generation-guidelines.md create mode 100644 .kiro/steering/javadoc-guidelines.md create mode 100644 .kiro/steering/logging-guidelines.md create mode 100644 .kiro/steering/reactive-streams-guidelines.md create mode 100644 .kiro/steering/testing-guidelines.md delete mode 100644 docs/design/APIReference.md delete mode 100644 docs/design/UseOfCompletableFuture.md rename docs/{design => guidelines}/ClientConfiguration.md (97%) rename docs/{design => guidelines}/FavorStaticFactoryMethods.md (95%) rename docs/{design => guidelines}/NamingConventions.md (90%) create mode 100644 docs/guidelines/README.md rename docs/{design => guidelines}/UseOfOptional.md (100%) create mode 100644 docs/guidelines/async-programming-guidelines.md create mode 100644 docs/guidelines/aws-sdk-java-v2-general.md create mode 100644 docs/guidelines/code-generation-guidelines.md create mode 100644 docs/guidelines/javadoc-guidelines.md create mode 100644 docs/guidelines/logging-guidelines.md create mode 100644 docs/guidelines/reactive-streams-guidelines.md create mode 100644 docs/guidelines/testing-guidelines.md diff --git a/.gitignore b/.gitignore index d9d99cd3decb..308916e8938b 100644 --- a/.gitignore +++ b/.gitignore @@ -30,4 +30,5 @@ target/ # VS Code .vscode/* +bin/ diff --git a/.kiro/steering/async-programming-guidelines.md b/.kiro/steering/async-programming-guidelines.md new file mode 100644 index 000000000000..5d20078eb719 --- /dev/null +++ b/.kiro/steering/async-programming-guidelines.md @@ -0,0 +1,9 @@ +--- +title: Asynchronous Programming Guidelines for AWS SDK v2 +inclusion: fileMatch +fileMatchPattern: "**/*.java" +--- + +# Asynchronous Programming Guidelines for AWS SDK v2 + +For complete guidelines, refer to: #[[file:docs/guidelines/async-programming-guidelines.md]] \ No newline at end of file diff --git a/.kiro/steering/aws-sdk-java-v2-general.md b/.kiro/steering/aws-sdk-java-v2-general.md new file mode 100644 index 000000000000..c20a851b3bc3 --- /dev/null +++ b/.kiro/steering/aws-sdk-java-v2-general.md @@ -0,0 +1,38 @@ +--- +title: AWS SDK for Java v2 General Guidelines +inclusion: always +--- + +# AWS SDK for Java v2 General Guidelines + +## Development Environment + +- **Java Version**: Java 8 is the target language version for the AWS SDK for Java v2 +- **Build System**: Maven is used for building and dependency management + +## Build Instructions + +To check if the SDK compiles properly, follow these steps: + +1. **Build with dependencies**: Only need to run once. First run the build command with `--am` (also-make) flag to build all dependencies: + ```bash + mvn clean install -pl :${module} -P quick --am + ``` + Example for S3 module: + ```bash + mvn clean install -pl :s3 -P quick --am + ``` + +2. **Build module only**: Then run the build for just the specific module (skips testing and checkstyles): + ```bash + mvn clean install -pl :${module} -P quick + ``` + +3. **Run tests**: To run tests, use the standard command without the quick profile: + ```bash + mvn clean install -pl :${module} + ``` + +## Guidelines + +All detailed guidelines are in #[[file:docs/guidelines/aws-sdk-java-v2-general.md]] \ No newline at end of file diff --git a/.kiro/steering/client-configuration-guidelines.md b/.kiro/steering/client-configuration-guidelines.md new file mode 100644 index 000000000000..ed00a7b3252d --- /dev/null +++ b/.kiro/steering/client-configuration-guidelines.md @@ -0,0 +1,9 @@ +--- +title: "Client Configuration Guidelines for AWS SDK v2" +inclusion: fileMatch +fileMatchPattern: "**/*{Config,Configuration,Builder}*.java" +--- + +# Client Configuration Guidelines for AWS SDK v2 + +For complete guidelines, refer to: #[[file:docs/guidelines/ClientConfiguration.md]] \ No newline at end of file diff --git a/.kiro/steering/code-generation-guidelines.md b/.kiro/steering/code-generation-guidelines.md new file mode 100644 index 000000000000..be40dc727d3e --- /dev/null +++ b/.kiro/steering/code-generation-guidelines.md @@ -0,0 +1,9 @@ +--- +title: Code Generation Guidelines for AWS SDK v2 +inclusion: fileMatch +fileMatchPattern: "{codegen/**/*.java,**/poet/**/*.java}" +--- + +# Code Generation Guidelines for AWS SDK v2 + +For complete guidelines, refer to: #[[file:docs/guidelines/code-generation-guidelines.md]] diff --git a/.kiro/steering/javadoc-guidelines.md b/.kiro/steering/javadoc-guidelines.md new file mode 100644 index 000000000000..94d3e52a36f5 --- /dev/null +++ b/.kiro/steering/javadoc-guidelines.md @@ -0,0 +1,9 @@ +--- +title: Javadoc Guidelines for AWS SDK v2 +inclusion: fileMatch +fileMatchPattern: "{**/src/main/**/*.java}" +--- + +# Javadoc Guidelines for AWS SDK v2 + +For complete guidelines, refer to: #[[file:docs/guidelines/javadoc-guidelines.md]] \ No newline at end of file diff --git a/.kiro/steering/logging-guidelines.md b/.kiro/steering/logging-guidelines.md new file mode 100644 index 000000000000..1a17df7e98ef --- /dev/null +++ b/.kiro/steering/logging-guidelines.md @@ -0,0 +1,9 @@ +--- +title: "Logging Guidelines for AWS SDK v2" +inclusion: fileMatch +fileMatchPattern: "**/*.java" +--- + +# Logging Guidelines for AWS SDK v2 + +For complete guidelines, refer to: #[[file:docs/guidelines/logging-guidelines.md]] \ No newline at end of file diff --git a/.kiro/steering/reactive-streams-guidelines.md b/.kiro/steering/reactive-streams-guidelines.md new file mode 100644 index 000000000000..7965062d7d86 --- /dev/null +++ b/.kiro/steering/reactive-streams-guidelines.md @@ -0,0 +1,9 @@ +--- +title: "Reactive Streams Implementation Guidelines" +inclusion: fileMatch +fileMatchPattern: '**/*{Publisher,Subscriber}*.java' +--- + +# Reactive Streams Implementation Guidelines + +See #[[file:docs/guidelines/reactive-streams-guidelines.md]] for detailed implementation guidelines, patterns, and testing requirements. \ No newline at end of file diff --git a/.kiro/steering/testing-guidelines.md b/.kiro/steering/testing-guidelines.md new file mode 100644 index 000000000000..5f26e9d90dbe --- /dev/null +++ b/.kiro/steering/testing-guidelines.md @@ -0,0 +1,9 @@ +--- +title: Testing Guidelines for AWS SDK v2 +inclusion: fileMatch +fileMatchPattern: "**/{test,it}/**/*.java" +--- + +# Testing Guidelines for AWS SDK v2 + +For complete guidelines, refer to: #[[file:docs/guidelines/testing-guidelines.md]] \ No newline at end of file diff --git a/docs/design/APIReference.md b/docs/design/APIReference.md deleted file mode 100644 index c5ec1dfedcee..000000000000 --- a/docs/design/APIReference.md +++ /dev/null @@ -1,28 +0,0 @@ -**Design:** Convention, **Status:** [Accepted](README.md) - -## API Reference - -This page describes a general guideline for API Reference. - -### Definition -- SDK public APIs: all classes/interfaces annotated with `@SdkPublicApi` -- SDK protected APIs: all classes/interfaces annotated with `@SdkProtectedApi` -- SDK internal APIs: all classes/interfaces annotated with `@SdkInternalApi` - -### General Guideline - -- All SDK public APIs MUST have documentation. -- All public methods in SDK public APIs MUST have documentation except the ones that implement or override a method in an interface or superclass. -- For SDK protected APIs and internal APIs, documentation is recommended but not required. -- Javadocs for SDK public APIs in high level libraries such as DynamoDB Enhanced Client SHOULD include code snippets. - -### Style guideline - -- Javadoc MUST be properly formatted following the [Javadoc standard](https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html). -- A single `

` MUST be used between paragraphs. -- Use `@link` and `@see` tags to point to related methods/classes. -- Use `@throws` to specify exceptions that could be thrown from a method. -- Use `@snippet` to add code snippets. [External code snippets](https://docs.oracle.com/en/java/javase/18/code-snippet/index.html#external-snippets) SHOULD be favored over inline code snippets. -- Use `@deprecated` tag for deprecated methods. The replacement MUST be documented and linked using `{@link}`. -- Avoid using `@version` and `@since` tags. - diff --git a/docs/design/UseOfCompletableFuture.md b/docs/design/UseOfCompletableFuture.md deleted file mode 100644 index 376d2f30b74f..000000000000 --- a/docs/design/UseOfCompletableFuture.md +++ /dev/null @@ -1,20 +0,0 @@ -**Design:** Convention, **Status:** [Accepted](README.md) - -## Use of CompletableFuture - -Operations on the asynchronous clients return [`CompleteableFuture`][1] where `T` is the response type for the operation. This is somewhat curious in that [`CompleteableFuture`][1] is a concrete implementation rather than an interface. The alternative to returning a [`CompleteableFuture`][1] would be to return a [`CompletionStage`][2], an interface intended to allow chaining of asynchronous operations. - -The key advantage of [`CompleteableFuture`][1] is that it implements both the [`CompletionStage`][2] and [`Future`][3] interfaces - giving users of the SDK maximum flexibility when it comes to handling responses from their asynchronous calls. - -Currently [`CompleteableFuture`][1] is the only implementation of [`CompletionStage`][2] that ships with the JDK. Whilst it's possible that future implementations will be added [`CompletionStage`][2] will always be tied to [`CompleteableFuture`][1] via the [`#toCompletableFuture`][4] method. Additionally, [`CompleteableFuture`][1] is not a `final` class and thus could be extended if there was a requirement to do so. - -One of the perceived risks with exposing [`CompleteableFuture`][1] rather than [`CompletionStage`][2] is that a user of the SDK may spuriously call [`#complete`](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletableFuture.html#complete-T-) or [`#completeExceptionally`](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletableFuture.html#completeExceptionally-java.lang.Throwable-) which could cause unintended consequences in their application and the SDK itself. However this risk is also present on the [`CompletionStage`][2] via the [`#toCompletableFuture`][4] method. - -If the [`CompletionStage`][2] interface did not have a [`#toCompletableFuture`][4] method the argument for using it would be a lot stronger, however as it stands the interface and its implementation are tightly coupled. - -Using [`CompleteableFuture`][1] gives users the best bang for their buck without much additional risk. - -[1]: https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletableFuture.html -[2]: https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletionStage.html -[3]: https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Future.html -[4]: https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletionStage.html#toCompletableFuture-- diff --git a/docs/design/ClientConfiguration.md b/docs/guidelines/ClientConfiguration.md similarity index 97% rename from docs/design/ClientConfiguration.md rename to docs/guidelines/ClientConfiguration.md index 58c1ef036ebe..0049fef8f439 100644 --- a/docs/design/ClientConfiguration.md +++ b/docs/guidelines/ClientConfiguration.md @@ -1,10 +1,8 @@ -**Design:** Convention, **Status:** [Accepted](README.md) - -## Client Configuration +# Client Configuration This page describes the structure and conventions used for client configuration objects. Client configuration objects are any objects used to configure an AWS client builder. -#### Example +## Example This section walks through an example configuration class structure and describes each of its components at a high level. @@ -83,7 +81,7 @@ public final class SdkConfiguration // (3) 11. One "set" method should be created for each option to mutate the value in this builder. This method's name should exactly match the name of the field it is setting. 12. Each option should have a bean-style setter to allow configuring the object reflectively using `Inspector`-based frameworks, like spring XML. -#### Configuration Fields +### Configuration Fields This section details the semantics of configuration fields. diff --git a/docs/design/FavorStaticFactoryMethods.md b/docs/guidelines/FavorStaticFactoryMethods.md similarity index 95% rename from docs/design/FavorStaticFactoryMethods.md rename to docs/guidelines/FavorStaticFactoryMethods.md index b42c246de74f..b17dc39aed48 100644 --- a/docs/design/FavorStaticFactoryMethods.md +++ b/docs/guidelines/FavorStaticFactoryMethods.md @@ -1,10 +1,10 @@ **Design:** Convention, **Status:** [Accepted](README.md) -## Favor Static Factory Methods Over Constructors +# Favor Static Factory Methods Over Constructors This page describes the structures and conventions used to initialize a class. -### Static Factory Methods vs. Constructors +## Static Factory Methods vs. Constructors Static factory methods are preferable than constructors for the following reasons: - Static factory methods provide meaningful names compared with constructors, which improves the readability of the codes by describing the objects being returned. ```java @@ -23,7 +23,7 @@ There are a few disadvantages of static factory methods: In general, we should favor static factory methods over constructors. -### Example +## Example ```java public class DefaultCredentialsProvider implements AwsCredentialsProvider, SdkAutoCloseable { @@ -52,7 +52,7 @@ public class DefaultCredentialsProvider implements AwsCredentialsProvider, SdkAu DefaultCredentialsProvider defaultCredentialsProvider1 = DefaultCredentialsProvider.create(); DefaultCredentialsProvider defaultCredentialsProvider2 = DefaultCredentialsProvider.builder().build; ``` -### Naming Conventions +## Naming Conventions The naming conventions for the static factory methods: - `create()`, `create(params)` when creating a new instance eg: `DynamoDBClient.create()` diff --git a/docs/design/NamingConventions.md b/docs/guidelines/NamingConventions.md similarity index 90% rename from docs/design/NamingConventions.md rename to docs/guidelines/NamingConventions.md index b3b3dc76a04f..caf1ec56014c 100644 --- a/docs/design/NamingConventions.md +++ b/docs/guidelines/NamingConventions.md @@ -1,16 +1,14 @@ -**Design:** Convention, **Status:** [Accepted](README.md) - -## Naming Conventions +# Naming Conventions This page describes the naming conventions, nouns and common terms -### Class Naming +## Class Naming -#### General Rules +### General Rules * Prefer singular class names: `SdkSystemSetting`, not `SdkSystemSettings`. * Treat acronyms as a single word: `DynamoDbClient`, not `DynamoDBClient`. -#### Classes that instantiate other classes +### Classes that instantiate other classes * If the class's primary purpose is to return instances of another class: * If the "get" method has no parameters: @@ -18,7 +16,7 @@ This page describes the naming conventions, nouns and common terms * If the class does not implement `Supplier`: `{Noun}Provider` (e.g. `AwsCredentialsProvider`) * If the "get" method has parameters: `{Noun}Factory` (e.g. `AwsJsonProtocolFactory`) -#### Service-specific classes +### Service-specific classes * If the class makes service calls: * If the class can be used to invoke *every* data-plane operation: @@ -36,6 +34,6 @@ This page describes the naming conventions, nouns and common terms * If the class creates presigned URLs: `{ServiceName}Presigner` (e.g. `S3Presigner`) * If the class is a collection of various unrelated "helper" methods: `{ServiceName}Utilities` (e.g. `S3Utilities`) -### Tests Naming +## Tests Naming Test names SHOULD follow `methodToTest_when_expectedBehavior` (e.g. `close_withCustomExecutor_shouldNotCloseCustomExecutor`, `uploadDirectory_withDelimiter_filesSentCorrectly`) \ No newline at end of file diff --git a/docs/guidelines/README.md b/docs/guidelines/README.md new file mode 100644 index 000000000000..62d2a380d90c --- /dev/null +++ b/docs/guidelines/README.md @@ -0,0 +1,38 @@ +# AWS SDK for Java v2 Development Guidelines + +This directory contains comprehensive guidelines for developing with the AWS SDK for Java v2. These guidelines are organized into separate, focused documents to make them more manageable and easier to reference. + +## Available Guidelines + +### [General Guidelines](aws-sdk-java-v2-general.md) +Core development principles, code style standards, design patterns, and performance considerations that apply to all AWS SDK Java v2 development. Includes naming conventions, proper use of Optional, object method implementations, and exception handling patterns. + +### [Asynchronous Programming Guidelines](async-programming-guidelines.md) +Best practices for CompletableFuture usage, thread safety considerations, and proper handling of asynchronous operations. Covers cancellation, exception handling, and testing patterns for async code. + +### [Reactive Streams Guidelines](reactive-streams-guidelines.md) +Requirements and patterns for implementing Reactive Streams components. Ensures compliance with the Reactive Streams specification, proper backpressure handling, and mandatory TCK testing for Publisher/Subscriber implementations. + +### [Testing Guidelines](testing-guidelines.md) +Comprehensive testing strategies including unit tests, functional tests, integration tests, and specialized test types like TCK tests for reactive streams. Covers test naming conventions, mocking guidelines, and coverage expectations. + +### [Javadoc Guidelines](javadoc-guidelines.md) +Documentation standards for public APIs including formatting requirements, proper use of Javadoc tags, code snippet guidelines, and examples for different API classifications (public, protected, internal). + +### [Logging Guidelines](logging-guidelines.md) +Logging standards specific to the AWS SDK including proper use of the SDK Logger, log level guidelines, structured logging patterns, and critical rules about avoiding duplicate error reporting when exceptions are thrown. + +### [Code Generation Guidelines](code-generation-guidelines.md) +Patterns and standards for JavaPoet-based code generation including ClassSpec implementations, generator task organization, model processing, and fixture-based testing approaches for generated code validation. + +### [Naming Conventions](NamingConventions.md) +Specific naming patterns for classes, methods, and tests including service client naming, acronym handling, and test naming conventions that clearly describe the method, conditions, and expected behavior. + +### [Use of Optional](UseOfOptional.md) +Guidelines for when and how to use Optional in the SDK, including restrictions on usage in method parameters, member variables, and return types to maintain API clarity and consistency. + +### [Static Factory Methods](FavorStaticFactoryMethods.md) +Patterns for preferring static factory methods over constructors, including naming conventions for factory methods and the benefits of this approach for immutable objects and API design. + +### [Client Configuration](ClientConfiguration.md) +Structural requirements for configuration objects including immutability patterns, builder interfaces, field naming conventions, and proper handling of collection types in configuration APIs. \ No newline at end of file diff --git a/docs/design/UseOfOptional.md b/docs/guidelines/UseOfOptional.md similarity index 100% rename from docs/design/UseOfOptional.md rename to docs/guidelines/UseOfOptional.md diff --git a/docs/guidelines/async-programming-guidelines.md b/docs/guidelines/async-programming-guidelines.md new file mode 100644 index 000000000000..828814d3cad9 --- /dev/null +++ b/docs/guidelines/async-programming-guidelines.md @@ -0,0 +1,88 @@ +# Asynchronous Programming Guidelines for AWS SDK v2 + +## CompletableFuture Guidelines + +### Best Practices for CompletableFuture + +- **Read the documentation**: Always read the [CompletionStage Javadocs](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletionStage.html) to understand the nuances of CompletableFuture +- **Prefer Non-Blocking Methods for Getting Results**: + +```java + // Avoid when possible - blocks the current thread + String result = future.get(); + + // Better - use callbacks + future.thenAccept(result -> processResult(result)); +``` +- **Add stacktrace to exceptions**: When using `CompletableFuture#join`, use `CompletableFutureUtils#joinLikeSync` to preserve stacktraces +- **Don't ignore results**: Never ignore the result of a new `CompletionStage` if the parent stage can fail (unless you're absolutely sure it's safe to) +- **Don't make thread assumptions**: Never make assumptions about which thread completes the future + - CompletableFuture callbacks may execute on: + - The thread that completed the future + - A thread from the common ForkJoinPool (for async methods without explicit executor) + - A thread from a provided executor + - Thread behavior can vary based on: + - Whether the future is already completed when a callback is added + - Whether an async or non-async method is used (e.g., `thenApply` vs `thenApplyAsync`) + - The specific JVM implementation and platform + - Always ensure thread safety when: + - Accessing shared state in callbacks + - Modifying UI components (use the appropriate UI thread) + - Working with thread-affinity resources like ThreadLocals + - Example of incorrect assumption: + ```java + // INCORRECT: Assuming the callback runs on a specific thread + ThreadLocal contextHolder = new ThreadLocal<>(); + + public void processAsync(CompletableFuture responseFuture) { + Context context = new Context(); + contextHolder.set(context); // Set in current thread + + responseFuture.thenApply(response -> { + // WRONG: Assuming contextHolder still has the context + Context ctx = contextHolder.get(); // May be null if running on different thread! + return processWithContext(response, ctx); + }); + } + ``` + - Correct approach: + ```java + // CORRECT: Explicitly passing context to callback + public void processAsync(CompletableFuture responseFuture) { + Context context = new Context(); + + responseFuture.thenApply(response -> { + // Explicitly use the context passed from the outer scope + return processWithContext(response, context); + }); + } + ``` +- **Always provide custom executors**: Don't use `CompletableFuture#xxAsync` methods (like `runAsync` or `thenComposeAsync`) without providing a custom executor, as the default `ForkJoinPool.commonPool()` behavior can vary by platform +- **Handle cancellation properly**: CompletableFuture does not support automatic cancellation propagation, so use `CompletableFutureUtils#forwardExceptionTo` to manually propagate cancellation +- **Avoid chaining multiple API calls**: This can lead to cancellation issues without proper handling +- **Avoid blocking operations**: Never use `get()` or `join()` inside a CompletableFuture chain as it defeats the purpose of asynchronous execution +- **Handle exceptions properly**: Always include exception handling with `exceptionally()` or `handle()` methods +```java + CompletableFuture.supplyAsync(() -> fetchData()) + .exceptionally(ex -> { + logger.error("Error processing data", ex); + return fallbackData(); + }, executor); +``` +- **Use appropriate completion methods**: + - thenApply() - when transforming a result + - thenAccept() - when consuming a result without returning anything + - thenRun() - when executing code regardless of the result + - thenCompose() - when the next step returns a CompletableFuture +- **Test asynchronous code properly**: + - Use `CompletableFuture.join()` in tests to wait for completion + - Set appropriate timeouts for tests + +## Related Guidelines + +For reactive streams implementation guidelines, see [Reactive Streams Implementation Guidelines](reactive-streams-guidelines.md). + +## References + +- [CompletableFuture JavaDoc](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletableFuture.html) +- [CompletionStage JavaDoc](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletionStage.html) \ No newline at end of file diff --git a/docs/guidelines/aws-sdk-java-v2-general.md b/docs/guidelines/aws-sdk-java-v2-general.md new file mode 100644 index 000000000000..1053722ccae4 --- /dev/null +++ b/docs/guidelines/aws-sdk-java-v2-general.md @@ -0,0 +1,192 @@ +# AWS SDK for Java v2 General Guidelines + +## General Principles + +- Write clean, readable, and maintainable code +- Follow the SOLID principles of object-oriented design +- Favor composition over inheritance +- Program to interfaces, not implementations +- Fail fast - detect and report errors as soon as possible +- Maintain backward compatibility when modifying existing APIs + +## Code Style Standards + +- Follow Java coding conventions and the existing style in the codebase +- Use meaningful variable, method, and class names that clearly indicate purpose +- Include comprehensive Javadoc for public APIs +- Keep methods short and focused on a single responsibility +- Limit the number of method parameters (ideally 3 or fewer) +- Use appropriate access modifiers (private, protected, public) +- Follow consistent indentation and formatting + +## Common Design Patterns + +- Use builder pattern for object creation +- Follow reactive programming principles for async operations +- Use SdkException hierarchy for error handling +- Prefer immutable objects where appropriate + +## SDK Utilities + +The AWS SDK for Java v2 provides several utility classes that should be used instead of external libraries or custom implementations: + +### JSON Parsing +- **Production Code**: **MUST** use `JsonNodeParser` from the `json-utils` module (`core/json-utils/src/main/java/software/amazon/awssdk/protocols/jsoncore/JsonNodeParser.java`) for parsing JSON content +- **Test Code Exception**: Test code **MAY** use external JSON libraries like Jackson or javax.json for convenience +- **Codegen Exception**: Code generation templates **MAY** use external JSON libraries when appropriate +- **MUST NOT** use external JSON libraries in production SDK code +- Example usage: + ```java + JsonNodeParser parser = JsonNodeParser.create(); + JsonNode rootNode = parser.parse(jsonString); + ``` + +### Lazy Initialization +- **MUST** use `Lazy` from the `utils` module (`utils/src/main/java/software/amazon/awssdk/utils/Lazy.java`) for thread-safe lazy initialization +- **MUST NOT** implement custom lazy initialization patterns +- Example usage: + ```java + private static final Lazy EXPENSIVE_OBJECT = + new Lazy<>(() -> new ExpensiveObject()); + + public ExpensiveObject getExpensiveObject() { + return EXPENSIVE_OBJECT.getValue(); + } + ``` + +### Caching with TTL +- **MUST** use `CachedSupplier` from the `utils` module (`utils/src/main/java/software/amazon/awssdk/utils/cache/CachedSupplier.java`) for caching values with time-to-live (TTL) functionality +- **MUST NOT** implement custom caching mechanisms with expiration logic +- Use `CachedSupplier` when you need to cache expensive operations that should be refreshed periodically +- Example usage: + ```java + private final Supplier tokenCache = CachedSupplier.builder(() -> + RefreshResult.builder(fetchAuthToken()) + .staleTime(Instant.now().plus(Duration.ofMinutes(5))) + .build()) + .cachedValueName("AuthToken") + .build(); + + public AuthToken getAuthToken() { + return tokenCache.get(); + } + + private AuthToken fetchAuthToken() { + // Expensive operation to fetch token + return callAuthService(); + } + ``` +- **Key Features**: + - Thread-safe caching with automatic expiration + - Configurable stale time for cache invalidation + - Optional prefetch functionality for proactive refresh + - Built-in logging and debugging support via `cachedValueName` + +## Naming Conventions + +See [Naming Conventions Guidelines](NamingConventions.md) for detailed information. + +## Class Initialization + +See [Favor Static Factory Methods Guidelines](FavorStaticFactoryMethods.md) for detailed information. + +## Use of Optional + +See [Use of Optional Guidelines](UseOfOptional.md) for detailed information. + +## Object Methods (toString, equals, hashCode) + +- All public POJO classes **MUST** implement `toString()`, `equals()`, and `hashCode()` methods +- When adding new fields to existing POJO classes, these methods **MUST** be updated to include the new fields +- Implementation guidelines: + - **MUST** use the SDK's `ToString` utility class (`utils/src/main/java/software/amazon/awssdk/utils/ToString.java`) for consistent formatting: + ```java + @Override + public String toString() { + return ToString.builder("YourClassName") + .add("fieldName1", fieldValue1) + .add("fieldName2", fieldValue2) + .build(); + } + ``` + - **MUST NOT** include fields that could be considered sensitive such as credentials + - `equals()`: Compare all fields for equality, including proper null handling + - `hashCode()`: Include all fields in the hash code calculation +- Unit tests **MUST** be added using EqualsVerifier to ensure all fields are properly included in equals and hashCode implementations. Example: + ```java + @Test + public void equalsHashCodeTest() { + EqualsVerifier.forClass(YourClass.class) + .withNonnullFields("requiredFields") + .verify(); + } + ``` +- See example implementation in `core/regions/src/test/java/software/amazon/awssdk/regions/PartitionEndpointKeyTest.java` + +## Exception Handling + +- Avoid throwing checked exceptions in public APIs +- Don't catch exceptions unless you can handle them properly +- Always include meaningful error messages +- Clean up resources in finally blocks or use try-with-resources +- Don't use exceptions for flow control because exceptions are expensive + ```java + // BAD: Calling validators expecting them to throw + public ValidationResult validateRequest(Request request) { + try { + validateRequired(request); // Throws if required fields missing + validateFormat(request); // Throws if format invalid + validatePermissions(request); // Throws if permissions insufficient + return ValidationResult.success(); + } catch (ValidationException e) { + return ValidationResult.failure(e.getMessage()); + } + } + + // GOOD: Explicit validation with return values + public ValidationResult validateRequest(Request request) { + ValidationResult result = validateRequired(request); + if (!result.isValid()) { + return result; + } + + result = validateFormat(request); + if (!result.isValid()) { + return result; + } + + result = validatePermissions(request); + return result; + } + ``` +- **MUST NOT** catch and rethrow the same exception type + +```java +// BAD: Catching and rethrowing with the same message +public void processMessage(String message) { + try { + parseMessage(message); + } catch (SnsMessageParsingException e) { + throw new SnsMessageParsingException(e.getMessage(), e); + } + } +// BAD: Even with additional context, don't catch and rethrow the same exception type +public void processMessage(String message) { + try { + parseMessage(message); + } catch (SnsMessageParsingException e) { + // WRONG - Don't catch and rethrow SnsMessageParsingException as SnsMessageParsingException + throw SnsMessageParsingException.builder() + .message("Failed to process SNS message in batch operation. " + e.getMessage() + + " Message index: " + getCurrentMessageIndex()) + .cause(e) + .build(); + } +} +``` + +### Error Message Guidelines +- **MUST** provide context-specific error messages that explain what went wrong and how to fix it +- **MUST** include relevant details (field names, expected formats, received values) without exposing sensitive information +- **MUST** chain exceptions properly to preserve error context while adding meaningful information +- **SHOULD** include troubleshooting guidance when appropriate \ No newline at end of file diff --git a/docs/guidelines/code-generation-guidelines.md b/docs/guidelines/code-generation-guidelines.md new file mode 100644 index 000000000000..daf1978924d9 --- /dev/null +++ b/docs/guidelines/code-generation-guidelines.md @@ -0,0 +1,211 @@ +# Code Generation Guidelines + +## Table of Contents +- [Overview](#overview) +- [Architecture Overview](#architecture-overview) +- [Poet Specification Development](#poet-specification-development) +- [Generator Task Organization](#generator-task-organization) +- [Generated Code Standards](#generated-code-standards) +- [Model Processing](#model-processing) +- [Customization and Extension](#customization-and-extension) +- [Code Examples and References](#code-examples-and-references) + +## Overview + +This document provides guidelines for developing and maintaining code generation components in the AWS SDK for Java v2. Code generation is a critical part of the SDK that transforms service models into Java client code using JavaPoet for programmatic Java code generation. + +## Architecture Overview + +The AWS SDK v2 code generation system is built around several key components: + +### Core Components +- **CodeGenerator**: Main orchestrator that coordinates the generation process +- **IntermediateModel**: Processed service model used for code generation +- **Poet Specs**: JavaPoet-based specifications that define generated classes +- **Generator Tasks**: Parallel execution units that generate specific code artifacts + +### JavaPoet Integration +The SDK uses Square's JavaPoet library for type-safe Java code generation: +- **ClassSpec**: Interface defining generated class specifications +- **TypeSpec**: JavaPoet's class/interface/enum builders +- **MethodSpec**: Method generation specifications +- **FieldSpec**: Field generation specifications + +## Poet Specification Development + +### Creating ClassSpec Implementations + +When creating new code generators, implement the `ClassSpec` interface: + +```java +public class MyGeneratorSpec implements ClassSpec { + @Override + public TypeSpec poetSpec() { + return TypeSpec.classBuilder(className()) + .addAnnotation(PoetUtils.generatedAnnotation()) + .addModifiers(Modifier.PUBLIC) + .build(); + } + + @Override + public ClassName className() { + return ClassName.get("com.example", "GeneratedClass"); + } +} +``` + +#### ClassSpec Best Practices +- Always include the `@Generated` annotation using `PoetUtils.generatedAnnotation()` +- Use meaningful class names that reflect the generated component's purpose +- Implement `staticImports()` when static imports improve code readability +- Keep poet specifications focused on single responsibilities + +### JavaPoet Code Generation Patterns + +#### Type Safety +- Use `ClassName.get()` for referencing existing classes +- Use `ParameterizedTypeName.get()` for generic types +- Leverage `TypeVariableName` for generic type parameters +- Use `WildcardTypeName` for wildcard types + +#### Code Block Construction +```java +CodeBlock.builder() + .add("$T.<$T>builder($T.$L)\n", + SdkField.class, fieldType, + MarshallingType.class, marshallingType) + .add(".memberName($S)\n", memberName) + .add(".build()") + .build(); +``` + +#### Method Generation +```java +MethodSpec.methodBuilder("methodName") + .addModifiers(Modifier.PUBLIC) + .returns(returnType) + .addParameter(paramType, "paramName") + .addStatement("return $L", expression) + .build(); +``` + +### Poet Utilities + +#### PoetUtils Helper Methods +- `generatedAnnotation()`: Standard @Generated annotation +- `createClassBuilder()`: Class builder with @Generated annotation +- `createInterfaceBuilder()`: Interface builder with @Generated annotation +- `addJavadoc()`: Safe JavaDoc addition with proper escaping +- `buildJavaFile()`: Convert ClassSpec to JavaFile with static imports + +## Generator Task Organization + +### Task Hierarchy +Code generation is organized into hierarchical tasks: +- **AwsGeneratorTasks**: Top-level coordinator +- **CommonGeneratorTasks**: Shared model and client generation +- **AsyncClientGeneratorTasks**: Async client-specific generation +- **PaginatorsGeneratorTasks**: Paginator generation +- **EventStreamGeneratorTasks**: Event stream support +- **WaitersGeneratorTasks**: Waiter generation + +### Task Implementation Pattern +```java +public class MyGeneratorTask extends GeneratorTask { + @Override + public void compute() { + // Generate code using JavaPoet + ClassSpec spec = new MyClassSpec(model); + new PoetGeneratorTask(outputDir, fileHeader, spec).compute(); + } +} +``` + +## Generated Code Standards + +Generated code must adhere to all existing AWS SDK for Java v2 guidelines and standards. See the [Guidelines Index](README.md) for the complete list of applicable guidelines. + +## Model Processing + +### Intermediate Model Usage +The `IntermediateModel` provides processed service definitions: +- **ShapeModel**: Represents service data structures +- **MemberModel**: Represents structure members/fields +- **OperationModel**: Represents service operations +- **MetadataModel**: Service metadata and configuration + +## Customization and Extension + +### Service-Specific Customizations +- Use `CustomizationConfig` for service-specific behavior +- Document all customizations in service-specific files + +### Extension Points +- Implement custom `ClassSpec` for new generators +- Extend existing generator tasks for additional functionality +- Use `PoetExtension` for model-to-class name mapping +- Leverage `NamingStrategy` for consistent naming + +## Code Examples and References + +### Existing ClassSpec Implementations +For practical examples of JavaPoet code generation patterns, refer to these existing implementations: + +#### Model Generation +- **ShapeModelSpec**: `codegen/src/main/java/software/amazon/awssdk/codegen/poet/model/ShapeModelSpec.java` + - Demonstrates field generation with SdkField metadata + - Shows trait application for marshalling/unmarshalling + - Implements static field initialization patterns + +- **ModelBuilderSpecs**: `codegen/src/main/java/software/amazon/awssdk/codegen/poet/model/ModelBuilderSpecs.java` + - Builder interface and implementation generation + - Union type handling with enum generation + - Fluent setter method patterns + +#### Task Organization +- **AwsGeneratorTasks**: `codegen/src/main/java/software/amazon/awssdk/codegen/emitters/tasks/AwsGeneratorTasks.java` + - Composite task pattern for organizing generation + - Parallel execution coordination + +- **PoetGeneratorTask**: `codegen/src/main/java/software/amazon/awssdk/codegen/emitters/PoetGeneratorTask.java` + - Integration between ClassSpec and file output + - Error handling during code generation + +#### Utility Classes +- **PoetUtils**: `codegen/src/main/java/software/amazon/awssdk/codegen/poet/PoetUtils.java` + - Common JavaPoet patterns and utilities + - Type creation helpers and annotation management + - JavaFile building with static imports + +- **CodeGenerator**: `codegen/src/main/java/software/amazon/awssdk/codegen/CodeGenerator.java` + - Main orchestration and validation workflow + - Model processing and task execution patterns + +### Validation and Testing Patterns + +#### Fixture-Based Testing +Use static resource fixtures to validate generated code output, following the pattern in `AsyncClientClassTest`: + +```java +// From AsyncClientClassTest.java +@Test +public void asyncClientClassRestJson() { + AsyncClientClass asyncClientClass = createAsyncClientClass(restJsonServiceModels(), false); + assertThat(asyncClientClass, generatesTo("test-json-async-client-class.java")); +} +``` + +**Testing Pattern Guidelines:** +- **Static Fixtures**: Store expected generated code in `src/test/resources/` as `.java` files +- **PoetMatchers**: Use `generatesTo()` matcher to compare generated code with fixtures +- **Fixture Organization**: Group fixtures by component type (client, model, etc.) +- **Multiple Scenarios**: Test different service models and configurations +- **Whitespace Normalization**: The matcher ignores whitespace differences for robust comparison + +**Fixture File Structure:** +``` +codegen/src/test/resources/software/amazon/awssdk/codegen/poet/client/ +├── test-json-async-client-class.java # Expected async client output +├── test-query-client-class.java # Expected sync client output +├── test-custompackage-async.java # Custom package scenarios +``` diff --git a/docs/guidelines/javadoc-guidelines.md b/docs/guidelines/javadoc-guidelines.md new file mode 100644 index 000000000000..0818c7f51d35 --- /dev/null +++ b/docs/guidelines/javadoc-guidelines.md @@ -0,0 +1,149 @@ +# Javadoc Guidelines for AWS SDK v2 + +## API Classification + +The AWS SDK for Java v2 uses annotations to classify APIs: + +- **SDK Public APIs**: Classes/interfaces annotated with `@SdkPublicApi` +- **SDK Protected APIs**: Classes/interfaces annotated with `@SdkProtectedApi` +- **SDK Internal APIs**: Classes/interfaces annotated with `@SdkInternalApi` + +## Documentation Requirements + +### Required Documentation + +- All SDK public APIs **MUST** have documentation +- All public methods in SDK public APIs **MUST** have documentation except: + - Methods that implement or override a method in an interface + - Methods that override a method in a superclass +- For SDK protected APIs and internal APIs, documentation is recommended but not required +- Javadocs for SDK public APIs in high-level libraries (e.g., DynamoDB Enhanced Client) **SHOULD** include code snippets + +## Style Guidelines + +### General Formatting + +- Javadoc **MUST** be properly formatted following the [Javadoc standard](https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html) +- A single `

` **MUST** be used at the beginning of paragraphs (except the first paragraph), with no closing tag, as shown in this example: + + ```java + /** + * First paragraph with no

tag. + * + *

Second paragraph starts with a

tag. + * + *

Third paragraph also starts with a

tag. + */ + ``` + +- First sentence should be a summary of the method/class purpose +- Use complete sentences with proper punctuation +- Use third person (e.g., "Returns the value" not "Return the value") + +### Javadoc Tags + +- Use `@param` to document all parameters with clear descriptions +- Use `@return` to document return values (except for void methods) +- Use `@throws` to specify exceptions that could be thrown from a method +- Use `@link` and `@see` tags to point to related methods/classes +- Use `@deprecated` tag for deprecated methods, including: + - When the method was deprecated + - Why it was deprecated + - The replacement method, linked using `{@link}` +- Avoid using `@version` and `@since` tags + +### Code Snippets + +- Use `@snippet` to add code snippets +- [External code snippets](https://docs.oracle.com/en/java/javase/18/code-snippet/index.html#external-snippets) **SHOULD** be favored over inline code snippets +- Code snippets should be: + - Concise and focused on demonstrating the API + - Compilable and correct + - Well-commented to explain key points + - Following the same code style as the rest of the codebase + +## Examples + +### Class Documentation Example + +```java +/** + * A high-level library for uploading and downloading objects to and from Amazon S3. + * This can be created using the static {@link #builder()} method. + * + *

S3TransferManager provides a simplified API for efficient transfers between a local environment + * and S3. It handles multipart uploads/downloads, concurrent transfers, progress tracking, and + * automatic retries. + * + *

See {@link S3TransferManagerBuilder} for information on configuring an S3TransferManager. + * + *

Example usage: + * {@snippet : + * S3TransferManager transferManager = S3TransferManager.builder() + * .s3ClientConfiguration(b -> b.credentialsProvider(credentialsProvider) + * .region(Region.US_WEST_2)) + * .build(); + * + * // Upload a file + * UploadFileRequest uploadRequest = UploadFileRequest.builder() + * .putObjectRequest(req -> req.bucket("bucket").key("key")) + * .source(Paths.get("file.txt")) + * .build(); + * + * FileUpload upload = transferManager.uploadFile(uploadRequest); + * CompletedFileUpload uploadResult = upload.completionFuture().join(); + * } + * + * @see S3TransferManagerBuilder + */ +@SdkPublicApi +public interface S3TransferManager extends SdkAutoCloseable { + // ... +} +``` + +### Method Documentation Example + +```java +/** + * Uploads a file from a specified path to an S3 bucket. + * + *

This method handles large files efficiently by using multipart uploads when appropriate. + * Progress can be tracked through the returned {@link FileUpload} object. + * + *

Example: + * {@snippet : + * UploadFileRequest request = UploadFileRequest.builder() + * .putObjectRequest(r -> r.bucket("bucket-name").key("key")) + * .source(Paths.get("my-file.txt")) + * .build(); + * + * FileUpload upload = transferManager.uploadFile(request); + * CompletedFileUpload completedUpload = upload.completionFuture().join(); + * } + * + * @param request Object containing the bucket, key, and file path for the upload + * @return A {@link FileUpload} object to track the upload and access the result + * @throws S3Exception If any errors occur during the S3 operation + * @throws SdkClientException If any client-side errors occur + * @throws IOException If the file cannot be read + */ +FileUpload uploadFile(UploadFileRequest request); +``` + +### Deprecated Method Example + +```java +/** + * Returns the value of the specified header. + * + *

This method provides direct access to the header value. + * + * @param name The name of the header + * @return The value of the specified header + * @deprecated Use {@link #firstMatchingHeader(String)} instead, as it properly handles + * headers with multiple values. + */ +@Deprecated +String header(String name); +``` \ No newline at end of file diff --git a/docs/guidelines/logging-guidelines.md b/docs/guidelines/logging-guidelines.md new file mode 100644 index 000000000000..47d9f18227a0 --- /dev/null +++ b/docs/guidelines/logging-guidelines.md @@ -0,0 +1,205 @@ +# Logging Guidelines for AWS SDK for Java v2 + +## Table of Contents +- [General Principles](#general-principles) +- [Log Levels](#log-levels) +- [Structured Logging](#structured-logging) +- [Sensitive Data Handling](#sensitive-data-handling) +- [Error Logging](#error-logging) +- [Testing Logging](#testing-logging) + +## General Principles + +### Core Logging Standards +- **Use SDK Logger**: Always use `software.amazon.awssdk.utils.Logger`, never use SLF4J or other logging frameworks directly +- **Meaningful messages**: Write clear, actionable log messages that help with debugging +- **Include context**: Add relevant context like request IDs or operation names +- **Be consistent**: Use consistent formatting and terminology across the codebase +- **Consider the audience**: Write logs for the person who will debug the issue (often not you) + - Include enough context so someone unfamiliar with the code can understand what happened + - Avoid internal jargon or abbreviations that only the original developer would understand + - Include business context, not just technical details + - Think about what information would help during a production incident at 3 AM + +### Logger Declaration +- **Static final loggers**: Declare loggers as `private static final` +- **Class-based loggers**: Use the class for logger creation + +```java +// Good: Proper logger declaration +private static final Logger logger = Logger.loggerFor(MyClass.class); +``` + +## Log Levels + +### Level Guidelines +Use log levels consistently across the SDK: + +**Important**: Do not abuse WARN and ERROR levels. Only use WARN and ERROR for issues that are NOT surfaced to users through exceptions. If an exception will be thrown to the caller, do not also log it as WARN or ERROR - this creates duplicate error reporting and log noise. The exception itself is the error reporting mechanism for the user. When WARN or ERROR conditions affect every request (e.g., deprecated configuration, missing optional features, or suboptimal settings), use a one-time logging pattern to avoid flooding logs with repetitive messages. Log once per JVM startup rather than per request. + +**ERROR**: System errors, exceptions that prevent operation completion AND are not thrown to the caller +- Internal system or callback failures that don't result in thrown exceptions +- Background process failures +- Resource cleanup failures + +```java +// ERROR: Critical failures that don't throw exceptions to caller +logger.error(() -> "SdkAsyncHttpResponseHandler " + responseHandler + " threw an exception in onError. It will be ignored.", e); +logger.error(() -> "Connection pool health check failed [pool=" + poolName + "]", exception); + +// BAD: Don't log ERROR if you're throwing the exception to the caller +public void validateInput(String input) { + if (input == null) { + logger.error(() -> "Input validation failed: null input"); // DON'T DO THIS + throw new IllegalArgumentException("Input cannot be null"); // Exception already tells the user + } +} +``` + +**WARN**: Recoverable issues, deprecated usage, performance concerns that do NOT result in exceptions thrown to caller +- Successful fallback operations (user doesn't see the failure) +- Deprecated API usage (operation still succeeds) +- Performance degradation (operation completes but slowly) +- Configuration issues with successful fallbacks + +```java +logger.warn(() -> "Primary endpoint failed, falling back to secondary [primary=" + + primaryEndpoint + ", secondary=" + secondaryEndpoint + "]"); +logger.warn(() -> "SSL Certificate verification is disabled. This is not a safe setting and should only be used for testing."); + +// BAD: Don't log WARN if the failure will be thrown to caller +public Response callService() { + try { + return primaryService.call(); + } catch (ServiceException e) { + logger.warn(() -> "Service call failed", e); // DON'T DO THIS + throw e; // Exception already informs the caller + } +} + +// GOOD: Log WARN only when you handle the error internally +public Response callServiceWithFallback() { + try { + return primaryService.call(); + } catch (ServiceException e) { + logger.warn(() -> "Primary service failed, using fallback [error=" + e.getMessage() + "]"); + return fallbackService.call(); // User gets successful response + } +} +``` + +**INFO**: Do not use INFO level logging in the AWS SDK for Java v2. + +Use DEBUG level instead for operational information that might be useful for troubleshooting: + +**DEBUG**: Operational information, detailed execution flow, parameter values +- Service startup/shutdown +- Major operation completion +- Configuration changes +- Important state transitions +- Method entry/exit with parameters +- Intermediate processing steps +- Configuration details +- Performance metrics + +```java +// DEBUG: Operational and detailed execution information +logger.debug(() -> "Retry attempt [attemptNumber=" + attemptNumber + + ", delay=" + delayMs + "ms]"); +``` + +**TRACE**: Very detailed execution, typically for troubleshooting +- Detailed state information +- Fine-grained timing information + +```java + logger.debug(() -> "Interceptor '" + interceptor + "' modified the message with its " + methodName + " method."); + // TRACE: detailed execution information +logger.trace(() -> "Old: " + originalMessage + "\nNew: " + newMessage); +``` +## Structured Logging + +### Consistent Format +Use consistent key-value formatting for structured data when applicable: + +```java +// Good: Consistent structured format +logger.debug(() -> "Operation completed [operation=" + operationName + + ", duration=" + duration + "ms, status=" + status + + ", itemCount=" + itemCount + "]"); +``` + +## Sensitive Data Handling + +Never log sensitive information. + +**Never Log:** +- HTTP header values +- Authentication credentials +- Members with senstive trait + +**Safe to Log:** +- Request IDs, correlation IDs +- Operation names, status codes +- Timing and performance metrics + +### Error Context +Provide sufficient context for debugging: + +```java +// Good: Rich error context +s3AsyncClient.abortMultipartUpload(request) + .exceptionally(throwable -> { + logger.warn(() -> String.format("Failed to abort previous multipart upload. You may need to call S3AsyncClient#abortMultiPartUpload to " + + "free all storage consumed by all parts. [id=%s]", uploadId), throwable); + return null; + }); +``` + +## Testing Logging + +### Using LogCaptor +The AWS SDK provides `software.amazon.awssdk.testutils.LogCaptor` for testing log output: + +- **Create LogCaptor**: Use `LogCaptor.create(ClassName.class)` to capture logs for a specific class +- **Set log level**: Use `logCaptor.setLevel(Level.DEBUG)` to capture logs at specific levels +- **Access events**: Use `logCaptor.loggedEvents()` to get captured log events +- **Clean up**: Always call `logCaptor.close()` in `@AfterEach` to prevent memory leaks + +### Test Log Levels +Test important log messages in unit tests and verify appropriate log levels are used using `LogCaptor`: + +```java +@Test +void processRequest_withFallback_logsAtWarnLevel() { + Request request = createRequestThatTriggersFailover(); + + Response response = service.processRequestWithFallback(request); + + assertThat(response).isNotNull(); // Operation succeeded via fallback + + List warnEvents = logCaptor.loggedEvents().stream() + .filter(event -> event.getLevel() == Level.WARN) + .collect(Collectors.toList()); + + assertThat(warnEvents).hasSize(1); + assertThat(warnEvents.get(0).getMessage()) + .contains("Primary service failed, using fallback"); +} + +@Test +void processRequest_withValidation_logsAtDebugLevel() { + logCaptor.setLevel(Level.DEBUG); + Request request = createInvalidRequest(); + + assertThatThrownBy(() -> service.processRequest(request)) + .isInstanceOf(ValidationException.class); + + List debugEvents = logCaptor.loggedEvents().stream() + .filter(event -> event.getLevel() == Level.DEBUG) + .collect(Collectors.toList()); + + assertThat(debugEvents).isNotEmpty(); + assertThat(debugEvents.get(0).getMessage()).contains("Input validation failed"); +} +``` \ No newline at end of file diff --git a/docs/guidelines/reactive-streams-guidelines.md b/docs/guidelines/reactive-streams-guidelines.md new file mode 100644 index 000000000000..a6ec8a8aa14e --- /dev/null +++ b/docs/guidelines/reactive-streams-guidelines.md @@ -0,0 +1,40 @@ +# Reactvie Streams Guidelines + +The AWS SDK for Java v2 uses Reactive Streams for asynchronous, non-blocking data processing with backpressure support. All implementations must adhere to the following requirements to ensure proper functionality and compatibility. + +## Implementation Guidelines + +### Compliance Requirements + +- All implementations **MUST** fully comply with the [Reactive Streams Specification](https://github.com/reactive-streams/reactive-streams-jvm) +- All implementations **MUST** pass the [Reactive Streams Technology Compatibility Kit (TCK)](https://github.com/reactive-streams/reactive-streams-jvm/tree/master/tck) tests +- Any code changes to Reactive Streams implementations **MUST** include TCK verification tests + +### Best Practices +- Developers **SHOULD NOT** implement new Publisher or Subscriber interfaces from scratch +- Developers **SHOULD** utilize existing utility classes such as: + - `SimplePublisher` - A simple implementation of the Publisher interface + - `ByteBufferStoringSubscriber` - A subscriber that stores received ByteBuffers + - Methods in `SdkPublisher` - Utility methods for common Publisher operations + +## Common Patterns + +- Use `SdkPublisher` for SDK-specific publisher implementations +- Implement proper resource cleanup in both success and failure scenarios +- Handle cancellation gracefully, including cleanup of any allocated resources +- Ensure thread safety in all implementations +- Document thread safety characteristics and any assumptions about execution context + +## Testing Requirements + +- All Reactive Streams implementations **MUST** include TCK verification tests +- Tests **SHOULD** cover both normal operation and edge cases: + - Cancellation during active streaming + - Error propagation + - Backpressure handling under various request scenarios + - Resource cleanup in all termination scenarios + +## References + +- [Reactive Streams Specification](https://github.com/reactive-streams/reactive-streams-jvm) +- [Reactive Streams TCK](https://github.com/reactive-streams/reactive-streams-jvm/tree/master/tck) \ No newline at end of file diff --git a/docs/guidelines/testing-guidelines.md b/docs/guidelines/testing-guidelines.md new file mode 100644 index 000000000000..6f17414c6b7b --- /dev/null +++ b/docs/guidelines/testing-guidelines.md @@ -0,0 +1,170 @@ +# Testing Guidelines for AWS SDK v2 + +## General Guidelines + +- **Prefer JUnit 5** for new test classes +- **Prefer AssertJ** over Hamcrest for assertions +- Create dedicated test method for each test scenario +- Prefer parameterized tests for testing multiple similar cases +- When testing a module with dependencies, creating a constructor for dependency injection is preferred over exposing getter/setter APIs just for testing +- Test-only methods/constructors **MUST NOT** be public +- While test coverage is not a strict requirement, we **SHOULD** aim to cover 80% of the code + +## Test Naming Conventions + +Test names **SHOULD** follow `methodToTest_when_expectedBehavior` pattern: + +- Example: `close_withCustomExecutor_shouldNotCloseCustomExecutor` +- Example: `uploadDirectory_withDelimiter_filesSentCorrectly` + +This naming convention makes it clear: +1. What method is being tested +2. Under what conditions it's being tested +3. What behavior is expected + +## Unit Testing Best Practices + +- Each test should focus on a single behavior or aspect +- Use descriptive test names that explain the test's purpose +- Arrange-Act-Assert (AAA) pattern: + - Arrange: Set up the test data and conditions + - Act: Perform the action being tested + - Assert: Verify the expected outcome +- Use appropriate assertions for the specific condition being tested +- Avoid test interdependencies - tests should be able to run in any order +- Clean up resources in @After or @AfterEach methods +- When testing error path, verify part of the error message and the exception type +- **Avoid unnecessary inline comments**: Do not add inline comments like `// Arrange`, `// Act`, `// Assert` unless they provide meaningful context beyond the obvious test structure. Well-named test methods and clear code should be self-documenting + +## Mocking Guidelines + +- Mock external dependencies, not the class under test +- Only mock what you need to control for the test +- Prefer constructor injection to make dependencies mockable +- Use Mockito's verify() to ensure interactions with mocks are as expected +- Avoid excessive stubbing - it can make tests brittle and hard to maintain +- [Wiremock](https://wiremock.org/) is commonly used to mock server responses in functional tests + +## Testing Asynchronous Code + +- Use appropriate mechanisms to wait for async operations to complete +- Set reasonable timeouts to prevent tests from hanging +- For CompletableFuture: + - Use `join()` or `get()` with timeout in tests to wait for completion + - Test both successful completion and exceptional completion paths +- For Reactive Streams: + - Test backpressure handling + - Test cancellation scenarios + - Test error propagation + - For Publisher/Subscriber implementations, use TCK tests (see [Reactive Streams TCK Tests](#reactive-streams-tck-tests) section) + +## Test Suites + +### Unit Tests + +Unit tests verify the behaviors of an individual unit of source code. + +- **Goal**: Verify behaviors of a specific component and catch regressions +- **Location**: Under `src/test` directory in each module +- **When to add**: New unit tests **SHOULD** be added for any new changes +- **Naming Convention**: ClassNameTest, MethodNameTest +- **Test Automation**: Run for every PR and before release +- **Example**: [S3TransferManagerTest](https://github.com/aws/aws-sdk-java-v2/blob/master/services-custom/s3-transfer-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/S3TransferManagerTest.java) + +### Functional Tests + +Functional tests verify end-to-end behaviors and functionalities of an SDK feature. + +- **Goal**: Verify end-to-end behaviors of an SDK feature +- **Location**: + - Generated SDK common functionalities: In [codegen-generated-classes-test](https://github.com/aws/aws-sdk-java-v2/tree/master/test/codegen-generated-classes-test) module and [protocol-test](https://github.com/aws/aws-sdk-java-v2/tree/master/test/protocol-tests) module + - Service-specific functionalities: Under `src/test` directory in that service module + - High-level libraries (HLL): Under `src/test` directory in that HLL module +- **When to add**: Functional tests **SHOULD** be added for new SDK features or critical bug fixes +- **Naming Convention**: BehaviorTest, OperationTest +- **Test Automation**: Run for every PR and before release +- **Examples**: [WaitersSyncFunctionalTest](https://github.com/aws/aws-sdk-java-v2/blob/2532ec4f8ab36bab545689a2406d6a61d1696650/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/waiters/WaitersSyncFunctionalTest.java), [SelectObjectContentTest](https://github.com/aws/aws-sdk-java-v2/blob/master/services/s3/src/test/java/software/amazon/awssdk/services/s3/functionaltests/SelectObjectContentTest.java) + +### Reactive Streams TCK Tests + +[Reactive Streams TCK tests](https://github.com/reactive-streams/reactive-streams-jvm/tree/master/tck) verify Reactive Streams implementations against the rules defined in [the Reactive Streams Specification](https://github.com/reactive-streams/reactive-streams-jvm). + +- **Goal**: Ensure implementations are compliant with the spec +- **Location**: Under `src/test` directory in each module. The tests always have `TckTest` suffix +- **When to add**: New reactive streams TCK tests **MUST** be added when a new Subscriber/Publisher implementation is added +- **Naming Convention**: ClassNameTckTest +- **Test Automation**: Run for every PR and before release +- **Example**: [FileAsyncRequestPublisherTckTest](https://github.com/aws/aws-sdk-java-v2/blob/master/core/sdk-core/src/test/java/software/amazon/awssdk/core/async/FileAsyncRequestPublisherTckTest.java) + +### Integration Tests + +Integration tests verify end-to-end behaviors of the SDK with real AWS services. + +- **Goal**: Verify end-to-end behaviors of an SDK feature with real services +- **Location**: Under `src/it` directory in each module. The tests always have `IntegrationTest` suffix +- **When to add**: Integration tests **MAY** be added for new SDK features (functional tests are preferred) +- **Naming Convention**: ClassNameIntegrationTest, OperationIntegrationTest, BehaviorIntegrationTest +- **Important Notes**: Resources **MUST** be cleaned up after each test +- **Test Automation**: Run before release +- **Example**: [S3TransferManagerDownloadDirectoryIntegrationTest](https://github.com/aws/aws-sdk-java-v2/blob/master/services-custom/s3-transfer-manager/src/it/java/software/amazon/awssdk/transfer/s3/S3TransferManagerDownloadDirectoryIntegrationTest.java) + +### Stability Tests + +[Stability regression tests](https://github.com/aws/aws-sdk-java-v2/tree/master/test/stability-tests) detect stability regressions by sending high concurrent requests to AWS services. + +- **Goal**: Detect SDK errors in high-concurrency environment +- **Location**: Under `src/it` directory in `stability-tests` module. The tests always have `StabilityTest` suffix +- **When to add**: Stability tests **SHOULD** be added when a critical feature is being developed such as an HTTP Client +- **Naming Convention**: ClassNameStabilityTest +- **Test Automation**: Run before release +- **Example**: [KinesisStabilityTest](https://github.com/aws/aws-sdk-java-v2/blob/master/test/stability-tests/src/it/java/software/amazon/awssdk/stability/tests/kinesis/KinesisStabilityTest.java) + +### Long Running Canaries + +Long-running canaries continuously send requests to real or mock services at a constant rate, collecting resource usage, error rate, and latency metrics. + +- **Goal**: Detect resource leaks, latency increases, and performance issues +- **Location**: In a separate internal repository +- **When to add**: Canaries **SHOULD** be added when a critical feature is being developed such as an HTTP Client +- **Naming Convention**: FeatureTxnCreator +- **Test Automation**: Always running, deployed weekly + +### Performance Tests + +Performance tests are built using [Java Microbenchmark Harness (JMH)](https://github.com/openjdk/jmh) and measure throughput and latency. + +- **Goal**: Detect performance regression +- **Location**: In [sdk-benchmarks](https://github.com/aws/aws-sdk-java-v2/tree/master/test/sdk-benchmarks) module +- **When to add**: Benchmark code **SHOULD** be added for critical features or when verifying SDK performance impact +- **Naming Convention**: FeatureBenchmark +- **Test Automation**: No automation, manually triggered +- **Example**: [ApacheHttpClientBenchmark](https://github.com/aws/aws-sdk-java-v2/blob/master/test/sdk-benchmarks/src/main/java/software/amazon/awssdk/benchmark/apicall/httpclient/sync/ApacheHttpClientBenchmark.java) + +### Protocol Tests + +Protocol tests verify SDK behavior with different [protocols](https://smithy.io/2.0/aws/protocols/index.html) including rest-json, rest-xml, json, xml, and query protocols. + +- **Goal**: Verify marshalling/unmarshalling with different protocols +- **Location**: In [protocol-tests](https://github.com/aws/aws-sdk-java-v2/tree/master/test/protocol-tests) module and [protocol-tests-core](https://github.com/aws/aws-sdk-java-v2/tree/master/test/protocol-tests-core) module +- **When to add**: Protocol tests **MUST** be added if we are adding support for a new structure +- **Naming Convention**: XmlProtocolTest +- **Test Automation**: Run for every PR and before release +- **Example**: [RestJsonProtocolTest](https://github.com/aws/aws-sdk-java-v2/blob/master/test/protocol-tests/src/test/java/software/amazon/awssdk/protocol/tests/RestJsonProtocolTest.java) + +## Test Coverage + +- Aim for high test coverage, especially for critical paths +- Don't focus solely on line coverage - consider branch and path coverage +- Identify and test edge cases and boundary conditions +- Test error handling and exception paths +- While test coverage is not a strict requirement, we **SHOULD** aim to cover 80% of the code + +## References + +- [JUnit 5 User Guide](https://junit.org/junit5/docs/current/user-guide/) +- [AssertJ Documentation](https://assertj.github.io/doc/) +- [Mockito Documentation](https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html) +- [Wiremock Documentation](https://wiremock.org/) +- [AWS SDK for Java Developer Guide - Testing](https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/testing.html) +- [Reactive Streams Specification](https://github.com/reactive-streams/reactive-streams-jvm) +- [Reactive Streams TCK](https://github.com/reactive-streams/reactive-streams-jvm/tree/master) \ No newline at end of file