[#120179893] Make sure compression and gzip header go together #27
Conversation
ericlemes
commented
Nov 9, 2016
ericlemes
commented
- When removing the content-encoding header, don't check for gzip anymore. The idea is to be more strict and avoid content not matching with content-encoding header.
- After gzip compressing, compare the first 500 bytes of the compressed and uncompressed stream. If they are the same, send the original stream without setting the content encoding. A safety net to make sure we only send gzip header when the content is really gzipped.
| for (Header header : headers) { | ||
| if (header.getKey().equalsIgnoreCase("content-encoding") | ||
| && header.getValue().equalsIgnoreCase("gzip")) { | ||
| if (header.getKey().equalsIgnoreCase("content-encoding")) { |
There was a problem hiding this comment.
this shouldGzip() method isn't being used anymore and can be removed
There was a problem hiding this comment.
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.
| entity.setContentEncoding("gzip"); | ||
| } | ||
| else { | ||
| entity = new ByteArrayEntity(data); |
There was a problem hiding this comment.
i think this is maybe missing a unit test case
| String content = message.getBody(); | ||
|
|
||
| byte[] data = content.getBytes("UTF-8"); | ||
| if (data.length <= (AndroidHttpClient.getMinGzipSize(context.getContentResolver()) * 2)){ |
There was a problem hiding this comment.
Does this mean that we'll gzip everything that is over the gzip minimum regardless of whether they have the gzip header or not?
There was a problem hiding this comment.
Fixing this. Should respect the header.
| @Test | ||
| public void sendMessageHappyPathWithChineseCharactersShouldSetExecuteCorrectHttpPostAndReturnOK() throws URISyntaxException, IOException, KeyManagementException, UnrecoverableKeyException, KeyStoreException, NoSuchAlgorithmException, CertificateException { | ||
| Message message = RepositoryTest.getMessageWithHeaders("谷歌拼音输入法"); | ||
| Message message = RepositoryTest.getMessageWithHeaders("谷歌拼音输入"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
b2mdevelopment
left a comment
b2mdevelopment
left a comment
There was a problem hiding this comment.
I don't know.
It seems more risky than leaving it without fix or stop using legacy AndroidHttpClient and using URLConnection.
|
|
||
| private static AbstractHttpEntity getCompressedEntity(String content, Context context) throws Exception { | ||
| byte[] data = content.getBytes(); | ||
| return AndroidHttpClient.getCompressedEntity(data, context.getContentResolver()); |
There was a problem hiding this comment.
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.
| protected Clock clock; | ||
|
|
||
| public Dispatcher() { | ||
| public Dispatcher(OutputStreamFactory outputStreamFactory) { |
There was a problem hiding this comment.
can you leave the other constructor in as well? I think we have a few usages of Dispatcher() without args in Elemez and I can't be bothered to update them :D.
There was a problem hiding this comment.
Or you could follow the dependency "injection" anti-pattern we have in some of the other classes where we instantiate the dep in constructor and inject mock to the protected member... :P
