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

feat(exceptions): separate failed signin error #1478

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

jorwoods
Copy link
Contributor

Closes #1472

This makes sign in failures their own class of exceptions, while still inheriting from NotSignedInException to not break backwards compatability for any existing client code. This should allow users to get out more specific exceptions more easily on what failed with their authentication request.

@jorwoods jorwoods force-pushed the jorwoods/signin_error branch 2 times, most recently from 878e813 to 74f7797 Compare September 28, 2024 17:07
@bcantoni
Copy link
Contributor

@jorwoods do you think we should/could incorporate a change here for the minor issue #1450? I don't know whether that other issue is big enough to fix, but since your change here is in a related area maybe we could do together.

@jorwoods
Copy link
Contributor Author

@bcantoni I made a tweak to how the ServerInfoItem does its error handling to raise the error immediately. It currently raises a NonXMLResponseError. Not sure if you would want something custom raised there to indicate that the problem is more extensive beyond it not being valid XML.

@jorwoods
Copy link
Contributor Author

I also changed the log level on those messages from info to exception so that they'll bubble up more clearly in logs.

Closes tableau#1472

This makes sign in failures their own class of exceptions, while still
inheriting from NotSignedInException to not break backwards
compatability for any existing client code. This should allow users
to get out more specific exceptions more easily on what failed with
their authentication request.
If ServerInfoItem.from_response gets invalid XML, raise the error
immediately instead of suppressing the error and setting an invalid
version number
@bcantoni
Copy link
Contributor

Did some quick testing and this change looks good.

Sign in with incorrect PAT, before:

tableauserverclient.server.endpoint.exceptions.NotSignedInError: (b'<?xml version=\'1.0\' encoding=\'UTF-8\'?><tsResponse xmlns="http://tableau.com/api" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://tableau.com/api https://help.tableau.com/samples/en-us/rest_api/ts-api_3_25.xsd"><error code="401001"><summary>Signin Error</summary><detail>The personal access token you provided is invalid.</detail></error></tsResponse>', 'https://MYSERVER/api/3.25/auth/signin')

Sign in with incorrect PAT, after:

tableauserverclient.server.endpoint.exceptions.FailedSignInError: Failed Sign In Error:

	401001: Signin Error
		The personal access token you provided is invalid.

Copy link
Contributor

@bcantoni bcantoni left a comment

Choose a reason for hiding this comment

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

On second thought I don't think we need to do anything specific for 1450 (no specific error for pointing to a non-Tableau server).

@jorwoods
Copy link
Contributor Author

Do you want it to raise the NonXMLResponseError? Or should I revert that change?

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