Add FullHttpRequest and FullHttpResponse classes to Styx API #40

kvosper · 2017-11-27T10:21:02Z

These are some classes representing full (aggregated/decoded) HTTP messages.

alobodzki · 2017-11-28T11:11:17Z

I lookd at the change, looks good for me, Mikko?

mikkokar · 2017-11-27T11:49:14Z

components/api/src/main/java/com/hotels/styx/api/HttpResponse.java

+                .map(decoded -> new com.hotels.styx.api.messages.FullHttpResponse.Builder<>(this, decoded))
+                .map(com.hotels.styx.api.messages.FullHttpResponse.Builder::build);
+    }
+


I think we could now mark the old decode() method, and the accompanying DecodedRequest/DecodedResponse types as @Deprecated?

mikkokar · 2017-11-28T17:47:39Z

components/api/src/main/java/com/hotels/styx/api/HttpRequest.java

+     */
+    public <T> Observable<com.hotels.styx.api.messages.FullHttpRequest<T>> toFullHttpRequest(Function<ByteBuf, T> decoder, int maxContentBytes) {
+        return body.decode(decoder, maxContentBytes)
+                .map(decoded -> new com.hotels.styx.api.messages.FullHttpRequest.Builder<>(this, decoded))


FullHttpRequest could be statically imported. There are other usages also in this file.

mikkokar · 2017-11-28T17:55:11Z

components/api/src/main/java/com/hotels/styx/api/HttpResponse.java

+     * @param maxContentBytes maximum content bytes before an exception is thrown
+     * @return a decoded (aggregated/full) response
+     */
+    public Observable<com.hotels.styx.api.messages.FullHttpResponse<String>> toFullHttpResponse(int maxContentBytes) {


FullHttpResponse doesn't have to be fully qualified AFAICS.

mikkokar · 2017-11-28T18:05:10Z

components/api/src/main/java/com/hotels/styx/api/messages/FullHttpRequest.java

+     * @param <T> body type
+     * @return {@code this}
+     */
+    public static <T> Builder<T> get(String uri, T body) {


The GET and HEAD (below) methods are not intended for pushing content up to server. Therefore we should remove this overloaded variant and the another for head(String, T).

mikkokar · 2017-11-28T18:09:10Z

components/api/src/main/java/com/hotels/styx/api/messages/FullHttpRequest.java

+     * @param <T> body type
+     * @return {@code this}
+     */
+    public static <T> Builder<T> delete(String uri, T body) {


I would remove this method as well. See my comments for get(String, T) above.

mikkokar · 2017-11-28T18:16:00Z

components/api/src/main/java/com/hotels/styx/api/messages/FullHttpRequest.java

+     * @param request a request
+     * @return an encoded (streaming) request
+     */
+    public static com.hotels.styx.api.HttpRequest toStreamingHttpRequest(FullHttpRequest<String> request) {


Could this be statically imported?

mikkokar · 2017-11-28T18:17:44Z

components/api/src/main/java/com/hotels/styx/api/messages/FullHttpRequest.java

+            this.cookies = new ArrayList<>(request.cookies());
+        }
+
+        private Builder(FullHttpRequest<?> request, boolean doNotCopyBody) {


Unused constructor.

mikkokar · 2017-11-28T18:31:46Z

components/api/src/main/java/com/hotels/styx/api/messages/FullHttpRequest.java

+        }
+
+        public Builder<T> body(T content) {
+            setContentLength(content);


I would remove this for now. For the following reasons:

The API consumer may want to set Transfer-Encoding: Chunked instead.

For testing purposes, etc we may want to create HTTP messages which deliberately do not have content length set. In fact this is a perfectly valid use case for servers returning HttpResponses.

The content length depends on the kind of encodings applied to the body, and it should be in-line with the other headers applied such as content type charset etc. This method doesn't attempt to set those headers, and also I am not confident the length calculation works in all situations.

mikkokar · 2017-11-28T18:35:05Z

components/api/src/main/java/com/hotels/styx/api/messages/FullHttpRequest.java

+                ensureMethodIsValid();
+            }
+
+            setHostHeader();


This too will prevent some use cases where we deliberately might not want to have the host header present. Such as adapting wire-mock onto Styx server framework. Perhaps move into the validate block or something?

mikkokar · 2017-11-30T06:43:30Z

components/api/src/main/java/com/hotels/styx/api/HttpRequest.java

+     * @param maxContentBytes maximum content bytes before an exception is thrown
+     * @return a decoded (aggregated/full) request
+     */
+    public Observable<com.hotels.styx.api.messages.FullHttpRequest<String>> toFullHttpRequest(int maxContentBytes) {


While it is an important safety feature, it quickly becomes tedious having to specify maxContentBytes every time. Especially in the unit tests.

For this reason, please consider adding an overloaded version where the maxContentBytes have a sensible system-wide default. Ideally, the system-wide default can be modified via system properties.

This same comment applies to the HttpResponse too.

mikkokar · 2017-12-01T09:04:46Z

components/api/src/test/java/com/hotels/styx/api/messages/FullHttpResponseTest.java

+import static org.hamcrest.collection.IsIterableContainingInOrder.contains;
+
+public class FullHttpResponseTest {
+    @Test


We need to have a test to ensure that all headers, status & other response attributes are converted correctly to streaming type:

eg.

assertThat(streaming.status(), is(response.status())); assertThat(streaming.headers(), is(response.headers()); ...

mikkokar · 2017-12-01T09:09:36Z

components/api/src/test/java/com/hotels/styx/api/messages/FullHttpResponseTest.java

+
+        assertThat(subscriber.getOnNextEvents().size(), is(1));
+        ByteBuf buf = subscriber.getOnNextEvents().get(0);
+        assertThat(buf.toString(UTF_8), is("foobar"));


Also, it should be sufficient to use streaming.body().decode(...) to aggregate the content stream for assertions.

IMO the fact that it is streamed as a single content chunk should be an implementation detail, not part of the method contract (assertion in line 77). That is, we could at any time choose to change the toStreamingHttpResponse implementation to split it into several smaller chunks if necessary without affecting the API contract.

Perhaps this could be even clarified in the javadoc comments.

mikkokar · 2017-12-01T09:12:21Z

components/api/src/test/java/com/hotels/styx/api/messages/FullHttpResponseTest.java

+    }
+
+    @Test
+    public void setsASingleOutboundCookie() throws Exception {


IntelliJ says the the Exception is never thrown.

mikkokar · 2017-12-01T09:14:27Z

components/api/src/test/java/com/hotels/styx/api/messages/FullHttpRequestTest.java

+
+public class FullHttpRequestTest {
+    @Test
+    public void encodesToStreamingHttpRequest() throws Exception {


IntelliJ says the Exception is never thrown.

mikkokar · 2017-12-01T09:16:17Z

components/api/src/test/java/com/hotels/styx/api/messages/FullHttpRequestTest.java

+
+public class FullHttpRequestTest {
+    @Test
+    public void encodesToStreamingHttpRequest() throws Exception {


Here we need to have tests to demonstrate that all HTTP request attributes (URL, path, protocol, headers) are converted accordingly by the call to toStreamingHttpRequest(). Not just the request body.

mikkokar · 2017-12-01T09:17:56Z

components/api/src/test/java/com/hotels/styx/api/messages/FullHttpRequestTest.java

+
+public class FullHttpRequestTest {
+    @Test
+    public void encodesToStreamingHttpRequest() throws Exception {


It should be sufficient to use streaming.body().decode(...) for content assertions. See my comments for the FullHttpResponseTest.

Copying my comment from the FullHttpResponseTest:

IMO the fact that it is streamed as a single content chunk should be an implementation detail, not part of the method contract (assertion in line 74 below). That is, we could at any time choose to change the toStreamingHttpResponse implementation to split it into several smaller chunks if necessary without affecting the API contract.

mikkokar · 2017-12-01T09:34:25Z

components/api/src/main/java/com/hotels/styx/api/messages/FullHttpRequest.java

+         * @return {@code this}
+         */
+        public Builder<T> header(CharSequence name, Object value) {
+            checkNotCookie(name);


The checkNotCookie(name) should be present only when validation is set to true.

We need to come up with a better approach for representing cookies. Going forward, we should build the structured cookie API on top of textual HTTP headers API, making it complementary, instead of enforcing its usage.

This is important because enforcing structured cookie API restricts the applicability of the API (Think of the WireMock use case). Therefore we need to let API consumers to set/read cookies directly via headers.

For now, make the checkNotCookie() somehow optional, but leave any further API improvements to the next increment.

This comment also addHeader(CharSequence, Object) variant.

Hmm... looking at code. This alone is not sufficient. Let me think about this a bit.

kvosper added 2 commits November 27, 2017 10:17

Add new API classes and conversion methods

0455296

Make changes to static methods suggested in PR comments

45a04c3

Make class descriptions more descriptive

fd6b08f

mikkokar reviewed Nov 28, 2017

View reviewed changes

mikkokar suggested changes Nov 28, 2017

View reviewed changes

Tidy test

85d446f

mikkokar changed the title ~~Add new API classes and conversion methods~~ Add FullHttpRequest and FullHttpResponse classes to Styx API Nov 30, 2017

Changes based on PR comments

6279f3d

mikkokar suggested changes Dec 1, 2017

View reviewed changes

kvosper added 2 commits December 5, 2017 10:33

Changes requested in PR

1db54a1

Fix checkstyle violation

8bd0e27

mikkokar approved these changes Dec 5, 2017

View reviewed changes

mikkokar merged commit 72c5156 into ExpediaGroup:master Dec 5, 2017

kvosper deleted the add-full-http-messages branch August 24, 2018 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FullHttpRequest and FullHttpResponse classes to Styx API #40

Add FullHttpRequest and FullHttpResponse classes to Styx API #40

kvosper commented Nov 27, 2017

alobodzki commented Nov 28, 2017

mikkokar Nov 27, 2017

mikkokar Nov 28, 2017

mikkokar Nov 28, 2017

mikkokar Nov 28, 2017

mikkokar Nov 28, 2017

mikkokar Nov 28, 2017

mikkokar Nov 28, 2017

mikkokar Nov 28, 2017

mikkokar Nov 28, 2017

mikkokar Nov 30, 2017

mikkokar Dec 1, 2017

mikkokar Dec 1, 2017

mikkokar Dec 1, 2017

mikkokar Dec 1, 2017

mikkokar Dec 1, 2017

mikkokar Dec 1, 2017

mikkokar Dec 1, 2017

mikkokar Dec 1, 2017

mikkokar Dec 1, 2017

mikkokar Dec 1, 2017

Add FullHttpRequest and FullHttpResponse classes to Styx API #40

Add FullHttpRequest and FullHttpResponse classes to Styx API #40

Conversation

kvosper commented Nov 27, 2017

alobodzki commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment