Add guidance for implementing HTTP interfaces in a Smithy client #2868

JordonPhillips · 2025-12-01T17:30:31Z

This adds a section to the docs to discuss guidance and reccomendations for implementing Smithy clients. Currently this is only covers HTTP interfaces, but it is intended to be expanded broadly.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

.changes/next-release/documentation-b6f64a8421e29dc8eea3139ed59a6440f6fed83d.json

docs/source-2.0/guides/client-guidance/application-protocols/http.md

docs/source-2.0/guides/client-guidance/application-protocols/index.md

docs/source-2.0/guides/client-guidance/index.md

docs/source-2.0/guides/client-guidance/application-protocols/index.md

ianbotsf

This is merging into main? Do we want any more foundations in place before releasing this?

ianbotsf · 2025-12-03T19:10:07Z

docs/source-2.0/guides/client-guidance/application-protocols/http.md

+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.


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.

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.

docs/source-2.0/guides/client-guidance/application-protocols/http.md

ianbotsf · 2025-12-03T21:32:56Z

docs/source-2.0/guides/client-guidance/application-protocols/http.md

+```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);
+    }


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:

Why extend Iterable?

Why include a static factory method?

Why offer conversions to/from Map?

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!

Why extend Iterable

This is covered in the leading paragraphs. HTTP fields aren't maps, they're iterable pairs.

Why offer conversions to/from Map?

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 map.put(k, v) which would erase any previous values.

ianbotsf · 2025-12-03T22:04:41Z

docs/source-2.0/guides/client-guidance/application-protocols/http.md

+#### 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.


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)?

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.

ianbotsf · 2025-12-03T22:18:16Z

docs/source-2.0/guides/client-guidance/application-protocols/http.md

+### 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);
+}
+```


Is #transport-clients a valid link? That heading appears in index.md but not in this doc. (I'm a Sphinx noob so maybe this is just how Sphinx works?)

index.md contains the line (transport-clients)=, which creates an explicit target that can be referenced outside of its file.

ianbotsf · 2025-12-03T22:23:21Z

docs/source-2.0/guides/client-guidance/application-protocols/http.md

+```java
+public final class HttpContext {
+    public static final Context.Key<Duration> HTTP_REQUEST_TIMEOUT = Context.key("HTTP.RequestTimeout");
+
+    private HttpContext() {}
+}
+```


In general, how do we feel about access modifiers? I recognize that good library design requires careful attention to which declarations are public, internal, private, etc. but these interfaces are intended to be illustrative, not necessarily copy-paste solutions. I lean towards omitting any unnecessary syntax which isn't useful in conveying the intent of the abstraction, including removing public from basically everything.

I think we need to stick to the idioms and best-practices of the language. Access modifiers have meaning and having no access modifiers has meaning. While we shouldn't venture into concepts that are overly abstract without reason, access modifiers are a fundamental concept to Java.

We talked before about our target audiences for these guidelines. At a minimum, we're expecting people who read this to have a formal education in computer science or equivalent experience. I don't expect such a person to be a Java expert, but I do expect them to be able to learn enough Java to understand these examples.

For developers who are more experienced, I expect them to be familiar with the concept of access modifiers already, if not the minutiae of how they work in Java. I expect that they know enough about their language of expertise to know if and how that concept applies.

these interfaces are intended to be illustrative, not necessarily copy-paste solutions

While I do agree, I think the closer to copy-pasteable we are the better. For me, I think the lines are anything an IDE would collapse by default (i.e. imports) and most things that would require defining another class.

ianbotsf · 2025-12-03T22:27:05Z

docs/source-2.0/guides/client-guidance/application-protocols/index.md

+```java
+public interface ClientTransport<RequestT, ResponseT> {
+    ResponseT send(Context context, RequestT request);
+}
+```


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 AutoCloseable or adding an explicit close method?

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.

ianbotsf · 2025-12-03T22:29:03Z

docs/source-2.0/guides/client-guidance/application-protocols/http.md

+#### 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
+public final class HttpContext {
+    public static final Context.Key<Duration> HTTP_REQUEST_TIMEOUT = Context.key("HTTP.RequestTimeout");
+
+    private HttpContext() {}
+}
+```


We touch on the notion of context here without ever really defining what it is. I suspect the notion of context varies quite a bit across languages/frameworks so we'll likely need a dedicated topic on this.

Context is talked about more in the application protocols section. I agree that it needs more... context... but I think it's big enough a topic to be its own topic. I can add that to this PR if you like, otherwise I'll do it in a followup PR

A follow up PR sounds good.

ianbotsf · 2025-12-03T22:36:03Z

docs/source-2.0/guides/client-guidance/index.md

+## Tenets for Smithy clients
+
+These are the tenets of Smithy clients (unless you know better ones):


I like many of the ideas in this section but it reads too much like a list of requirements or mandates. I think we could rephrase this slightly (or more extensively in some cases) to be advice, desirable outcomes, etc.

Some of these we can be strict on. 1 and 6 in particular are critical. I've softened the language broadly and made some editorial changes

JordonPhillips requested a review from a team as a code owner December 1, 2025 17:30

JordonPhillips requested a review from kstich December 1, 2025 17:30

github-actions bot reviewed Dec 1, 2025

View reviewed changes

.changes/next-release/documentation-b6f64a8421e29dc8eea3139ed59a6440f6fed83d.json Show resolved Hide resolved

kstich requested changes Dec 1, 2025

View reviewed changes

JordonPhillips force-pushed the smithy-client-guide branch from 813db74 to 0abad2a Compare December 2, 2025 11:19

JordonPhillips requested a review from kstich December 2, 2025 11:19

ianbotsf reviewed Dec 3, 2025

View reviewed changes

JordonPhillips added 2 commits December 4, 2025 15:20

Add initial landing page for client guidance

e06e4e2

Add client guidance for HTTP interfaces

460cd2e

JordonPhillips force-pushed the smithy-client-guide branch from 0abad2a to 460cd2e Compare December 4, 2025 14:21

JordonPhillips requested a review from ianbotsf December 4, 2025 14:21

ianbotsf approved these changes Dec 9, 2025

View reviewed changes

		## Tenets for Smithy clients

		These are the tenets of Smithy clients (unless you know better ones):

Add guidance for implementing HTTP interfaces in a Smithy client #2868

Are you sure you want to change the base?

Add guidance for implementing HTTP interfaces in a Smithy client #2868

Uh oh!

Conversation

JordonPhillips commented Dec 1, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ianbotsf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants