Closing response body on RateLimitException#175
Closing response body on RateLimitException#175lbalmaceda merged 4 commits intoauth0:masterfrom j-m-x:connection-leak-on-rate-limit-exception
Conversation
| response = client.newCall(request).execute(); | ||
| try (Response response = client.newCall(request).execute()) { | ||
| return parseResponse(response); | ||
| } catch (APIException e) { |
There was a problem hiding this comment.
Shouldn't this be Auth0Exception instead? Given that APIException is a subclass, people could later try/catch or ask what instanceof is the exception they receive and cast that to a APIException or read it as Auth0Exception. What are your thoughts?
There was a problem hiding this comment.
You're right, will do.
There was a problem hiding this comment.
Your comment got me thinking and you're very right. This not only is better for the user of this client, but it actually was a bug. If an exception of type Auth0Exception was thrown, it would pass through the APIException catch and be caught by the IOException catch where it got wrapped in an extra Auth0Exception with the message Failed to execute request. Changing the first catch to Auth0Exception fixes this. I realised this a little late, but in a second commit I improvement the tests a bit to prove this.
| return createRateLimitException(response); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
can this diff be removed?
There was a problem hiding this comment.
Shouldn't have been there, will do.
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.hamcrest.Matchers.*; | ||
|
|
||
| public class BaseRequestTest { |
There was a problem hiding this comment.
Please add a FIXME comment at the class level pointing to the link you shared in the issue. My idea here is that in the future when they let users use this feature programmatically, we can come back to this code and fix it. Because today it seems enabling this extension is affecting the whole tests folder and not just a single class. https://github.com/mockito/mockito/wiki/What%27s-new-in-Mockito-2#mock-the-unmockable-opt-in-mocking-of-final-classesmethods
| response = Mockito.mock(Response.class); | ||
|
|
||
| call = Mockito.mock(Call.class); | ||
| Mockito.when(call.execute()).thenReturn(response); |
There was a problem hiding this comment.
try statically importing this Mockito methods so you don't repeat Mockito. every time
There was a problem hiding this comment.
Bad habit of mine, will do.
lbalmaceda
left a comment
lbalmaceda
left a comment
There was a problem hiding this comment.
Thanks (again!) for the time you've invested in this 🎉
|
No problem, thanks for the constructive feedback and quick replies 🤘 |
2 participants
References #174 #