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

Adding more informative error messaging due to potential JWT and Clerk data discrepancy #144

Merged
merged 12 commits into from
Jul 23, 2024

Conversation

Andrewq11
Copy link
Contributor

Changelogs

Due to the potential discrepancy between claims in JWTs issued from the Hub and the true data in Clerk, we want to provide users with a more informative error message in the event of forbidden errors (for upload) and not found errors (for downloads). More specifically, we would like to suggest to the user to retry logging in to get a fresh JWT with up-to-date claims from Clerk.

  • Added error messaging suggesting a re-login in the case of 403 forbidden errors when a user tries to upload a dataset, benchmark or result.
  • Added error messaging suggesting a re-login in the case of 404 not found errors when a user tries to retrieve a dataset or benchmark.
  • Updated the doc strings for the above client methods with a helpful note regarding the potential discrepancy.

Checklist:

  • Was this PR discussed in an issue? It is recommended to first discuss a new feature into a GitHub issue before opening a PR.
  • Add tests to cover the fixed bug(s) or the newly introduced feature(s) (if appropriate).
  • Update the API documentation if a new function is added, or an existing one is deleted.
  • Write concise and explanatory changelogs above.
  • If possible, assign one of the following labels to the PR: feature, fix, chore, documentation or test (or ask a maintainer to do it for you).

discussion related to that PR

@Andrewq11 Andrewq11 added documentation Improvements or additions to documentation chore labels Jul 19, 2024
@Andrewq11 Andrewq11 self-assigned this Jul 19, 2024
@Andrewq11 Andrewq11 requested a review from cwognum as a code owner July 19, 2024 19:44
Copy link
Collaborator

@cwognum cwognum left a comment

Choose a reason for hiding this comment

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

Thanks for the initiative, @Andrewq11 ! That is a nice quality of life improvement.

@Andrewq11 Andrewq11 marked this pull request as draft July 22, 2024 02:19
@Andrewq11 Andrewq11 marked this pull request as ready for review July 22, 2024 02:20
@Andrewq11 Andrewq11 requested a review from cwognum July 22, 2024 02:27
Copy link
Collaborator

@cwognum cwognum left a comment

Choose a reason for hiding this comment

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

Thanks again, @Andrewq11 ! Throwing custom exceptions in Python is not something I've really done before, so I'm learning with you here. I feel like the current approach is not very Pythonic and did some further suggestions that may help in that regard. Curious to hear what you think! Appreciate your patience.

I'm curious how other libs do this, e.g. numpy. No need to overthink it yet, but maybe there is some inspiration to be found there once we want more advanced functionality.

@cwognum cwognum mentioned this pull request Jul 23, 2024
@cwognum
Copy link
Collaborator

cwognum commented Jul 23, 2024

@Andrewq11 With me being OOO, the time difference and the fast approaching deadline, I decide to make a PR into your branch to make my suggestions very explicit: #153.

I won't be available after 14:00 CET (08:00 EST), so you have my blessing to accept or reject these suggestions as you see fit and to merge this PR afterwards! 😉

cwognum and others added 3 commits July 23, 2024 15:13
* Suggestion that came up during PR feedback

* Update raise statements

* Updated parent class of PolarisUnauthorizedError

* Updated error message

* Updated error message

* Pass response in the PolarisFS

* small changes

* moving mixins to folder

---------

Co-authored-by: Andrew Quirke <andrewquirke99@gmail.com>
Copy link
Contributor

@mercuryseries mercuryseries left a comment

Choose a reason for hiding this comment

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

Thanks @Andrewq11! I added some minor comments/suggestions.

@Andrewq11 Andrewq11 merged commit dc93d84 into main Jul 23, 2024
4 checks passed
@jstlaurent jstlaurent deleted the chore/informative-error-messaging branch December 4, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants