-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
WIP: Test gzip support in RESTEasy Reactive #16924
Changes from 5 commits
bdd5a5f
09c5fa7
f69d0b6
3f8f018
aa7bd48
e45b503
2eb085a
4438534
045548d
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,105 @@ | ||
package io.quarkus.resteasy.reactive; | ||
|
||
import java.awt.*; | ||
import java.awt.image.BufferedImage; | ||
import java.io.File; | ||
import java.io.IOException; | ||
import java.util.function.Supplier; | ||
|
||
import javax.imageio.ImageIO; | ||
import javax.ws.rs.GET; | ||
import javax.ws.rs.Path; | ||
|
||
import org.hamcrest.Matchers; | ||
import org.jboss.shrinkwrap.api.ShrinkWrap; | ||
import org.jboss.shrinkwrap.api.asset.StringAsset; | ||
import org.jboss.shrinkwrap.api.spec.JavaArchive; | ||
import org.junit.jupiter.api.Test; | ||
import org.junit.jupiter.api.extension.RegisterExtension; | ||
|
||
import io.quarkus.test.QuarkusUnitTest; | ||
import io.restassured.RestAssured; | ||
|
||
public class GZipTest { | ||
|
||
private static final String APP_PROPS = "" + | ||
"quarkus.http.enable-compression=true\n"; | ||
|
||
static String longString; | ||
static { | ||
|
||
StringBuilder sb = new StringBuilder(); | ||
for (int i = 0; i < 1000; ++i) { | ||
sb.append("Hello RESTEasy Reactive;"); | ||
} | ||
longString = sb.toString(); | ||
} | ||
|
||
@RegisterExtension | ||
static final QuarkusUnitTest test = new QuarkusUnitTest() | ||
.setArchiveProducer(new Supplier<JavaArchive>() { | ||
@Override | ||
public JavaArchive get() { | ||
return ShrinkWrap.create(JavaArchive.class) | ||
.addAsResource(new StringAsset(APP_PROPS), "application.properties") | ||
.addClasses(TestCompression.class); | ||
} | ||
}); | ||
|
||
@Test | ||
public void testServerCompression() throws Exception { | ||
|
||
RestAssured.given().get("/test/compression").then().statusCode(200) | ||
.header("content-encoding", "gzip") | ||
.header("content-length", Matchers.not(Matchers.equalTo(Integer.toString(longString.length())))) | ||
.body(Matchers.equalTo(longString)); | ||
|
||
RestAssured.given().get("/test/nocompression").then().statusCode(200) | ||
.header("content-encoding", "identity") | ||
.header("content-length", Matchers.equalTo((long) (createImage().getData().getDataBuffer().getSize() * 4L))) | ||
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.
Finding the 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. The size of this png image(original) is Using the above technique, the size is On calculating
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. Actually you can just use a static test image as a resource :) 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. Humm I thought it's better to generate an image only when this test is run. You are right, it will make code simpler. Instead of creating an image through code it is better to use a static image 💯 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. |
||
.body(Matchers.equalTo(createImage())); | ||
} | ||
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. So does this test pass? I don't see anything in the endpoints that would justify a different behaviour. 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. 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. 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. So in the beginning I am initializing APP_PROPS as true
perhaps that's why content encoding is always That means vert.x is trying to compress the binary data as well, no? 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. 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. So perhaps the conclusion is - It is trying to compress images as well. Am I right @FroMage? 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. Yes, probably, could you add that test to your PR? 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. Yeah sure, I missed this comment. |
||
|
||
@Path("/test") | ||
public static class TestCompression { | ||
|
||
@Path("/compression") | ||
@GET | ||
public String registerCompression() { | ||
return longString; | ||
} | ||
|
||
@Path("/nocompression") | ||
@GET | ||
public BufferedImage registerNoCompression() throws IOException { | ||
BufferedImage image = createImage(); | ||
return image; | ||
} | ||
} | ||
|
||
public static BufferedImage createImage() throws IOException { | ||
int width = 300; | ||
int height = 300; | ||
|
||
BufferedImage bufferedImage = new BufferedImage(width, height, BufferedImage.TYPE_INT_RGB); | ||
// Create a graphics which can be used to draw into the buffered image | ||
Graphics2D graphic = bufferedImage.createGraphics(); | ||
// fill all the image with white | ||
graphic.setColor(Color.white); | ||
graphic.fillRect(0, 0, width, height); | ||
|
||
// create a circle with black | ||
graphic.setColor(Color.black); | ||
graphic.fillOval(0, 0, width, height); | ||
|
||
// create a string with yellow | ||
graphic.setColor(Color.yellow); | ||
graphic.drawString("Quarkus RESTEasy Reactive", 50, 120); | ||
graphic.dispose(); | ||
|
||
// Save as PNG | ||
File file = new File("src/test/resources/imageForTest.png"); | ||
ImageIO.write(bufferedImage, "png", file); | ||
return bufferedImage; | ||
} | ||
} | ||
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. |
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.
what do you think of a test with the other way?
quarkus.http.enable-compression=false
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.
If it is disabled, should we really perform any sort of test? I don't think that's needed when is not even enabled :)
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.
but how to ensure that the property is working correctly?
and how do you make sure that no future code from other developers doesn't affect this and go undetected because not have any tests?
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.
@FroMage WDYT
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.
Well, we should test the default behaviour, that's true, but because we know what the default is ATM, we want to test the default behaviour without any configuration, to make sure it won't be compressed by default.