Large file upload fix without rest template#123
Large file upload fix without rest template#123maheshsooryambylu wants to merge 19 commits intodevelopfrom
Conversation
…dcontent failing the whole operation wont fail
maheshsooryambylu
requested review from
ashishjain14,
dheerasameerk,
rashmiangadi11,
rishikunnath2747 and
yashmeet29
|
NIT: |
sdm/pom.xml
Outdated
| <dependency> | ||
| <groupId>org.apache.httpcomponents.client5</groupId> | ||
| <artifactId>httpclient5</artifactId> | ||
| <version>5.4.2</version> |
There was a problem hiding this comment.
Please move the version to top of pom.
sdm/pom.xml
Outdated
| <counter>INSTRUCTION</counter> | ||
| <value>COVEREDRATIO</value> | ||
| <minimum>0.90</minimum> | ||
| <minimum>0.85</minimum> |
There was a problem hiding this comment.
why was this reduced?
| return cachedToken; | ||
| } | ||
|
|
||
| public static Map<String, String> fillTokenExchangeBody(String token, SDMCredentials sdmEnv) { |
There was a problem hiding this comment.
SDMCredentials sdmEnv is passed but not used, can we remove it.
There was a problem hiding this comment.
SDMCredentials sdmEnvis passed but not used, can we remove it.
Done
|
|
||
| public static Map<String, String> fillTokenExchangeBody(String token, SDMCredentials sdmEnv) { | ||
| Map<String, String> parameters = new HashMap<>(); | ||
| parameters.put("assertion", token); |
There was a problem hiding this comment.
We can add a null check for token and throw an exception if ever token is null.
There was a problem hiding this comment.
This is the application access token and if null the app framework would have thrown the exception even before all these flows!
| try { | ||
| httpClient = HttpClients.createDefault(); | ||
| if (sdmCredentials.getClientId() == null) { | ||
| throw new IOException("No SDM binding found"); |
There was a problem hiding this comment.
Please move the message to constants.
| HttpPost httpPost = | ||
| new HttpPost( | ||
| sdmCredentials.getBaseTokenUrl() | ||
| + "/oauth/token?grant_type=urn:ietf:params:oauth:grant-type:jwt-bearer"); |
There was a problem hiding this comment.
"/oauth/token?grant_type=urn:ietf:params:oauth:grant-type:jwt-bearer" can be moved to a constant.
| HttpResponse response = httpClient.execute(httpPost); | ||
| String responseBody = extractResponseBodyAsString(response); | ||
| if (response.getStatusLine().getStatusCode() != HttpStatus.SC_OK) { | ||
| logger.error("Error fetching token with JWT bearer : " + responseBody); |
There was a problem hiding this comment.
In case status is not 200 do we still need to continue the processing? Or can we throw an exception from here and exit?
| throw new OAuth2ServiceException( | ||
| "Unexpected error while fetching client protocol: " + e.getMessage()); | ||
| } catch (IOException e) { | ||
| logger.error( |
There was a problem hiding this comment.
In case of IO exception do we not need to throw any exception? We are only logging it?
| public class RetryUtils { | ||
|
|
||
| private RetryUtils() { | ||
| // Doesn't do anything |
There was a problem hiding this comment.
Yes, we need a default constructor for a class with the static methods.
|
Thanks for the review comments @PujaDeshmukh17 |
PujaDeshmukh17
left a comment
PujaDeshmukh17
left a comment
There was a problem hiding this comment.
Looks good, Thanks
maheshsooryambylu
changed the title
| <dependency> | ||
| <groupId>log4j</groupId> | ||
| <artifactId>log4j</artifactId> | ||
| <version>1.2.17</version> | ||
| </dependency> | ||
|
|
There was a problem hiding this comment.
There is a Critical Security Risk in this library version. Please check this.
maheshsooryambylu
dismissed stale reviews from yashmeet29 and PujaDeshmukh17
via
671f4d6
|
Hi @maheshsooryambylu : Can we close this PR? |
5 participants
Content Upload fix with Content Streaming:
Please refer the PR #121 for overall fix approach and details. The main difference here when compared to that PR is that there is no restTemplate here and hence removed all the dependency with that of springfaramework. Could match the performance if not exactly same as that with restTemplate. May be we need to test for performance with this fix and with that should be good to ship this.