amplitude/Amplitude-Java

SDK did not exception when wrong apikey, also do you have the way to enable log when using sdk ?

TuanDang1509 opened this issue · 6 comments

Using wrong api-key but sdk without thow or log any exception.
image

Expected Behavior

Should throw out clearly exception about wrong credentials.

Current Behavior

Just keep silent in case wrong key.

Possible Solution

Should throw out clearly exception about wrong credentials.
Should not retry hundred times in case wrong key.

You should response response.status = 401 in this case and throw out if meet this err code.

Steps to Reproduce

Environment

  • Unity Plugin Version:
  • Device:
  • Device OS and Version:

Please update your document about sdk version. Cause the version below was fail when build with jdk 11

image

Good catch on the Java version issue @TuanDang1509 . That's been fixed now.

That's also good reporting for the wrong API key case. We'll improve on that.

Hi @dantetam ,
Thank you for quickly response on this. I have just take a look at sdk source code.
Could you please consider change apiKey parameter from String to char[] ?
Cause I am using char[] to store key as below advice. I can change to String when call Amplitude but I think it's better if char[] is used.
https://www.baeldung.com/java-storing-passwords

And, May you public these parameters out by getter ?
private Queue eventsToSend;
private boolean aboutToStartFlushing;
Cause I need write down some JUnit testcase and your code is async, I need add await until your queue is empty.

Ex:
await().atMost(10, TimeUnit.SECONDS).until(Amplitude.eventsToSend.size() == 0)

image

@TuanDang1509

  1. For the suggestion of using char[] for apiKey:
    I just looked over the link you provided. I think the apiKey is not meant to be a secure password and actually, we don't need to change it so it's ok to immutable. Also from a convention perspective, we want to keep using the String type for apiKey.
  2. For the requirement of making the private method public.
    Actually, we don't want to the public those private functions to the customer. And we are going to add unit tests in this sprint. But you're welcome to write your own tests inside the SDK.

Thank you @yuhao900914,

I got your point of view.

Hi @TuanDang1509,
We fixed the API key issue in the new version v1.2.2. It will be released soon.
The response status will be still 400 since that's not an easy change for the HTTP API side. And because we are using async call, it cannot directly throw exceptions without joining the main thread.
However, for the API key issue, we are able to print out the error message with the logger. And it also won't retry anymore.
Thanks.