Skip to content
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

Moshi parsing exceptions are swallowed #6

Closed
sebaslogen opened this issue Feb 18, 2020 · 6 comments
Closed

Moshi parsing exceptions are swallowed #6

sebaslogen opened this issue Feb 18, 2020 · 6 comments
Labels
bug Something isn't working

Comments

@sebaslogen
Copy link

sebaslogen commented Feb 18, 2020

Moshi parsing exceptions are swallowed when data parsing fails.

The culprit is this line that rethrows exceptions instead of parsing them into unknown/generic errors:
https://github.com/haroldadmin/NetworkResponseAdapter/blob/master/src/main/kotlin/com/haroldadmin/cnradapter/ErrorExtraction.kt#L32

Strange enough, the exception is logged to logcat but wrapping the suspend function with a try/catch does not work, the exception is swallowed somewhere between this library and retrofit.

W/System.err: com.squareup.moshi.JsonDataException: Required value 'pointsReceived' missing at $
W/System.err:     at com.squareup.moshi.internal.Util.missingProperty(Util.java:605)
W/System.err:     at core.data.remote.model.Remote_LoyaltyUpgradeResponseJsonAdapter.fromJson(Remote_LoyaltyUpgradeResponseJsonAdapter.kt:44)
W/System.err:     at core.data.remote.model.Remote_LoyaltyUpgradeResponseJsonAdapter.fromJson(Remote_LoyaltyUpgradeResponseJsonAdapter.kt:17)
W/System.err:     at com.squareup.moshi.internal.NullSafeJsonAdapter.fromJson(NullSafeJsonAdapter.java:40)
W/System.err:     at retrofit2.converter.moshi.MoshiResponseBodyConverter.convert(MoshiResponseBodyConverter.java:45)
W/System.err:     at retrofit2.converter.moshi.MoshiResponseBodyConverter.convert(MoshiResponseBodyConverter.java:27)
W/System.err:     at retrofit2.OkHttpCall.parseResponse(OkHttpCall.java:225)
W/System.err:     at retrofit2.OkHttpCall$1.onResponse(OkHttpCall.java:121)
W/System.err:     at okhttp3.RealCall$AsyncCall.run(RealCall.kt:140)
W/System.err:     at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
W/System.err:     at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
W/System.err:     at java.lang.Thread.run(Thread.java:919)
@haroldadmin
Copy link
Owner

Is it possible that you're running a CoroutineExceptionHandler in the CoroutineScope of your API call?

I shall take a look at this over the weekend and see if I can reproduce this.

@sebaslogen
Copy link
Author

Is it possible that you're running a CoroutineExceptionHandler in the CoroutineScope of your API call?

We don't have any CoroutineExceptionHandler, I also searched in the whole project just in case but nothing.

@haroldadmin
Copy link
Owner

haroldadmin commented Feb 23, 2020

I'm not sure where exactly is the error swallowed, but this issue does highlight the need for a new subclass for NetworkResponse, which can be used for all errors that are not Network/Server errors.

I've created a new development branch and just pushed a new release 4.0.0-alpha01 to it, which contains these changes: a new NetworkResponse.UnknownError class, which can be used for handling data parsing errors.

Since this is a breaking change, I have not merged it to the master branch yet. Please try it and let me know if this change helps you.

Version 4.0.0-alpha01

@sebaslogen
Copy link
Author

Awesome, thanks! I'll give it a try and let you know.

@sebaslogen
Copy link
Author

Integration into our project worked fine and now it shows a correct error in the UI instead of swallowing the exception 👍
PS: I quickly reviewed the code of the fix and it looks 💯 good quality.

Thanks again for the fix and fast response.

@haroldadmin haroldadmin added the bug Something isn't working label Feb 23, 2020
@haroldadmin
Copy link
Owner

I'm glad the fix works for you. I'm gonna keep it in alpha, and promote it to a stable release after some time. Closing this issue for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants