Skip to content

[#120179893] Make sure compression and gzip header go together #27

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.

Sign up for GitHub

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

Open
ericlemes wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[#120179893] Make sure compression and gzip header go together #27

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d217f2
[#120179893] Make sure compression and gzip header go together
ericlemes Nov 9, 2016
12b0568
[#120179893] Merge from master
ericlemes Nov 9, 2016
ad502fe
[#120179893] Adding tests and checking if gzip header is present befo…
ericlemes Nov 10, 2016
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
13 changes: 8 additions & 5 deletions reyna-test/src/com/b2msolutions/reyna/DispatcherTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import com.b2msolutions.reyna.blackout.TimeRange;
import com.b2msolutions.reyna.http.HttpPost;
import com.b2msolutions.reyna.shadows.ShadowAndroidHttpClient;
import com.b2msolutions.reyna.system.Header;
import com.b2msolutions.reyna.system.Clock;
import com.b2msolutions.reyna.system.Message;
import com.b2msolutions.reyna.system.Preferences;
Expand All @@ -25,6 +26,7 @@
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.ArgumentCaptor;
import org.mockito.Matchers;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
import org.robolectric.RobolectricTestRunner;
Expand Down Expand Up @@ -100,7 +102,7 @@ public void sendMessageHappyPathShouldSetExecuteCorrectHttpPostAndReturnOK() thr

@Test
public void sendMessageHappyPathWithChineseCharactersShouldSetExecuteCorrectHttpPostAndReturnOK() throws URISyntaxException, IOException, KeyManagementException, UnrecoverableKeyException, KeyStoreException, NoSuchAlgorithmException, CertificateException {
Message message = RepositoryTest.getMessageWithHeaders("谷歌拼音输入法");
Message message = RepositoryTest.getMessageWithHeaders("谷歌拼音输入");
Copy link
Contributor

@CharlesGarth CharlesGarth Nov 9, 2016

Choose a reason for hiding this comment

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

I know you explained to me already but I'm still not quite understanding why this has changed, this test should not be gzipping anything as far as i can tell, meaning it shouldn't be limited by gzip min size and so shouldn't be entering the getCompressedEntity() method, and staying as text/plain. Or am i missing something?

Copy link
Author

@ericlemes ericlemes Nov 10, 2016

Choose a reason for hiding this comment

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

Moving back to the old version. This was failing due to the missing check for the shouldGzip function. With the check in place the previous test case works. I didn't observe that this test sets up for no compression headers (getMessageWithHeaders instead of getMessageWithGzipHeaders). Nice spot.

Copy link
Contributor

@CharlesGarth CharlesGarth Nov 10, 2016

Choose a reason for hiding this comment

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

😎


StatusLine statusLine = mock(StatusLine.class);
when(statusLine.getStatusCode()).thenReturn(200);
Expand All @@ -123,7 +125,7 @@ public void sendMessageHappyPathWithChineseCharactersShouldSetExecuteCorrectHttp
verify(httpPost).setEntity(stringEntityCaptor.capture());
StringEntity stringEntity = stringEntityCaptor.getValue();
assertEquals(stringEntity.getContentType().getValue(), "text/plain; charset=UTF-8");
assertEquals(EntityUtils.toString(stringEntity), "谷歌拼音输入法");
assertEquals(EntityUtils.toString(stringEntity), "谷歌拼音输入");
}

@Test
Expand Down Expand Up @@ -209,6 +211,7 @@ public void getResultShouldReturnExpected() {
@Test
public void sendMessageWithGzipAndContentIsLessThanMinGzipLengthShouldRemoveGzipHeaderAndSendMessageAsString() throws Exception {
Message message = RepositoryTest.getMessageWithGzipHeaders("body");
message.addHeader(new Header("Content-Encoding", " gzip "));

StatusLine statusLine = mock(StatusLine.class);
when(statusLine.getStatusCode()).thenReturn(200);
Expand All @@ -228,9 +231,9 @@ public void sendMessageWithGzipAndContentIsLessThanMinGzipLengthShouldRemoveGzip

this.verifyHttpPost(message, httpPost);

ArgumentCaptor<ByteArrayEntity> byteArrayEntityCaptor = ArgumentCaptor.forClass(ByteArrayEntity.class);
verify(httpPost).setEntity(byteArrayEntityCaptor.capture());
ByteArrayEntity entity = byteArrayEntityCaptor.getValue();
ArgumentCaptor<StringEntity> stringEntityArgumentCaptor = ArgumentCaptor.forClass(StringEntity.class);
verify(httpPost).setEntity(stringEntityArgumentCaptor.capture());
AbstractHttpEntity entity = stringEntityArgumentCaptor.getValue();
assertNull(entity.getContentEncoding());
assertEquals(EntityUtils.toString(entity, "utf-8"), "body");
}
Expand Down
58 changes: 44 additions & 14 deletions reyna/src/com/b2msolutions/reyna/Dispatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,19 @@
import org.apache.http.HttpResponse;
import org.apache.http.client.HttpClient;
import org.apache.http.entity.AbstractHttpEntity;
import org.apache.http.entity.ByteArrayEntity;
import org.apache.http.entity.StringEntity;
import org.apache.http.impl.client.AbstractHttpClient;
import org.apache.http.protocol.HTTP;

import java.io.ByteArrayOutputStream;
import java.io.OutputStream;
import java.net.URI;
import java.text.ParseException;
import java.util.ArrayList;
import java.util.GregorianCalendar;
import java.util.Locale;
import java.util.zip.GZIPOutputStream;

public class Dispatcher {

Expand Down Expand Up @@ -232,8 +237,7 @@ private static Header[] removeGzipEncodingHeader(Header[] headers) {
ArrayList<Header> filteredHeaders = new ArrayList<Header>();

for (Header header : headers) {
if (header.getKey().equalsIgnoreCase("content-encoding")
&& header.getValue().equalsIgnoreCase("gzip")) {
if (header.getKey().equalsIgnoreCase("content-encoding")) {
Copy link
Contributor

@CharlesGarth CharlesGarth Nov 9, 2016

Choose a reason for hiding this comment

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

this shouldGzip() method isn't being used anymore and can be removed

Copy link
Author

@ericlemes ericlemes Nov 10, 2016

Choose a reason for hiding this comment

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

This is a very good spot. Thanks. Actually shouldGzip should be used. If we don't receive the header from elemez (or any app using reyna lib) we should not compress at all. Fixing it.

continue;
}

Expand All @@ -244,9 +248,44 @@ private static Header[] removeGzipEncodingHeader(Header[] headers) {
return filteredHeaders.toArray(returnedHeaders);
}
Copy link
Member

@b2mdevelopment b2mdevelopment Nov 10, 2016

Choose a reason for hiding this comment

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

Can we leave the old logic as it is and check if the steam is compressed or not
boolean isZipped = new ZipInputStream(yourInputStream).getNextEntry() != null;
and remove the content-type "zip" if not?

Or just replace the function as you did but remove this check as this means we have to go through every single bytes if it is not compressed.


private static AbstractHttpEntity getCompressedEntity(String content, Context context) throws Exception {
byte[] data = content.getBytes();
return AndroidHttpClient.getCompressedEntity(data, context.getContentResolver());
private static AbstractHttpEntity getEntity(Message message, Context context) throws Exception {
String content = message.getBody();

byte[] data = content.getBytes("UTF-8");
if (data.length <= (AndroidHttpClient.getMinGzipSize(context.getContentResolver()) * 2)){
Copy link
Contributor

@CharlesGarth CharlesGarth Nov 9, 2016

Choose a reason for hiding this comment

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

Does this mean that we'll gzip everything that is over the gzip minimum regardless of whether they have the gzip header or not?

Copy link
Author

@ericlemes ericlemes Nov 10, 2016

Choose a reason for hiding this comment

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

Fixing this. Should respect the header.

return new StringEntity(content, HTTP.UTF_8);
}
else {
return getCompressedEntity(data, context);
}
}

private static AbstractHttpEntity getCompressedEntity(byte[] data, Context context) throws Exception {
ByteArrayOutputStream arr = new ByteArrayOutputStream();
OutputStream zipper = new GZIPOutputStream(arr);
zipper.write(data);
zipper.close();

byte[] compressedData = arr.toByteArray();
int end = compressedData.length > 500 ? 500 : compressedData.length;

Boolean equal = true;
for (int i = 0; i < end; i++){
if (compressedData[i] != data[i]){
equal = false;
break;
}
}
AbstractHttpEntity entity;
if (!equal){
entity = new ByteArrayEntity(compressedData);
entity.setContentEncoding("gzip");
}
else {
entity = new ByteArrayEntity(data);
Copy link
Contributor

@CharlesGarth CharlesGarth Nov 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

i think this is maybe missing a unit test case

entity.setContentEncoding("");
}
return entity;
}

private void setHeaders(HttpPost httpPost, Header[] headers) {
Expand All @@ -263,16 +302,7 @@ private String getSubmittedTimestamp() {
return String.valueOf(this.clock.getCurrentTimeMillis());
}

private static AbstractHttpEntity getEntity(Message message, Context context) throws Exception {
String content = message.getBody();
AbstractHttpEntity entity = new StringEntity(content, HTTP.UTF_8);

if (Dispatcher.shouldGzip(message.getHeaders())) {
entity = getCompressedEntity(content, context);
}

return entity;
}

public static boolean isBatteryCharging(Context context) {
Intent batteryStatus = context.registerReceiver(null, new IntentFilter(Intent.ACTION_BATTERY_CHANGED));
Expand Down