Skip to content

Conversation

@yoshimin
Copy link
Contributor

In iOS, currentAccessToken does not throw error even if token is null, but currently throw in android.
I want the same behavior on both OS.

@CLAassistant
Copy link

CLAassistant commented Oct 24, 2019

CLA assistant check
All committers have signed the CLA.

@onevcat
Copy link
Member

onevcat commented Oct 24, 2019

@plateaukao How do you think about it?

IMO, the non-existing token should not be an error, but return a null to the caller. So I think making Android behavior aligning with iOS should be a nicer choice.

But I am afraid it should not be a result.error(null), but a result.success(null). Can you review and confirm it?

@onevcat onevcat requested a review from plateaukao October 24, 2019 05:24
@onevcat
Copy link
Member

onevcat commented Oct 24, 2019

The error case is corresponding to these lines in our Android SDK: https://git.linecorp.com/LINE-Client/linesdk-android/blob/v5.0/line-sdk/src/main/java/com/linecorp/linesdk/api/internal/LineApiClientImpl.java#L150-L154

All internal exceptions are caught inside the AccessTokenCache.getAccessToken method. We cannot tell the difference between EncryptionException from the non-existing token. I think it would be fine to ignore the errors and classify all the retrieving exceptions as a "non-existing token" case.

As above, we have two choices in LineSdkWrapper.kt (https://github.com/line/flutter_line_sdk/pull/9/files#diff-3bb5f5fc7df7a490be803e132fd9c2bd) :

  1. Simply treat all !lineApiResponse.isSuccess cases as no token existing, and call result.success(null) to let the caller know it.
  2. Check the error type of lineApiResponse, if it is "The cached access token does not exist." as declared here (I really suspect there is a chance to break it unexpectedly in future...), we call result.success(null). For other error cases (there is no such one currently), we call result.error(error).

Option 1 is enough to me to solve current situation. But option 2 is more future-oreintated and can handle and propagate real errors to caller.

lineApiResponse.errorData.message,
null
)
result.error(null)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
result.error(null)
result.success(null)

Copy link
Member

@plateaukao plateaukao left a comment

Choose a reason for hiding this comment

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

LGTM

@onevcat onevcat merged commit 2c4a5f8 into line:master Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants