Conversation
Includes 1. getRemoteConfiguration checking for the flag. And assuming false if no values exist. Also stores in sharedPreferences to load from it next time. 2. isAutoRetryOnJWTFailure on `IterableAPI` which other classes with use. Not public. 3. Introduced three states which Taskmanager can rely on. VALID, INVALID, UNKNOWN. 4. Listener pub/sub added which will invoke onAuthTokenReady() on listner class. Adding array of AuthTokenReadyListener on Authmanager and its add and remove list methods. 5. Right now authHandler null returns true hoping to bypass jwt. But JWT based request will fail if API needs jwt. This could pile up storage. But this seems like a valid scenario 6. handleAuthTokenSucess sets authtoken to unknown when a new token is set. I hope it should be right approach. Because its new and unknown at this point. 7. `IterableConfig` has final boolean `autoRetryOnJwtFailure`. Its not something I want customers to be able to configure. It should be removed before going to master. It should be internal only variable. 8. IterableRequestTask - has some changes introduced due to the flow not falling under jwt failure. Response code coming has -1 was the root cause. Hence the change around responseCode >= 0 && responseCode < 400 9. `IterableRequestTask` - Line 215 - Still not sure if RequestProcessor check should happen. It does make sense to have it as only offline stored request will be reprocessed. And this feature should only work for those requests going through offline Request processor 10. `IterableRequestTask` - fromJSON method change is something I havent checked properly. I think its for unit testing the changes are done. 11. IterableTaskRunner. Would like to see if I can remove the method - isJWTFailure(responseCode). May be its a small abstraction thats needed. 12. Havent checked unit tests properly yet
| * The remote configuration flag takes precedence when present; | ||
| * otherwise the local {@link IterableConfig#autoRetryOnJwtFailure} value is used. | ||
| */ | ||
| boolean isAutoRetryOnJwtFailure() { |
There was a problem hiding this comment.
is there a reason for it not to be private or public?
| try { | ||
| BufferedReader in; | ||
| if (responseCode < 400) { | ||
| if (responseCode >= 0 && responseCode < 400) { |
There was a problem hiding this comment.
is there a 0 code that can occur?
There was a problem hiding this comment.
Yes. In testing I received -1. Hence thought its a good idea to keep 0 - 300 range.
| } else { | ||
| in = new BufferedReader( | ||
| new InputStreamReader(urlConnection.getErrorStream())); | ||
| InputStream errorStream = urlConnection.getErrorStream(); |
There was a problem hiding this comment.
Isn't it possible to extract just this flow of getting the resultResult to a helper class? this part is quite verbose
Ayyanchira
left a comment
Ayyanchira
left a comment
There was a problem hiding this comment.
Self review until OfflienRequestProcessor.java
| in = new BufferedReader( | ||
| new InputStreamReader(urlConnection.getErrorStream())); | ||
| InputStream errorStream = urlConnection.getErrorStream(); | ||
| if (errorStream != null) { | ||
| in = new BufferedReader( | ||
| new InputStreamReader(errorStream)); | ||
| } else { | ||
| in = null; | ||
| } | ||
| } | ||
| String inputLine; | ||
| StringBuffer response = new StringBuffer(); | ||
| while ((inputLine = in.readLine()) != null) { | ||
| response.append(inputLine); | ||
| if (in != null) { | ||
| String inputLine; | ||
| StringBuffer response = new StringBuffer(); | ||
| while ((inputLine = in.readLine()) != null) { | ||
| response.append(inputLine); | ||
| } | ||
| in.close(); | ||
| requestResult = response.toString(); | ||
| } | ||
| in.close(); | ||
| requestResult = response.toString(); |
There was a problem hiding this comment.
Looks like this change is not required at all
| // If getResponseCode() returned -1 (e.g. due to network inspector | ||
| // interference) but the response body contains JWT error codes, | ||
| // we can infer the actual response was a 401. | ||
| if (responseCode == -1 && matchesJWTErrorCodes(jsonResponse)) { | ||
| responseCode = 401; | ||
| } |
There was a problem hiding this comment.
-1's can be discarded. Dont have to categorize into 401 flow.
| // retry. The task stays in the DB and IterableTaskRunner will retry it once | ||
| // a valid JWT is obtained via the AuthTokenReadyListener callback. | ||
| // For online requests or when autoRetry is disabled, use the existing inline retry. | ||
| boolean autoRetry = IterableApi.getInstance().isAutoRetryOnJwtFailure(); |
There was a problem hiding this comment.
Also thinking, it should not be a public variable that should be accessible for developers like this. Can cause confusion.
| IterableAuthManager authManager = IterableApi.getInstance().getAuthManager(); | ||
| authManager.setIsLastAuthTokenValid(false); | ||
| long retryInterval = authManager.getNextRetryInterval(); | ||
| authManager.scheduleAuthTokenRefresh(retryInterval, false, null); |
There was a problem hiding this comment.
This 4 line can be combined into one if authmanager has scheduleAuthTOkenRefresh which figures out internally whats the nextRetryInterval. Too much back and forth.
| String authToken; | ||
| if (authTokenOverride != null) { | ||
| authToken = authTokenOverride; | ||
| } else if (jsonData.has("authToken")) { | ||
| authToken = jsonData.getString("authToken"); | ||
| } else { | ||
| authToken = ""; |
There was a problem hiding this comment.
When is authTokenOverride coming in picture? Need to revisit this.
| // Only schedule timed retry for non-auth failures. | ||
| // Auth failures will resume via onAuthTokenReady() callback. | ||
| if (!autoRetry || !isPausedForAuth) { | ||
| scheduleRetry(); | ||
| } |
There was a problem hiding this comment.
Revisit. Test needed for this.
| // Use the current live auth token instead of the stale one stored in the DB. | ||
| // The token in the DB was captured at queue time and may have since expired. | ||
| String currentAuthToken = IterableApi.getInstance().getAuthToken(); | ||
| IterableApiRequest request = IterableApiRequest.fromJSON(getTaskDataWithDate(task), currentAuthToken, null, null); | ||
| request.setProcessorType(IterableApiRequest.ProcessorType.OFFLINE); | ||
| response = IterableRequestTask.executeApiRequest(request); |
There was a problem hiding this comment.
authToken should be applied during from within IterableRequestTask.execute. I dont think we should handle and add auth token here. But can be acceptable.
| healthMonitor.onDBError(); | ||
| } | ||
|
|
||
| boolean autoRetry = IterableApi.getInstance().isAutoRetryOnJwtFailure(); |
There was a problem hiding this comment.
This is repeated. Probably just one time class level variable that gets value from AuthManager should be good.
2 participants
🔹 Jira Ticket(s) if any
✏️ Description
IterableAPIwhich other classes with use. Not public.IterableConfighas final booleanautoRetryOnJwtFailure. Its not something I want customers to be able to configure. It should be removed before going to master. It should be internal only variable.IterableRequestTask- Line 215 - Still not sure if RequestProcessor check should happen. It does make sense to have it as only offline stored request will be reprocessed. And this feature should only work for those requests going through offline Request processorIterableRequestTask- fromJSON method change is something I havent checked properly. I think its for unit testing the changes are done.