8000 Change server_info endpoint error message to be verbose by jacobj10 · Pull Request #439 · tableau/server-client-python · GitHub
[go: up one dir, main page]
Skip to content

Change server_info endpoint error message to be verbose #439

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
May 29, 2019
Merged

Change server_info endpoint error message to be verbose #439

merged 5 commits into from
May 29, 2019

Conversation

jacobj10
Copy link
Contributor

Add handling for case in which API version is not supported by the Tableau server. While trying to explore this library, I ran into an error when trying to call server.server_info_get()

>>> server.version = "2.5"
>>> server.server_info.get()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/jojacob/development/dasre-tableau-insights_trunk/build/dasre-tableau-insights/venv/lib/python3.7/site-packages/tableauserverclient/server/endpoint/endpoint.py", line 108, in wrapper
    return func(self, *args, **kwargs)
  File "/Users/jojacob/development/dasre-tableau-insights_trunk/build/dasre-tableau-insights/venv/lib/python3.7/site-packages/tableauserverclient/server/endpoint/server_info_endpoint.py", line 25, in get
    server_info = ServerInfoItem.from_response(server_response.content, self.parent_srv.namespace)
UnboundLocalError: local variable 'server_response' referenced before assignment

This stems from the code here:

https://github.com/jacobj10/server-client-python/blob/e4ec849b19f02f016f90e641756d9bd81d23d026/tableauserverclient/server/endpoint/server_info_endpoint.py#L17-L21

which can catch an error without setting the variable, leading to a confusing and non descriptive UnboundLocalError. Right now I just added an extra check for my specific error (version unsupported), but it my be worth just raising whatever error comes from the response.

This is my first time contributing so please let me know if you have any questions, comments, or concerns.

jacobj10 and others added 2 commits May 24, 2019 11:04
@jacobj10
Change server_info endpoint error message to be verbose
b64916d 
Add handling for case in which API version is not supported by the Tableau server.
@jacobj10
Update server_info_endpoint.py
6d8ed2c
@t8y8
Copy link
Collaborator
t8y8 commented May 24, 2019

I haven't reviewed in detail yet, but I think the problem is that we handle this a level above at https://github.com/jacobj10/server-client-python/blob/e4ec849b19f02f016f90e641756d9bd81d23d026/tableauserverclient/server/server.py#L76

In this case, you're manually setting the API version to 2.5, avoiding the 'use legacy version check' fallback. I agree we should handle this, and I'll take a look at your PR!

@jacobj10
Copy link
Contributor Author

Ahh I see, I think I was manually updating the version due to confusion when reading the documentation. The correct way is to use the class methods?

@t8y8
Copy link
Collaborator
t8y8 commented May 28, 2019 •
edited
Loading

Hey!

Yeah, the recommendation is to just use the methods like this:

auth = TSC.TableauAuth(USERNAME, PASSWORD)
server = TSC.Server("http://SERVER", use_server_version=True)

# OR
server = TSC.Server("http://SERVER")
server.use_server_version()

Regarding the bug, which is still sorta lurking there, even if you just use the methods above... maybe we should just catch all others and re-reaise? Would you be willing to update the PR to do that?

To contribute you just need to sign the CLA (and make the build green) and we can help you get it merged in. Info on filling out the CLA can be found here: http://tableau.github.io/contributing.html

@t8y8
Copy link
Collaborator
t8y8 commented May 28, 2019

... I'm not sure how that test is failing given you didn't touch sorting/filtering.

Can you make sure your fork is updated tot he latest development branch?

@jacobj10
Copy link
Contributor Author
jacobj10 commented May 28, 2019 •
edited
Loading

Weird, seems like the fork is up to date with the origin dev branch? I tried kicking off another CI run but to no avail :(

Screen Shot 2019-05-28 at 1 56 43 PM

@jacobj10
Copy link
Contributor Author
jacobj10 commented May 28, 2019 •
edited
Loading

The test fails locally on my master branch as well. It looks like [] isn't being URL encoded properly:

- pagenumber=13&pagesize=13&filter=tags:in:%5bstocks,market%5d
?                                          ^^^             ^^^
+ pagenumber=13&pagesize=13&filter=tags:in:[stocks,market]

@t8y8
Copy link
Collaborator
t8y8 commented May 28, 2019

The only explanation I have is some update in a library we depend on changed the encoding behavior?

Feel free to take a look at that and include it in your PR, or ignore it and I won't hold it against you :)

@jacobj10
Copy link
Contributor Author
jacobj10 commented May 28, 2019 •
edited
Loading

Ack I'll try to include in this PR, I'll let you know if I can't figure it out.

Update: Yeah looks like it was an issue with requests

@jacobj10
Copy link
Contributor Author

Yep that did it. More of a single issue fix. It might be worth seeing if there are any utils that the request module uses to URI decode/encode the response and use that accordingly with all relevant assertions, but that seems like it should be addressed in its own PR if at all. LGTM!

@t8y8 t8y8 self-assigned this May 29, 2019
@t8y8
Copy link
Collaborator
t8y8 commented May 29, 2019

Thanks, I'll take it!

Can you confirm you've sent in the CLA? (I don't have an easy way to check on my end) I'll wait a day and then merge this.

@jacobj10
Copy link
Contributor Author

Sent :)

@james-baker
Copy link

CLA approved.

t8y8
t8y8 approved these changes May 29, 2019
View reviewed changes
Copy link
Collaborator
@t8y8 t8y8 left a comment

Choose a reason for hiding this comment

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

🚀

@t8y8 t8y8 merged commit ea437a2 into tableau:development May 29, 2019
@jacobj10 jacobj10 deleted the server-info-verbose-error branch May 29, 2019 20:28
@shinchris shinchris mentioned this pull request Oct 4, 2019
Merged
shinchris pushed a commit that referenced this pull request Oct 4, 2019
Merge pull request #507 from tableau/development
b260ebc 
Release v0.9
## 0.9 (4 Oct 2019)

* Added Metadata API endpoints (#431)
* Added site settings for Data Catalog and Prep Conductor (#434)
* Added new fields to ViewItem (#331)
* Added support and samples for Tableau Server Personal Access Tokens (#465)
* Added Permissions endpoints (#429)
* Added tags to ViewItem (#470)
* Added Databases and Tables endpoints (#445)
* Added Flow endpoints (#494)
* Added ability to filter projects by topLevelProject attribute (#497)
* Improved server_info endpoint error handling (#439)
* Improved Pager to take in keyword arguments (#451)
* Fixed UUID serialization error while publishing workbook (#449)
* Fixed materalized views in request body for update_workbook (#461)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8y8 t8y8 t8y8 approved these changes

Assignees

@t8y8 t8y8

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jacobj10 @t8y8 @james-baker
0