Skip to content

Conversation

@weineran
Copy link

  • This is a bug fix.
  • The current code assumes that response_data will always be a dict or a mapping of some kind, but it is possible for response_data to be a str.
  • When response_data is a str, then the function fails on response_data.get("error") but the error response is swallowed so the user doesn't know why it failed. The user just sees: AttributeError: 'str' object has no attribute 'get' and a stacktrace
  • This is a quick and dirty fix that guarantees the .get call will succeed, and the result is that the error response bubbles up to the user.
  • Feel free to make this fix more robust/clean.

Steps to reproduce the bug

  • Set credentials like this:
credentials = google.oauth2.credentials.Credentials(
        token=None,
        refresh_token=os.getenv('GCP_REFRESH_TOKEN'),
        token_uri=os.getenv('GCP_TOKEN_URI'),
        client_id=os.getenv('GCP_CLIENT_ID'),
        client_secret=os.getenv('GCP_CLIENT_SECRET'))

But ensure that there is a typo in GCP_TOKEN_URI. Example: "https://oauth2.googleapis.com/token1"

  • Use the credentials to authenticate. Example:
df.to_gbq(destination_table=tablename, project_id=project_id, if_exists=if_exists, table_schema=schema, credentials=credentials)

Expected: Authentication should fail with a useful error response
Actual: Authentication fails with an AttributeError: "AttributeError: 'str' object has no attribute 'get'"

* The current code assumes that response_data will always be a dict or a mapping of some kind, but it is possible for response_data to be a str.
* When response_data is a str, then the function fails on `response_data.get("error")` but the error response is swallowed so the user doesn't know why it failed. The user just sees: `AttributeError: 'str' object has no attribute 'get'` and a stacktrace
* This is a quick and dirty fix that guarantees the `.get` call will succeed, and the result is that the error response bubbles up to the user.
* Feel free to make this fix more robust/clean.
@weineran weineran requested review from a team as code owners May 24, 2023 22:31
Comment on lines +328 to +330
if type(response_data) != dict:
response_data = {"data": response_data}

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this. This was fixed in the oauth2 client code like so.

Suggested change
if type(response_data) != dict:
response_data = {"data": response_data}
if isinstance(response_data, six.string_types):
raise exceptions.RefreshError(response_data, retryable=False)

Please make sure to add a unit test for this!

Copy link
Author

Choose a reason for hiding this comment

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

Hey @clundin25, I'm not really interested in running with this one. Mostly just wanted to point out the bug.

If the maintainers want to take over from here, that would be great 😊

@arithmetic1728
Copy link
Contributor

fixed in #1316 @weineran thank you for pointing out the issue and for your fix!

@weineran
Copy link
Author

weineran commented Jun 1, 2023

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants