Skip to content

FIX: fetch_nb_dependencies must not save HTTP error pages #904

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

Open
Sanjays2402 wants to merge 1 commit into
base: main
Choose a base branch
from
+69 −5
Open

FIX: fetch_nb_dependencies must not save HTTP error pages #904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 16 additions & 5 deletions quantecon/util/notebooks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,12 @@
BRANCH = "master"
#Hard Coded Dependencies Folder on QuantEcon.notebooks
FOLDER = "dependencies"
#-Default timeout (seconds) for each request-#
TIMEOUT = 30


def fetch_nb_dependencies(files, repo=REPO, raw=RAW, branch=BRANCH, folder=FOLDER, overwrite=False, verbose=True):
def fetch_nb_dependencies(files, repo=REPO, raw=RAW, branch=BRANCH, folder=FOLDER, overwrite=False, verbose=True,
timeout=TIMEOUT):
"""
Retrieve raw files from QuantEcon.notebooks or other Github repo

Expand All @@ -45,6 +48,8 @@ def fetch_nb_dependencies(files, repo=REPO, raw=RAW, branch=BRANCH, folder=FOLDE
folder str, optional(default=FOLDER)
overwrite bool, optional(default=False)
verbose bool, optional(default=True)
timeout int or float, optional(default=TIMEOUT)
Timeout in seconds passed to each request

Examples
--------
Expand Down Expand Up @@ -74,7 +79,7 @@ def fetch_nb_dependencies(files, repo=REPO, raw=RAW, branch=BRANCH, folder=FOLDE
import requests

#-Generate Common Data Structure-#
if type(files) == list:
if isinstance(files, (list, tuple)):
files = {"" : files}

status = []
Expand All @@ -98,9 +103,15 @@ def fetch_nb_dependencies(files, repo=REPO, raw=RAW, branch=BRANCH, folder=FOLDE
if verbose: print("Fetching file: %s"%fl)
#-Get file in OS agnostic way using requests-#
url = "/".join([repo, raw, branch, folder, fl])
r = requests.get(url)
with open(fl, "wb") as fl:
fl.write(r.content)
try:
r = requests.get(url, timeout=timeout)
r.raise_for_status()
except requests.exceptions.RequestException as e:
if verbose: print("Failed to fetch %s: %s ... skipping."%(fl, e))
status.append(False)
continue
with open(fl, "wb") as f:
f.write(r.content)
status.append(True)

return status
53 changes: 53 additions & 0 deletions quantecon/util/tests/test_notebooks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@

from quantecon.util import fetch_nb_dependencies
import os
from unittest import mock

import requests

FILES = ['test_file.md']
REPO = "https://github.com/QuantEcon/QuantEcon.py"
Expand Down Expand Up @@ -39,3 +42,53 @@ def test_fetch_nb_dependencies_overwrite(self):

def teardown_method(self):
os.remove("test_file.md")


class TestFetchNbDependenciesErrors:

def test_http_error_is_not_written_to_disk(self, tmp_path, monkeypatch):
"""
A failed request must report False and leave no file behind, rather
than saving the server's error page under the requested filename.
"""
response = mock.Mock()
response.content = b"<html>404: Not Found</html>"
response.raise_for_status.side_effect = requests.exceptions.HTTPError(
"404 Client Error")

monkeypatch.chdir(tmp_path)
with mock.patch("requests.get", return_value=response):
status = fetch_nb_dependencies(["does-not-exist.csv"], verbose=False)

assert status == [False]
assert not os.path.isfile("does-not-exist.csv")

def test_request_uses_a_timeout(self, tmp_path, monkeypatch):
"""
Requests must not be able to block indefinitely.
"""
response = mock.Mock()
response.content = b"data"
response.raise_for_status.return_value = None

monkeypatch.chdir(tmp_path)
with mock.patch("requests.get", return_value=response) as get:
fetch_nb_dependencies(["a.csv"], verbose=False, timeout=5)

assert get.call_args.kwargs["timeout"] == 5

def test_tuple_of_files_is_accepted(self, tmp_path, monkeypatch):
"""
A tuple is the natural alternative to a list and must not be treated
as a directory mapping.
"""
response = mock.Mock()
response.content = b"data"
response.raise_for_status.return_value = None

monkeypatch.chdir(tmp_path)
with mock.patch("requests.get", return_value=response):
status = fetch_nb_dependencies(("a.csv",), verbose=False)

assert status == [True]
assert os.path.isfile("a.csv")