[CT-352] catch and retry malformed json #4982

emmyoop · 2022-03-30T19:02:57Z

resolves #4849, resolves #4342

Description

There are still some exceptions when trying to install deps that fail without retrying. This PR tries to catch the rest of them.

It also caches the HUB response to reduce calls. See #4983 for some more detailed context.

Checklist

I have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
I have added information about my change to be included in the CHANGELOG.

github-actions · 2022-03-30T19:03:14Z

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

emmyoop · 2022-03-30T19:04:31Z

core/dbt/clients/registry.py

+    try:
+        # the quotes in the response were converted to single quotes which is not
+        # valid json.  json.dumps() will fix that.
+        json.loads(json.dumps(jsonData))


This feels bad. Any suggestions on a better way to do this welcome!

The comment doesn't state this yet but the api/v1/index.json path also returns a list objects that causes it fail.

wait... I think it should be valid JSON... it's just a list... according to jsonlint dot com it's valid

@jtcohen6 my understanding is json.resp() returns a json object which really means it's returning a python list in this case. So json.loads(jsonData) freaks out with TypeError: the JSON object must be str, bytes or bytearray, not list. I've updated all this confusing logic I added to just straight check if json.resp() is a list or dict. That's all we ever expect it to be. Since we control the HUB and the only time it's not one of those two things something else is going wrong, this is safe. It's also much clearer as to what's happening.

jtcohen6 · 2022-04-01T16:16:27Z

core/dbt/clients/registry.py

+    expected_type = dict
+    expected_keys = ["name", "packages", "downloads"]
+    return _get_with_retries(
+        "api/v1/{}/{}.json".format(name, version), registry_base_url, expected_type, expected_keys
+    )


We can avoid a lot of superfluous requests by making this a filter on the (cached) package request response, rather than a separate request. This is what I'm trying over in #4983:

dbt-core/core/dbt/clients/registry.py

Lines 85 to 87 in 25d4c96

def package_version(name, version, registry_base_url=None):

response = package(name, registry_base_url)

return response["versions"][version]

But we're not validating ["name", "packages", "downloads"] as expected keys of the package response there, just the presence of a versions key (which contains a list of version dictionaries, each of which should contain those keys).

Maybe we should take a hybrid approach like:

First try grabbing the specific version info from the (cached) package response

Check to ensure that the expected keys are present (["name", "packages", "downloads"])

If they're missing for some reason, fall back to making the current version-specific request, with retries: "api/v1/{}/{}.json"

Check to ensure that the expected keys are present (["name", "packages", "downloads"])

Another way of doing this: RegistryPackageMetadata.from_dict() here, on the specific version response, instead of in dbt.deps.registry._fetch_metadata

…t-labs/dbt-core into er/ct-352-json-malform

nathaniel-may

Makes sense to me! Added a bunch of optional suggestions as well.

nathaniel-may · 2022-04-04T14:51:44Z

core/dbt/clients/registry.py

+        error_msg = (
+            f"Request error: The response type of {type(response)} is not valid: {resp.text}"
+        )


let's put what we expected in this error message

nathaniel-may · 2022-04-04T14:52:41Z

core/dbt/clients/registry.py


-def package(name, registry_base_url=None):
-    response = _get_with_retries("api/v1/{}.json".format(name), registry_base_url)
+    expected_keys = ["name", "versions"]


since we seem to only use this as a set, maybe we could just create it as a set using {} instead of []

nathaniel-may · 2022-04-04T14:54:15Z

core/dbt/clients/registry.py

+    if not set(expected_keys).issubset(response) and not set(expected_version_keys).issubset(
+        response["versions"]
+    ):
+        error_msg = f"Request error: Expected the response to contain keys {expected_keys} but one or more are missing: {resp.text}"


[nit] we can use set diff to discover and explicitly report these keys. There's only 3 keys right now so if we don't want to bother I'm fine with that too.

nathaniel-may · 2022-04-04T14:55:42Z

core/dbt/clients/registry.py

-def _get(path, registry_base_url=None):
-    url = _get_url(path, registry_base_url)
+def _get(package_name, registry_base_url=None):
+    url = _get_url(package_name, registry_base_url)
    fire_event(RegistryProgressMakingGETRequest(url=url))
    resp = requests.get(url, timeout=30)


can this line throw at all?

Yes. But we're catching all exceptions from requests in the retry logic.

lets add a comment to that effect then.

nathaniel-may · 2022-04-04T14:58:31Z

core/dbt/clients/registry.py

+def package(package_name, registry_base_url=None):
+    # returns a dictionary of metadata for all versions of a package
+    response = _get_with_retries(package_name, registry_base_url)
+    return response["versions"]
+
+
+def package_version(package_name, version, registry_base_url=None):
+    # returns the metadata of a specific version of a package
+    response = package(package_name, registry_base_url)
+    return response[version]
+
+
+def get_available_versions(package_name):
+    # returns a list of all available versions of a package
+    response = package(package_name)
+    return list(response)
+
+
+def _get_index(registry_base_url=None):
+
+    url = _get_url("index", registry_base_url)


I like these functions and how they abstract over retries. Can we add type hints to them?

jtcohen6

Nice work! This is a huge improvement

github-actions · 2022-04-05T15:44:19Z

The backport to 1.0.latest failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.0.latest 1.0.latest
# Navigate to the new working tree
cd .worktrees/backport-1.0.latest
# Create a new branch
git switch --create backport-4982-to-1.0.latest
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick --mainline 1 7f953a6d48a4db254b17fd3929d267e4b6069228
# Push it to GitHub
git push --set-upstream origin backport-4982-to-1.0.latest
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.0.latest

Then, create a pull request where the base branch is 1.0.latest and the compare/head branch is backport-4982-to-1.0.latest.

emmyoop added 3 commits March 30, 2022 10:14

catch None and malformed json reponses

3a63407

add json.dumps for format

960a577

format

46fcc60

cla-bot bot added the cla:yes label Mar 30, 2022

emmyoop commented Mar 30, 2022

View reviewed changes

jtcohen6 and others added 4 commits March 31, 2022 17:14

Cache registry request results. Avoid one request per version

c2cf124

updated to be direct in type checking

152a1ae

add changelog entry

8ef8171

add back logic for none check

e599c42

barberscott mentioned this pull request Mar 31, 2022

Cache + minimize registry requests in dbt deps #4983

Closed

4 tasks

jtcohen6 and others added 2 commits April 1, 2022 11:10

PR feedback: memoize > global

25d4c96

add checks for expected types and keys

201171d

jtcohen6 reviewed Apr 1, 2022

View reviewed changes

emmyoop added 3 commits April 1, 2022 13:27

Merge branch 'jerco/cache-registry-requests' of https://github.com/db…

e8d10d2

…t-labs/dbt-core into er/ct-352-json-malform

consolidated cache and retry logic

62476d3

minor cleanup for clarity/consistency

ee3b5e5

emmyoop marked this pull request as ready for review April 1, 2022 21:16

emmyoop requested a review from a team April 1, 2022 21:16

emmyoop requested review from a team as code owners April 1, 2022 21:16

emmyoop added the backport 1.0.latest Tag for PR to be backported to the 1.0.latest branch label Apr 1, 2022

emmyoop requested review from nathaniel-may, gshank and jtcohen6 and removed request for a team April 4, 2022 13:05

nathaniel-may approved these changes Apr 4, 2022

View reviewed changes

add pr review suggestions

6a7b8f9

update unit test

ab1a31b

emmyoop requested a review from leahwicz April 5, 2022 01:15

leahwicz approved these changes Apr 5, 2022

View reviewed changes

jtcohen6 approved these changes Apr 5, 2022

View reviewed changes

emmyoop merged commit 7f953a6 into main Apr 5, 2022

emmyoop deleted the er/ct-352-json-malform branch April 5, 2022 15:44

This was referenced Apr 5, 2022

[BACKPORT] 4982 to 1.0.latest #4996

Closed

Backport 4982 deps #5007

Merged

dbeatty10 mentioned this pull request Jul 28, 2022

[Bug] dbt deps sometimes takes >90sec to complete #4342

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CT-352] catch and retry malformed json #4982

[CT-352] catch and retry malformed json #4982

emmyoop commented Mar 30, 2022 •

edited by dbeatty10

Loading

github-actions bot commented Mar 30, 2022

emmyoop Mar 30, 2022

emmyoop Mar 30, 2022

jtcohen6 Mar 31, 2022

emmyoop Mar 31, 2022

jtcohen6 Apr 1, 2022 •

edited

Loading

jtcohen6 Apr 1, 2022

nathaniel-may left a comment

nathaniel-may Apr 4, 2022

nathaniel-may Apr 4, 2022

nathaniel-may Apr 4, 2022

nathaniel-may Apr 4, 2022

emmyoop Apr 4, 2022

nathaniel-may Apr 4, 2022

nathaniel-may Apr 4, 2022

jtcohen6 left a comment

github-actions bot commented Apr 5, 2022

	def package_version(name, version, registry_base_url=None):
	response = package(name, registry_base_url)
	return response["versions"][version]

[CT-352] catch and retry malformed json #4982

[CT-352] catch and retry malformed json #4982

Conversation

emmyoop commented Mar 30, 2022 • edited by dbeatty10 Loading

Description

Checklist

github-actions bot commented Mar 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtcohen6 Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathaniel-may left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtcohen6 left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 5, 2022

emmyoop commented Mar 30, 2022 •

edited by dbeatty10

Loading

jtcohen6 Apr 1, 2022 •

edited

Loading