Add default timeout

[Sytten]

Changes

This PR is an updated version of #148.
I decided for 5s, because 30s seems excessive to me (especially in the context of a sync web server), feel free to disagree.

References

Having no timeout is never a good idea in prod. Auth0 might have issues and we don't want to hang our python server because of that.

Testing

• This change adds unit test coverage
• This change has been tested on the latest version

Checklist

• I have read the Auth0 general contribution guidelines
• I have read the Auth0 Code of Conduct
• All existing and new tests complete without errors

[Sytten]

@lbalmaceda this ready to merge, please don't let it rot :)

[damieng]

@joshcanhelp A lot of this addresses things you requested in #148 - can you take a look?

[damieng]

Fixes #82

[joshcanhelp]

@Sytten - This looks great so far, thank you for putting in the effort on this!

Can you put the default timeout in a constant so that value is only stored in a single place?

[joshcanhelp]

@lbalmaceda - Can you give this a quick once-over to make sure we're not missing anything?

[lbalmaceda]

@Sytten Thanks for raising this. We have already in our backlog a task to improve the customization of the HTTP client, but in a generic way.

Moving forward, this library should not be receiving a new parameter for each new configuration that the HTTP client requires to be set. Ideally a single options object can hold them all and be extended in the future without the need to change the rest of the method signatures.

What we would like is this configuration object to have for now 2 properties:

• connect_timeout
• read_timeout

These would later be set, when present, using this tuple.

Regarding the defaults I don't have a preference. I understand it should be a value close to 10 secs. Don't forget to move it to a constant somewhere it can be referenced from the multiple places you are using it.

Finally, please improve the tests. I rather them set a real world timeout value in seconds vs milliseconds. You should be able to "wait" for it to timeout vs depending on how fast the test is being run. What current tests are missing:

• Test the base or rest classes, use a value like 5 seconds (different from the default value)
• Add a test case for when the user doesn't set the timeout value and the default is used instead
• Add test cases for each of the entity classes (e.g. tenant) that are passing the options down to the base / rest class; ensure the base http client is set up with the correct options.

If you agree with all, we're happy to review the changes again after you submit them.

Cheers

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇‍♂️

[Sytten]

I will finish this PR later this week

[stevehobbsdev]

@Sytten Please let us know if this is something that you are still interested in working on, will be happy to review the changes when they come in.

[Sytten]

@stevehobbsdev Sorry I didn't have much time to work on open source and multiple projects, if its a priority to you I would say to go ahead and do it, otherwise I will eventually finish it but I don't have a timeline.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇‍♂️

[Sytten]

Not stale, but no progress either

[lbalmaceda]

Thanks for the update @Sytten. We'll see if we can fit this into our backlog in the coming weeks.

[lbalmaceda]

👋 we're closing this PR in favor of the linked one. We appreciate your contribution.