Fix request-compression stream handling

[lukasniemeier-zalando]

This is an attempt to fix the broken RequestCompressionPlugin.

Description

• ensure compressing stream wrapper is closed, hence compressors are flushed
• handle streaming output (also move header manipulation up to be visible in streaming mode)
• ensure compression stream is wrapped only once (vs. each time a writer to the output message calls getBody)

Also...

• extracted the plugin to it's own artifact to avoid module cycles
• extended the tests to actually verify (de-)compression for a set of
  request factories

Types of changes

• Bug fix
• New feature
• Breaking change

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.

[lukasniemeier-zalando]

At least in the LogbookPlugin this class already exists

[whiskeysierra]

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

[fatroom]

Why not to move them to core?

[whiskeysierra]

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

[lukasniemeier-zalando]

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.

[lukasniemeier-zalando]

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

[whiskeysierra]

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

[whiskeysierra]

Instead of Getter it could expose a decorate method directly

[lukasniemeier-zalando]

@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,

[whiskeysierra]

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;

}

[lukasniemeier-zalando]

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)

[whiskeysierra]

@AllArgsConstructor?

[lukasniemeier-zalando]

OutputStream stream is not injected, but managed internally.

[whiskeysierra]

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

[whiskeysierra]

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

[whiskeysierra]

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.

[lukasniemeier-zalando]

Ok, it'll be on your head! 😉

[whiskeysierra]

Is Jetty removing the original header and adds this one to indicate it was compressed?

[lukasniemeier-zalando]

Yes, I did not want to put some listener/callback mechanism in my (un-)gzipping rest client driver Jetty hack for now...

[whiskeysierra]

👍