Skip to content
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

Fix request-compression stream handling #822

Merged
merged 5 commits into from
Nov 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added docs/zipper.jpg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
7 changes: 6 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
<module>riptide-compatibility</module>
<module>riptide-core</module>
<module>riptide-chaos</module>
<module>riptide-compression</module>
<module>riptide-failsafe</module>
<module>riptide-faults</module>
<module>riptide-httpclient</module>
Expand Down Expand Up @@ -104,6 +105,11 @@
<artifactId>riptide-compatibility</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>org.zalando</groupId>
<artifactId>riptide-compression</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>org.zalando</groupId>
<artifactId>riptide-core</artifactId>
Expand Down Expand Up @@ -607,7 +613,6 @@
<configuration>
<compilerArgs>
<compilerArg>-Xlint:unchecked</compilerArg>
<compilerArg>-Werror</compilerArg>
<compilerArg>-parameters</compilerArg>
</compilerArgs>
</configuration>
Expand Down
81 changes: 81 additions & 0 deletions riptide-compression/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
# Riptide: Compression
lukasniemeier-zalando marked this conversation as resolved.
Show resolved Hide resolved

[![Zipper](../docs/zipper.jpg)](https://pixabay.com/photos/zipper-metal-gold-color-brass-201684/)

[![Build Status](https://img.shields.io/travis/zalando/riptide.svg)](https://travis-ci.org/zalando/riptide)
[![Coverage Status](https://img.shields.io/coveralls/zalando/riptide.svg)](https://coveralls.io/r/zalando/riptide)
[![Code Quality](https://img.shields.io/codacy/grade/1fbe3d16ca544c0c8589692632d114de/master.svg)](https://www.codacy.com/app/whiskeysierra/riptide)
[![Javadoc](https://www.javadoc.io/badge/org.zalando/riptide-compression.svg)](http://www.javadoc.io/doc/org.zalando/riptide-compression)
[![Release](https://img.shields.io/github/release/zalando/riptide.svg)](https://github.com/zalando/riptide/releases)
[![Maven Central](https://img.shields.io/maven-central/v/org.zalando/riptide-compression.svg)](https://maven-badges.herokuapp.com/maven-central/org.zalando/riptide-compression)
[![License](https://img.shields.io/badge/license-MIT-blue.svg)](https://raw.githubusercontent.com/zalando/riptide/master/LICENSE)

*Riptide: Compression* adds support to compress request bodies.

## Features

- pluggable compression mechanism
- out of the box GZIP support

## Dependencies

- Java 8
- Riptide: Core

## Installation

Add the following dependency to your project:

```xml
<dependency>
<groupId>org.zalando</groupId>
<artifactId>riptide-compression</artifactId>
<version>${riptide.version}</version>
</dependency>
```

## Configuration

```java
Http.builder()
.plugin(new RequestCompressionPlugin())
.build();
```

By default request bodies are compressed using [GZIP](https://docs.oracle.com/javase/8/docs/api/java/util/zip/GZIPOutputStream.html).

In order to specify the compression algorithm you can pass in a custom `Compression`:

```java
new RequestCompressionPlugin(Compression.of("br", BrotliOutputStream::new));
lukasniemeier-zalando marked this conversation as resolved.
Show resolved Hide resolved
```

## Usage

```java
http.post("/events")
.contentType(MediaType.APPLICATION_JSON)
.body(asList(events))
.call(pass())
.join();
```

All request bodies will be compressed using the configured compression method using `chunked` transfer-encoding.

If there is already a `Content-Encoding` specified on the request, the plugin does nothing.

### Limitations

* You must only configure a single `RequestCompressionPlugin` as only a single encoding is applied currently.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we reworded since you can adding multiple, but only the first one takes effect.

* Starting with Spring 4.3 the `Netty4ClientHttpRequestFactory` unconditionally adds a `Content-Length` header,
which breaks if used together with `RequestCompressionPlugin`. Use `riptide-httpclient` instead.


## Getting Help

If you have questions, concerns, bug reports, etc., please file an issue in this repository's [Issue Tracker](../../../../issues).

## Getting Involved/Contributing

To contribute, simply make a pull request and add a brief description (1-2 sentences) of your addition or change. For
more details, check the [contribution guidelines](../.github/CONTRIBUTING.md).
44 changes: 44 additions & 0 deletions riptide-compression/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>

<parent>
<groupId>org.zalando</groupId>
<artifactId>riptide-parent</artifactId>
<version>3.0.0-SNAPSHOT</version>
</parent>

<artifactId>riptide-compression</artifactId>

<name>Riptide: Request Compression</name>
<description>Client side response routing with stream support</description>

<dependencies>
<dependency>
<groupId>org.zalando</groupId>
<artifactId>riptide-core</artifactId>
</dependency>
<dependency>
<groupId>org.zalando</groupId>
<artifactId>riptide-httpclient</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-all</artifactId>
<version>4.1.43.Final</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-test</artifactId>
</dependency>
<dependency>
<groupId>com.github.rest-driver</groupId>
<artifactId>rest-client-driver</artifactId>
</dependency>
</dependencies>

</project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package org.zalando.riptide.compression;

import lombok.AllArgsConstructor;
import lombok.Getter;
import org.apiguardian.api.API;
import org.zalando.fauxpas.ThrowingUnaryOperator;

import java.io.IOException;
import java.io.OutputStream;

import static org.apiguardian.api.API.Status.EXPERIMENTAL;

@API(status = EXPERIMENTAL)
@AllArgsConstructor(staticName = "of")
@Getter
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of Getter it could expose a decorate method directly

Copy link
Member Author

Choose a reason for hiding this comment

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

@AllArgsConstructor(staticName = "of")
public final class Compression {
    @Getter
    private final String contentEncoding;
    private final ThrowingUnaryOperator<OutputStream, IOException> outputStreamDecorator;

    public OutputStream compress(final OutputStream stream) throws IOException {
        return outputStreamDecorator.tryApply(stream);
    }
}

Like this?

Working with contentEncoding may still be done by RequestCompressionPlugin (via the getter on Compression), or also the check for arguments.getHeaders().containsKey(CONTENT_ENCODING) should move IMO,

Copy link
Collaborator

@whiskeysierra whiskeysierra Nov 18, 2019

Choose a reason for hiding this comment

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

Maybe something like this?

public interface Compression {

    public static final Compression GZIP = new DefaultCompression("gzip", GzipOutputStream::new);

    void update(HttpHeaders headers);
    OutputStream decorate(OutputStream stream) throws IOException;

}

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think of:

Working with contentEncoding may still be done by RequestCompressionPlugin (via the getter on Compression), or also the check for arguments.getHeaders().containsKey(CONTENT_ENCODING) should move IMO,

Is related to your comment in #822 (comment)

public final class Compression {
private final String contentEncoding;
private final ThrowingUnaryOperator<OutputStream, IOException> outputStreamDecorator;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package org.zalando.riptide.compression;

import lombok.AllArgsConstructor;
import lombok.Getter;
import org.apiguardian.api.API;
import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpOutputMessage;
import org.springframework.http.StreamingHttpOutputMessage;
import org.zalando.riptide.Plugin;
import org.zalando.riptide.RequestArguments.Entity;
import org.zalando.riptide.RequestExecution;

import java.io.IOException;
import java.io.OutputStream;
import java.util.zip.GZIPOutputStream;

import static org.apiguardian.api.API.Status.EXPERIMENTAL;
import static org.springframework.http.HttpHeaders.CONTENT_ENCODING;
import static org.springframework.http.HttpHeaders.TRANSFER_ENCODING;

@API(status = EXPERIMENTAL)
public final class RequestCompressionPlugin implements Plugin {

private final Compression compression;
lukasniemeier-zalando marked this conversation as resolved.
Show resolved Hide resolved

public RequestCompressionPlugin(final Compression compression) {
this.compression = compression;
}

public RequestCompressionPlugin() {
this(Compression.of("gzip", GZIPOutputStream::new));
}

@Override
public RequestExecution aroundNetwork(final RequestExecution execution) {
return arguments -> {
final Entity entity = arguments.getEntity();
Copy link
Collaborator

Choose a reason for hiding this comment

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

final boolean alreadyCompressed = arguments.getHeaders().containsKey(CONTENT_ENCODING);

if (alreadyCompressed) {
    return execution.execute(arguments);
}

final Entity entity = arguments.getEntity();

if (entity.isEmpty()) {
    return execution.execute(arguments);
}

return ...;


if (entity.isEmpty() || arguments.getHeaders().containsKey(CONTENT_ENCODING)) {
return execution.execute(arguments);
}

return execution.execute(
arguments.withEntity(new CompressingEntity(compression, entity)));
};
}

@AllArgsConstructor
private static class CompressingEntity implements Entity {

private final Compression compression;
private final Entity entity;

@Override
public void writeTo(final HttpOutputMessage message) throws IOException {
update(message.getHeaders());

if (message instanceof StreamingHttpOutputMessage) {
final StreamingHttpOutputMessage streaming = (StreamingHttpOutputMessage) message;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm thinking whether this could be simplified and done only once for all entities, since it's always the same pattern.

streaming.setBody(stream ->
writeToCompressing(new DelegatingHttpOutputMessage(message.getHeaders(), stream)));
} else {
writeToCompressing(message);
}
}

private void writeToCompressing(HttpOutputMessage message) throws IOException {
try (final WrappingHttpOutputMessage compressingMessage =
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure whether this is the right time to close the output stream, the interfaces of Spring are very confusing to me. It seems to work though looking at the integration tests.

new WrappingHttpOutputMessage(message, compression.getOutputStreamDecorator())) {
entity.writeTo(compressingMessage);
}
}

private void update(final HttpHeaders headers) {
headers.set(CONTENT_ENCODING, compression.getContentEncoding());
lukasniemeier-zalando marked this conversation as resolved.
Show resolved Hide resolved
headers.set(TRANSFER_ENCODING, "chunked");
}

}

@AllArgsConstructor
@Getter
private static final class DelegatingHttpOutputMessage implements HttpOutputMessage {
Copy link
Member Author

Choose a reason for hiding this comment

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

At least in the LogbookPlugin this class already exists

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's just a couple of lines. No need to couple them together by trying to reuse something.

Copy link
Member

Choose a reason for hiding this comment

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

Why not to move them to core?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not worth having this kind of coupling just to save 3 lines of code.

private final HttpHeaders headers;
private final OutputStream body;
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package org.zalando.riptide.compression;

import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpOutputMessage;
import org.zalando.fauxpas.ThrowingUnaryOperator;

import javax.annotation.Nonnull;
import java.io.IOException;
import java.io.OutputStream;

final class WrappingHttpOutputMessage implements HttpOutputMessage, AutoCloseable {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd not be surprised if such class already exists somewhere in this project


private final HttpOutputMessage message;
private final ThrowingUnaryOperator<OutputStream, IOException> wrapper;
private OutputStream stream;

WrappingHttpOutputMessage(HttpOutputMessage message, ThrowingUnaryOperator<OutputStream, IOException> wrapper) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@AllArgsConstructor?

Copy link
Member Author

Choose a reason for hiding this comment

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

OutputStream stream is not injected, but managed internally.

this.message = message;
this.wrapper = wrapper;
}

@Nonnull
@Override
public OutputStream getBody() throws IOException {
if (stream == null) {
stream = wrapper.apply(message.getBody());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, this whole class is only used within writeToCompressing, i.e. it's safe to decorate the stream early on since we can be sure it will be called.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, it'll be on your head! 😉

}
return stream;
}

@Nonnull
@Override
public HttpHeaders getHeaders() {
return message.getHeaders();
}

@Override
public void close() throws IOException {
// make sure any underlying compressor gets flushed
if (stream != null) {
stream.close();
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
@ParametersAreNonnullByDefault
package org.zalando.riptide.compression;

import javax.annotation.ParametersAreNonnullByDefault;
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package org.zalando.riptide.compression;

import com.github.restdriver.clientdriver.ClientDriver;
import com.github.restdriver.clientdriver.DefaultRequestMatcher;
import com.github.restdriver.clientdriver.exception.ClientDriverSetupException;
import com.github.restdriver.clientdriver.jetty.ClientDriverJettyHandler;
import com.github.restdriver.clientdriver.jetty.DefaultClientDriverJettyHandler;
import com.google.gag.annotation.remark.Hack;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;
import org.eclipse.jetty.server.handler.gzip.GzipHandler;

@Hack
final class GzipClientDriver extends ClientDriver {

private int port;

static ClientDriver create() {
return new GzipClientDriver(new DefaultClientDriverJettyHandler(new DefaultRequestMatcher()));
}

private GzipClientDriver(ClientDriverJettyHandler handler) {
super(handler);
}

@Override
protected Server createAndStartJetty(int port) {
final Server jetty = new Server();
jetty.setHandler(handler);
final GzipHandler gzip = new GzipHandler();
gzip.setInflateBufferSize(1024);
jetty.insertHandler(gzip);
final ServerConnector connector = createConnector(jetty, port);
jetty.addConnector(connector);
try {
jetty.start();
} catch (Exception e) {
throw new ClientDriverSetupException("Error starting jetty on port " + port, e);
}
this.port = connector.getLocalPort();
return jetty;
}

@Override
public int getPort() {
return port;
}

@Override
public String getBaseUrl() {
return "http://localhost:" + port;
}

@Override
protected void replaceConnector(ServerConnector newConnector, Server jetty) {
throw new UnsupportedOperationException();
}
}
Loading