Skip to content
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

Optional support for connection / read timeout #27

Merged
merged 1 commit into from
Jun 12, 2018

Conversation

skjolber
Copy link
Contributor

@skjolber skjolber commented Jun 5, 2018

This would be really helpful for minimizing the impact of some transient network issues we are experiencing in our legacy network.

@skjolber
Copy link
Contributor Author

skjolber commented Jun 5, 2018

@lbalmaceda would you prefer validation of the connect-/read-timeout in the constructor? Any value below 0 will throw IllegalArgumentException in URLConnection.

@lbalmaceda
Copy link
Contributor

Thanks for the contribution.

Regarding defaults: Why not just use an Integer and check for null? null would mean "use the default" or "don't overwrite it at all".

Regarding validation: Yes, I rather it be part of the constructor (fail fast). The exception class is correct. Message should be something like "Invalid {name of param} value. Must be a positive integer.".

Finally, please remember to add tests. There should be a way to access those values. I thought about a package-private getter for the URLConnection (internally it just calls and returns url.openConnection()) but then you'd need to assert somehow that the getJWKS method is calling internally that getter.

@skjolber skjolber force-pushed the connectionTimeout branch 7 times, most recently from 2e73a85 to 9eb7a3a Compare June 5, 2018 17:18
@skjolber
Copy link
Contributor Author

skjolber commented Jun 5, 2018

@lbalmaceda changed type to Integer, added validation in constructor and added unit tests.

Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@skjolber
Copy link
Contributor Author

skjolber commented Jun 5, 2018

Accidentally used tabs for indent, changed to spaces.

@lbalmaceda
Copy link
Contributor

@skjolber thanks. I did notice the indentation was looking weird specially on the inner class, but thought you forgot to run the code formatter.

@lbalmaceda lbalmaceda merged commit 890662b into auth0:master Jun 12, 2018
@lbalmaceda lbalmaceda added this to the 0.5.0 milestone Jun 12, 2018
@skjolber skjolber deleted the connectionTimeout branch June 12, 2018 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants