Downloading corrupted files with OkHttp

Question

The method I wrote to download files always produce corrupted files.

public static String okDownloadToFileSync(final String link, final String fileName, final boolean temp, DownloadStatusManager statusManager, ErrorDisplayerInterface errorDisplayerInterface) {

    Request request = new Request.Builder()
            .url(link)
            .build();


    OkHttpClient client = Api.getInstance().getOkHttpClient();
    OutputStream output = null;
    InputStream input = null;

    try {

        Response response = client.newCall(request).execute();

        //Add the file length to the statusManager
        final int contentLength = Integer.parseInt(response.header("Content-Length"));
        if (statusManager != null) {
            statusManager.add(Hash.md5(link), contentLength);
        }

        //Get content type to know extension
        final String contentType = response.header("Content-Type");
        final String ext = contentTypeMap.get(contentType);

        Log.i(TAG, link + "\n --> contentType = " + contentType + "\n --> ext = " + ext);

        if (ext == null) {
            Log.e(TAG, "-----------\next is null, seems like there is a problem with that url : \n         " + link + "\n----------");
            return null;
        } else if (ext.equals("json")) {
            Log.e(TAG, "-----------\ndownloadable file seems to be a json, seems like there is a problem with that url : \n         " + link + "\n----------");
            return null;
        }

        //Check if file already exists
        if (!temp && fileName != null) {
            File test = new File(M360Application.getContext().getFilesDir(), fileName + "." + ext);
            if (test.exists()) {
                Log.i(TAG, "File exists ! : " + test.getPath());
                test.delete();
                //return test.getAbsolutePath();
            }
        }

        // expect HTTP 200 OK, so we don't mistakenly save error report instead of the file
        if (!response.isSuccessful()) {
            errorDisplayerInterface.popWarn(null, "Error while downloading " + link, "connection.getResponseCode() != HttpURLConnection.HTTP_OK");
            return null;
        }

        input = response.body().byteStream();

        File file;
        if (temp) {
            file = File.createTempFile(UUID.randomUUID().toString(), ext, M360Application.getContext().getCacheDir());
        } else {
            file = new File(M360Application.getContext().getFilesDir(), fileName + "." + ext);
        }


        output = new FileOutputStream(file);
        
        output.write(response.body().bytes());

//            byte data[] = new byte[4096];
//            long total = 0;
//            int count;
//            while ((count = input.read(data)) != -1) {
//                output.write(data, 0, count);
//                total++;
//
//                if (statusManager != null) {
//                    statusManager.update(Hash.md5(link), contentLength - total);
//                }
//           }

        return file.getAbsolutePath();
    } catch (IOException e) {
        e.printStackTrace();
        errorDisplayerInterface.popError(null, e);

    } finally {
        if (statusManager != null) {
            statusManager.finish(Hash.md5(link));
        }
        try {
            if (output != null)
                output.close();
            if (input != null)
                input.close();
        } catch (IOException ignored) {
            ignored.printStackTrace();
        }

    }
    return null;
}

I access these file via adb, transfer them to my sccard, and there I see that they seem to have the proper size, but has no type according to for instance Linux file command.

Would you know what is missing and how to fix it?

Thank you.

Edit

Simpler version of the code ( but the bug is the same )

public static String okioDownloadToFileSync(final String link, final String fileName) throws IOException {

    Request request = new Request.Builder()
            .url(link)
            .build();


    OkHttpClient client = Api.getInstance().getOkHttpClient();
    Response response = client.newCall(request).execute();

    final int contentLength = Integer.parseInt(response.header("Content-Length"));

    //Get content type to know extension
    final String contentType = response.header("Content-Type");
    final String ext = contentTypeMap.get(contentType);

    // expect HTTP 200 OK, so we don't mistakenly save error report instead of the file
    if (!response.isSuccessful()) {
        return null;
    }

    File file = new File(M360Application.getContext().getFilesDir(), fileName + "." + ext);

    BufferedSink sink = Okio.buffer(Okio.sink(file));
    sink.writeAll(response.body().source());
    sink.close();

    Log.i(TAG, "file.length : " + file.length() + " | contentLength : " + contentLength);

    return file.getAbsolutePath();

}

The log : file.length : 2485394 | contentLength : 1399242

Solution

The problem was that I was getting the OkHttpClient from my API singleton, which was used by retrofit and had multiples interceptors. Those interceptors were polluting the response.

So I OkHttpClient client = Api.getInstance().getOkHttpClient(); became OkHttpClient client = new OkHttpClient.Builder().build(); and everything is now ok !

Thanks a lot. I'm dividing the method into smaller pieces right now.

Just as a hint: from a "clean code quality" point of view ... you could do a lot of things to improve this code; starting by removing waste (such as commented code) and paying more attention to the "single layer of abstraction" rule - you simply do way too many things in that one poor method. Finally: read about try-with-resources. That saves you that whole finally checking. — GhostCat, Jan 12 '17 at 10:10
`they seem to have ther proper size`. Seem? Dont you even know if size is equal? To the last byte? — greenapps, Jan 12 '17 at 10:16
Hello thx for the feedback ! The commented code is there to show you that there I tried multiples way to feed the file. As for the single abstraction rule, what would you recommand to move ? I feel that the only thing this method does is downloading a file, I don't see what I could extract. Never heard about try-with-resources, I'm reading this immediatly. @greenapps As I wrote it I realized that I only checked the KB size and not as you stated it "to the last byte", I'm currently compliling to check this fact — Renaud Favier, Jan 12 '17 at 10:22
I'll try this approach : http://stackoverflow.com/questions/25893030/download-binary-file-from-okhttp (the 80+ upvote response) — Renaud Favier, Jan 12 '17 at 10:48
@greenapps I should have checked sooner : file.length : 2485394 | contentLength : 1399242 — Renaud Favier, Jan 12 '17 at 10:50
You should compare the file size on the server with the resulting file size of the downloaded file. You did not do that. If you publish those values then tell how you determined them. — greenapps, Jan 12 '17 at 11:36
For the rest i do not understand that you have to use so much code to download a file using OkHttp. You are more or less downloading the file yourself now. It should be possible to let OkHttp do that all with one or two lines of code. — greenapps, Jan 12 '17 at 11:44
@greenapps the file size on the server is "1 399 242 bytes" (I downloaded it on my computer and checked) which is the same as the contentlenght given by it. the file length is "2 485 394 bytes" (I downloaded it on my computer and checked) whitch is identical to what file.length gave me. — Renaud Favier, Jan 12 '17 at 11:45
I add a simpler version of the code that reproduce the bug. I shouldn't have posted my full code. — Renaud Favier, Jan 12 '17 at 11:49
If you have a windows pc then open both the original file and the downloaded file in wordpad. What are the visible differences? What kind of file are you downloading? A jpg? Well try a .txt file as you can inspect much better txt files line by line as you can read them. — greenapps, Jan 12 '17 at 11:55
Hum ! it's the bad one : `2485394` maybe It's because my OkHttpClient has interceptors, I try to get a new one and not the one used by retrofit — Renaud Favier, Jan 12 '17 at 11:57
Is your response.header("Content-Length") same as response.body().contentLength(); ? Because you are using Integer.parseInt() while content length is usually Long — Chupik, Jan 12 '17 at 12:00
That was it ! I changed my OkHttpClient for a new one with no interceptors and I now have the correct sizes ! Thanks every one for guiding me throught this debug ! I'll update my post with the answer after lunch, even if I doubt it can be useful to anyone else.. — Renaud Favier, Jan 12 '17 at 12:04

score 1 · Answer 1 · answered Jan 12 '17 at 10:56

1

Instead of output.write(response.body().bytes()); try something like this

byte[] buff = new byte[1024 * 4];

while (true) {
   int byteCount = input.read(buff);
   if (byteCount == -1) {
       break;
   }
   output.write(buff, 0, byteCount);
}

answered Jan 12 '17 at 10:56

Chupik

1,040
8
13

It looks a lot like the commented part (just under `output.write(response.body().bytes());` ) that I tried before. I didn't have more success with it – Renaud Favier Jan 12 '17 at 10:59
What does your Log output says? Maybe, you have some problems with content-type and extension? – Chupik Jan 12 '17 at 11:02
The extension is ok, the file has an incorrect size thought : `Log.i(TAG, "file.length : " + file.length() + " | contentLength : " + contentLength);` => `file.length : 2485394 | contentLength : 1399242` – Renaud Favier Jan 12 '17 at 11:22
Welll try this suggestion and report the sizes please. – greenapps Jan 12 '17 at 11:51
zero to bytcount saved my life xd – Arif Nouman Khan Apr 16 '18 at 06:31

Downloading corrupted files with OkHttp

Edit

Solution

1 Answers1

Linked