-
Notifications
You must be signed in to change notification settings - Fork 23
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
Replaced httpclient with Okhttp. (bunq/sdk_java#25) #73
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor changes required.
|
||
return createBunqResponseRaw(response); | ||
} catch (IOException | URISyntaxException exception) { | ||
throw new UncaughtExceptionError(exception); | ||
} catch (IOException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exception
} catch (IOException | URISyntaxException exception) { | ||
throw new UncaughtExceptionError(exception); | ||
} catch (IOException e) { | ||
throw new UncaughtExceptionError(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exception
} | ||
|
||
private static void assertResponseSuccess(int responseCode, byte[] responseBodyBytes, String responseId) { | ||
if (responseCode != HttpStatus.SC_OK) { | ||
if (responseCode != 200) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constant
return new BunqRequestBody(contentType, content.length, 0, content); | ||
} | ||
|
||
@Override public MediaType contentType() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Override
on its own line.
return contentType; | ||
} | ||
|
||
@Override public long contentLength() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
return byteCount; | ||
} | ||
|
||
@Override public void writeTo(BufferedSink sink) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
case "PUT": | ||
return HttpMethod.PUT; | ||
default: | ||
throw new BunqException(String.format("The method \"%s\" is unexpected", methodString)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constant.
// List<NameValuePair> params = URLEncodedUtils.parse(uri, Charset.defaultCharset()); | ||
// | ||
// for (NameValuePair param : params) { | ||
// if (responseParam.equals(param.getName())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
U whut mate ?
@patrickdw1991 please 👀 |
return environmentType.getBaseUri(); | ||
} | ||
|
||
public String getApiVersoin() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
} catch (URISyntaxException exception) { | ||
throw new UncaughtExceptionError(exception); | ||
} | ||
String getBaseUri() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intended to be default access instead of private public or protected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, so that it can only be accessed publicly via ApiContext.
return this.baseUri; | ||
} | ||
|
||
String getApiVersion() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
* Time out constants. | ||
*/ | ||
private static final int TIMEOUT_SECONDS = 30; | ||
public static final int OK_STATUS_CODE = 200; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead of checking for only 200 just check for the full 200 range, like according to http spec 201 can be CREATED, we don't really use it in our backend but think it would be better to do so.
} catch (NoSuchAlgorithmException | KeyManagementException | MalformedURLException exception) { | ||
throw new UncaughtExceptionError(exception); | ||
} | ||
private OkHttpClient buildOkHttpClient() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally you put a docblock above all methods right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also for a lot of other methods ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doe het dan
@@ -344,14 +376,15 @@ public BunqResponseRaw get(String uri, Map<String, String> params, | |||
* @return The raw response of the PUT request. | |||
*/ | |||
public BunqResponseRaw put(String uri, byte[] requestBodyBytes, | |||
Map<String, String> customHeaders) { | |||
Map<String, String> customHeaders) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting
|
||
@Override | ||
public BunqRequestBuilder method(String method, RequestBody body) { | ||
RequestBody bodyToPassTpSuper; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
import com.bunq.sdk.exception.BunqException; | ||
|
||
public enum HttpMethod { | ||
GET("GET"), POST("POST"), PUT("PUT"), DELETE("DELETE"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicer to just put it on separate lines
if (responseJson.has(responseField) && !responseJson.get(responseField).isJsonNull()) { | ||
URI uri = new URI(responseJson.get(responseField).getAsString()); | ||
List<NameValuePair> params = URLEncodedUtils.parse(uri, Charset.defaultCharset()); | ||
HttpUrl url = HttpUrl.parse(EXAMPLE_URL + responseJson.get(responseField).getAsString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment here why you do this :)
HttpResponse httpEntity, PublicKey keyPublicServer) { | ||
byte[] responseBytes = getResponseBytes(responseCode, responseBodyBytes, | ||
httpEntity.getAllHeaders()); | ||
Response response, PublicKey keyPublicServer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting
Might be a good idea to also add the gradle checkstyle check in here as a simple check on code quality works great for our android code :) |
78261b2
to
e3442ce
Compare
e3442ce
to
fb26ecb
Compare
@patrickdw1991 yours again, please 👀 |
@@ -316,8 +316,8 @@ private static String getResponseId(Response response) { | |||
|
|||
/** | |||
*/ | |||
private static void assertResponseSuccess(int responseCode, byte[] responseBodyBytes, String responseId) { | |||
if (Pattern.matches(OK_STATUS_CODE_RANGE, responseId)) { | |||
private static void assertResponseSuccess(Integer responseCode, byte[] responseBodyBytes, String responseId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible here that the response code is null? (we had the situation before when the request doesn't reach the server (timeouts))
@OGKevin GoGoGo |
} catch (NoSuchAlgorithmException | KeyManagementException | MalformedURLException exception) { | ||
throw new UncaughtExceptionError(exception); | ||
} | ||
private OkHttpClient buildOkHttpClient() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doe het dan
41651e8
to
ddd991a
Compare
ddd991a
to
609b35e
Compare
This PR closes/fixes the following issues: