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.

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

Bugfix/issue 3350 snowflake non json response #3365

Merged
merged 7 commits into from
May 26, 2021
Merged

Bugfix/issue 3350 snowflake non json response #3365

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
955034e
attempt at solving with while loop
matt-winkler May 17, 2021
fa6b453
added comment on loop
matt-winkler May 17, 2021
5724409
update changelog
matt-winkler May 17, 2021
379763e
Merge branch 'develop' into bugfix/issue-3350-snowflake-non-json-resp…
matt-winkler May 19, 2021
04fa30d
modified per drew's suggestions
matt-winkler May 20, 2021
b4b1eef
updates after linting
matt-winkler May 25, 2021
94ffaf6
Merge branch 'develop' into bugfix/issue-3350-snowflake-non-json-resp…
jtcohen6 May 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
- Update the snowflake adapter to only comment on a column if it exists when using the persist_docs config ([#3039](https://github.com/fishtown-analytics/dbt/issues/3039), [#3149](https://github.com/fishtown-analytics/dbt/pull/3149))
- Separate `compiled_path` from `build_path`, and print the former alongside node error messages ([#1985](https://github.com/fishtown-analytics/dbt/issues/1985), [#3327](https://github.com/fishtown-analytics/dbt/pull/3327))
- Fix exception caused when running `dbt debug` with BigQuery connections ([#3314](https://github.com/fishtown-analytics/dbt/issues/3314), [#3351](https://github.com/fishtown-analytics/dbt/pull/3351))
- Fix `dbt run` errors caused from receiving non-JSON responses from Snowflake with Oauth ([#3350](https://github.com/fishtown-analytics/dbt/issues/3350)


### Under the hood
- Added logic for registry requests to raise a timeout error after a response hangs out for 30 seconds and 5 attempts have been made to reach the endpoint ([#3177](https://github.com/fishtown-analytics/dbt/issues/3177), [#3275](https://github.com/fishtown-analytics/dbt/pull/3275))
Expand Down
23 changes: 21 additions & 2 deletions plugins/snowflake/dbt/adapters/snowflake/connections.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from contextlib import contextmanager
from dataclasses import dataclass
from io import StringIO
from time import sleep
from typing import Optional

from cryptography.hazmat.backends import default_backend
Expand Down Expand Up @@ -125,10 +126,28 @@ def _get_access_token(self) -> str:
'Authorization': f'Basic {auth}',
'Content-type': 'application/x-www-form-urlencoded;charset=utf-8'
}
result = requests.post(token_url, headers=headers, data=data)
result_json = result.json()

result_is_json = False
result_json = {}
max_iter = 20
iter_count = 1
# Attempt to obtain JSON for 1 second before throwing an error
while not result_is_json:
if iter_count > max_iter:
break

result = requests.post(token_url, headers=headers, data=data)

if result.headers.get('content-type') == 'application/json':
Copy link
Contributor

Choose a reason for hiding this comment

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

checking the content-type is totally reasonable, but I think a more robust approach would try/catch the .json() method call. That might look like (pseudo code):

result_json = None
for i in range(max_iter):
  result = requests.post(token_url, headers=headers, data=data)
  try:
    result_json = result.json()
    break
  except ValueError as e:
    message = response.text() # or something like that
    logger.debug(f"Got a non-json response ({result.status}): {e}, message: {message}")
    time.sleep(0.05)

if result_json is None:
  # We'd want to raise an error here, otherwise `result_json['access_token']` below will fail
  pass

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right on. The only reason I recommended checking the content-type instead of try/except in my issue comment was from having read about the potential slowness of the exception-catching approach. On further reading/thinking, that should only matter for very large request responses, which this isn't.

result_is_json = True
result_json = result.json()

iter_count += 1
sleep(0.05)

if 'access_token' not in result_json:
raise DatabaseException(f'Did not get a token: {result_json}')

return result_json['access_token']

def _get_private_key(self):
Expand Down