DroidKaigi / conference-app-2017

The Official Conference App for DroidKaigi 2017 Tokyo
Apache License 2.0
470 stars 140 forks source link

Fix potential problems in ResourceResolver.loadJSONFromAsset() #368

Closed sumio closed 7 years ago

sumio commented 7 years ago

Issue

ResourceResolver.loadJSONFromAsset() has the following code:

            is = context.getAssets().open("json/" + jsonFileName);
            int size = is.available();
            byte[] buffer = new byte[size];
            is.read(buffer);
            json = new String(buffer, "UTF-8");

The above code may read only partial content of the file because is.available() may return less than the length of the file.

Overview (Required)

Links

N/A

Screenshot

N/A

eyedol commented 7 years ago

@sumio good call on the use of available(). However, I think it's much better to use Okio to handle file reads as it's already a dependency. Something like the code below should work. I haven't read Okio source code yet but I believe it solves that issue. If not, feel free to ignore this.

String json = null;
BufferedSource source = null;
try {
    final InputStream is = context.getAssets().open("json/" + jsonFileName);
    source = Okio.buffer(Okio.source(is));
    json = source.readUtf8();
} catch (IOException e) {
    Timber.tag(TAG).e(e, "assets/json/%s: read failed", jsonFileName);
} finally {
    try {
        if (source != null) {
            source.close();
        }
    } catch (IOException e) {
        Timber.tag(TAG).e(e, "assets/json/%s: read failed", jsonFileName);
    }
}
return json;       
konifar commented 7 years ago

:eyes:

sumio commented 7 years ago

@eyedol Thanks for your nice suggestion! I'll rewrite it by using Okio in this night.

sumio commented 7 years ago

Rewriting is done! commons-io dependecy is removed.

sumio commented 7 years ago

It seems that circleCI is failed in ContributorsLocalDataSourceTest > updateAllAsyncAsInsert which is unrelated to this PR... (and all tests are passed in my local environment)

konifar commented 7 years ago

@sumio Sorry, CircleCI is not stable now 🙇

sumio commented 7 years ago

@konifar Thanks for your reply. OK. I won't mind the result.

konifar commented 7 years ago

@sumio Sorry for late review 🙇 LGTM! Thanks for contribution! 💯 @eyedol Thanks for great suggestion! I appreciate you!

konifar commented 7 years ago

Hope CI success 🙏

konifar commented 7 years ago

👏