-
Notifications
You must be signed in to change notification settings - Fork 239
Add guidance for implementing HTTP interfaces in a Smithy client #2868
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "documentation", | ||
| "description": "Added a section to discuss recommendations for implementing Smithy clients. Currently this section only includes information about HTTP interfaces, but it will expand over time to cover more topics related to implementing clients.", | ||
| "pull_requests": [ | ||
| "[#2868](https://github.com/smithy-lang/smithy/pull/2868)" | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,189 @@ | ||
| # HTTP | ||
|
|
||
| HTTP is the most common application protocol used by Smithy clients. This guide | ||
| provides advice on how to integrate and expose HTTP clients. | ||
|
|
||
| ## Configuration | ||
|
|
||
| Smithy clients should allow their users to configure the HTTP client that the | ||
| Smithy client uses to send requests. Users may want to change the client used to | ||
| something that has different performance characteristics or support for features | ||
| that they need. | ||
|
|
||
| ## HTTP interfaces | ||
|
|
||
| Smithy clients should provide interfaces for HTTP clients that standardize how | ||
| the Smithy client interacts with the HTTP client, allowing any HTTP client to be | ||
| used as long as it implements the interface. | ||
|
|
||
| ### Clients | ||
|
|
||
| It is recommended to make HTTP clients implementations of | ||
| [`ClientTransport`](#transport-clients). | ||
|
|
||
| ```java | ||
| public interface HttpClient implements ClientTransport<HttpRequest, HttpResponse> { | ||
| HttpResponse send(Context context, HttpRequest request); | ||
| } | ||
| ``` | ||
|
|
||
| #### Context | ||
|
|
||
| HTTP clients don't have many common context parameters, but they should check | ||
| the context for a request timeout setting and use it if it's present. | ||
|
|
||
| ```java | ||
| /** | ||
| * This utility class holds shared context key definitions that are useful | ||
| * for HTTP implementations. | ||
| */ | ||
| public final class HttpContext { | ||
| public static final Context.Key<Duration> HTTP_REQUEST_TIMEOUT = Context.key("HTTP.RequestTimeout"); | ||
|
|
||
| // This is a utility class that is not intended to be constructed, so it | ||
| // has a private constructor. | ||
| private HttpContext() {} | ||
| } | ||
| ``` | ||
|
|
||
| ### Requests and Responses | ||
|
|
||
| {rfc}`9110` discusses HTTP requests and responses collectively as "messages", | ||
| and it can be useful to encode their shared features in a shared interface. | ||
|
|
||
| ```java | ||
| public interface HttpMessage { | ||
| /** | ||
| * Get the headers of the message. | ||
| * | ||
| * @return headers. | ||
| */ | ||
| HttpFields headers(); | ||
|
|
||
| /** | ||
| * Get the body of the message, or null. | ||
| * | ||
| * @return the message body or null. | ||
| */ | ||
| DataStream body(); | ||
| } | ||
| ``` | ||
|
|
||
| Requests introduce the `method` and `uri` properties. | ||
|
|
||
| ```java | ||
| public interface HttpRequest extends HttpMessage { | ||
| /** | ||
| * Get the method of the request. | ||
| * | ||
| * @return the method. | ||
| */ | ||
| String method(); | ||
|
|
||
| /** | ||
| * Get the URI of the request. | ||
| * | ||
| * @return the request URI. | ||
| */ | ||
| URI uri(); | ||
| } | ||
| ``` | ||
|
|
||
| Responses introduce a status code. | ||
|
|
||
| ```java | ||
| public interface HttpResponse extends HttpMessage { | ||
| /** | ||
| * Get the status code of the response. | ||
| * | ||
| * @return the status code. | ||
| */ | ||
| int statusCode(); | ||
| } | ||
| ``` | ||
|
|
||
| ### Fields | ||
|
|
||
| Most users who have interacted with HTTP directly are familiar with the concept | ||
| of headers. Headers were originally introduced in HTTP/1.0 and, since then, the | ||
| concept of key/value pairs has expanded to include trailers and other arbitrary | ||
| metadata. As of {rfc}`9110`, these key/value pairs are exclusively referred to | ||
| as {rfc}`fields <9110#section-5>`. | ||
JordonPhillips marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| When designing HTTP interfaces for Smithy clients, be careful to understand | ||
| field semantics. In particular, it is important to understand that field keys | ||
| are case-insensitive and may appear more than once in an HTTP message. Since | ||
| field keys may appear more than once, it is recommended that they are | ||
| represented as an iterable collection of pairs or as a map whose value type is a | ||
| list. This allows protocol implementations to safely handle joining and | ||
| splitting. | ||
|
|
||
| It is recommended to have utilities to convert fields to and from maps. Fields | ||
| are often conceptualized as maps, so providing these utilities allows users to | ||
| access fields in a more comfortable way without sacrificing correctness. | ||
|
|
||
| ```java | ||
| public interface HttpFields extends Iterable<Map.Entry<String, List<String>>> { | ||
| /** | ||
| * Create an HttpFields instance from a map. | ||
| * | ||
| * @param fields Field map to use as a data source. | ||
| * @return the created fields. | ||
| */ | ||
| static HttpFields of(Map<String, List<String>> fields) { | ||
| // This constructs a theoretical default implementation of the | ||
| // HttpFields interface that creates an unmodifiable copy of the given | ||
| // map. | ||
| return fields.isEmpty() ? UnmodifiableHttpFields.EMPTY : new UnmodifiableHttpFields(fields); | ||
| } | ||
|
Comment on lines
+125
to
+138
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Disclaimer: I'm going to pick on this interface a bit since it's our first one and it's a good strawman but I'm mostly wanting to establish precedent and principles for how we approach all interfaces present and future. This interface seems a little overengineered to me, at least for an introductory read. It makes several choices that seem to have future uses in mind without first laying out why they're necessary, useful, etc. For instance:
I'm not necessarily opposed to any of these on principle, but I don't know that I would've chosen to include them if I wrote this. That said, I do appreciate the well-written doc comments for each method, parameter, and return value. 😄 We should add docs to the interface as well!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is covered in the leading paragraphs. HTTP fields aren't maps, they're iterable pairs.
Despite the fact that fields aren't maps, people still conceptualize them this way. If you don't offer safe conversions, people will do it themselves and might do it wrong. For example, they may do a naive |
||
|
|
||
| /** | ||
| * Convert the HttpFields to a map. | ||
| * | ||
| * @return the fields as a map. | ||
| */ | ||
| Map<String, List<String>> toMap(); | ||
|
|
||
| /** | ||
| * Check if the given field is case-insensitively present. | ||
| * | ||
| * @param name Name of the field to check. | ||
| * @return true if the field is present. | ||
| */ | ||
| default boolean containsField(String name) { | ||
| return !getAllValues(name).isEmpty(); | ||
| } | ||
|
|
||
| /** | ||
| * Get the first field value of a specific field by case-insensitive name. | ||
| * | ||
| * Smithy clients know whether a given field should have a single value or | ||
| * a list value. This helper method simplifies usage for fields with a | ||
| * single value. | ||
| * | ||
| * @param name Name of the field to get. | ||
| * @return the matching field value, or null if not found. | ||
| */ | ||
| default String getFirstValue(String name) { | ||
| var list = getAllValues(name); | ||
| return list.isEmpty() ? null : list.get(0); | ||
| } | ||
|
|
||
| /** | ||
| * Get the values of a specific field by case-insensitive name. | ||
| * | ||
| * @param name Name of the field to get the values of. | ||
| * @return the values of the field, or an empty list. | ||
| */ | ||
| List<String> getAllValues(String name); | ||
| } | ||
| ``` | ||
|
|
||
| #### Implementation recommendations | ||
|
|
||
| It is not recommended to automatically attempt to join values for a given field | ||
| key at the HTTP layer. {rfc}`9110#section-5` allows field values to be joined | ||
| with a comma, but doing so automatically can introduce data corruption if one of | ||
| the field values already includes a comma. {rfc}`Section 5.6 <9110#section-5.6>` | ||
| includes productions that can help to handle those edge cases, but whether they | ||
| are used or not is up to the protocol definition. | ||
|
Comment on lines
+182
to
+189
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What should implementors do instead? Are we implying that all HTTP client implementations will be able to accept lists of header values (vs merely a single value)?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. HTTP implementations should do nothing. This is a serialization decision that is up to the protocol implementation. An HTTP client that can't take a list of values for a header is not compliant with the relevant RFCs. How they represent that might vary. It's up to the interface implementation to translate into whatever the underlying client uses. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| # Application Protocols | ||
|
|
||
| This section provides guidance on how to implement and integrate different | ||
| application protocols into a client. | ||
|
|
||
| Application protocols define how operations are transmitted over a network. The | ||
| most commonly used application protocol by Smithy clients is HTTP, but other | ||
| protocols like MQTT may also be used by Smithy clients and servers. When | ||
| designing a client, be careful to not couple any components to a particular | ||
| application protocol unless they interact explicitly with that protocol. For | ||
| example, an HTTP request serializer inherently needs to be coupled to HTTP, but | ||
| a JSON serializer does not. | ||
|
|
||
| (transport-clients)= | ||
JordonPhillips marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ## Transport clients | ||
|
|
||
| Smithy clients and services have a common access pattern regardless of what | ||
| application protocol is being used: a client sends requests to a server and | ||
| receives responses. This can be represented by a simple interface: | ||
|
|
||
| ```java | ||
| public interface ClientTransport<RequestT, ResponseT> { | ||
| ResponseT send(Context context, RequestT request); | ||
| } | ||
| ``` | ||
|
Comment on lines
+21
to
+25
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't most client transport implementations have resources which live outside of request/response scope (e.g., a thread pool, a task queue, etc.)? Should we capture that in the interface by extending
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think resource management is off topic. We might add topics about that later, but there's a lot that goes into it and a lot of it is going to be language specific. As a Java interface I still think it isn't necessary since the underlying implementation should be free to handle resource management however it likes. |
||
|
|
||
| In addition to the request, it is recommended to introduce a context parameter | ||
| to the `send` method to allow the client to be configured for each request. It | ||
| is recommended to make this a generic context object rather than a type with | ||
| fixed properties. Leaving it unrestricted allows context to be passed into | ||
| custom `ClientTransport` implementations that may not be relevant to other | ||
| implementations. | ||
|
|
||
| ## Navigation | ||
|
|
||
| ```{toctree} | ||
| :maxdepth: 1 | ||
|
|
||
| http | ||
| ``` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| # Smithy Client Guidance | ||
|
|
||
| This guide provides advice on how to build clients to interact with Smithy | ||
| services. In particular, it provides advice on how to design the generated | ||
| client and the components that make it up. | ||
|
|
||
| While topics in this guide may briefly discuss code generation, this guide | ||
| does not describe how to build a code generator. To learn how to build a code | ||
| generator, see | ||
| [Creating a Code Generator](project:../building-codegen/index.rst). | ||
|
|
||
| ## Goals of this guide | ||
|
|
||
| - Provide guidance on how to design client components. | ||
| - Provide guidance on how to avoid coupling components to particular transport | ||
| protocols or to specific features. | ||
| - Provide guidance on how to make clients extensible. | ||
|
|
||
| ## Non-goals of this guide | ||
|
|
||
| - Provide guidance on how to design and implement code generators. | ||
| - Force specific implementation details. This guide is non-normative, feel free | ||
| to deviate from its advice. | ||
|
|
||
| ## Tenets for Smithy clients | ||
|
|
||
| Smithy clients should follow these tenets: | ||
|
|
||
| 1. **Smithy implementations adhere to the spec**. The Smithy spec and model are | ||
| the contract between clients, servers, and other implementations. A Smithy | ||
| client written in any programming language should be able to connect to a | ||
| Smithy server written in any programming language without either having to | ||
| care about the programming language used to implement the other. | ||
| 2. **Smithy clients should be familiar to developers**. Language idioms and | ||
| developer experience factor into how developers and companies choose between | ||
| Smithy and alternatives. | ||
| 3. **Components are preferred over monoliths**. Components should be modular and | ||
| composable. They should have clear boundaries: a client that uses an AWS | ||
| protocol is not required to use AWS credentials, for example. | ||
| 4. **Smithy client code should prioritize maintainability by limiting public | ||
| interfaces**. Smithy clients should limit the dependencies they take on. They | ||
| shouldn't expose overly open interfaces that hinder the ability to evolve the | ||
| code base. | ||
| 5. **No implementation stands alone**. Test cases, protocol tests, code fixes, | ||
| and missing abstractions have a greater impact if every Smithy implementation | ||
| can use them rather than just a single implementation. | ||
| 6. **Service teams don't need to know the details of every client that exists or | ||
| will ever exist**. When modeling a service, service teams only need to | ||
| consider if the model is a valid Smithy model; the constraints of any | ||
| particular programming language should not be a concern when modeling a | ||
| service. Smithy is meant to work with any number of languages, and it is an | ||
| untenable task to attempt to bubble up every constraint, reserved word, or | ||
| other limitation to modelers. | ||
|
|
||
| ## Navigation | ||
|
|
||
| ```{toctree} | ||
| :maxdepth: 1 | ||
|
|
||
| application-protocols/index | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,4 +15,5 @@ Guides | |
| style-guide | ||
| model-translations/index | ||
| building-codegen/index | ||
| client-guidance/index | ||
| glossary | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: I think we should avoid proscriptive terms like "should" or "tenet". Instead, since this is guidance only, I think we should explore the reasons why certain features could be beneficial (you mostly already did that) and then "recommend" a solution.
For example:
## Configuration Smithy clients delegate to an HTTP client for low-level communication with remote web services. Certain users or use cases may benefit from replacing or customizing the HTTP client, for instance to achieve specific performance characteristics or to use advanced HTTP features. To facilitate these scenarios, we recommend making the HTTP client configurable in Smithy clients.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not agree that "should" is overly prescriptive. Should is a recommendation, not a requirement. Trying to ban it will lead to awkward phrasing.