From f810241d3bc40f75b78870c9800eeb6b0db22c15 Mon Sep 17 00:00:00 2001 From: Benito Matischen Date: Wed, 3 Sep 2025 14:07:58 +0100 Subject: [PATCH 1/8] tests: Describe codes to return booleans or errors for --- tests/test_clitool.py | 47 ++++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/tests/test_clitool.py b/tests/test_clitool.py index 5c87575..6aadd44 100644 --- a/tests/test_clitool.py +++ b/tests/test_clitool.py @@ -262,27 +262,36 @@ def test_call_api_handle_429_errors(self, mock_request_class, mock_urlopen): assert len(mock_urlopen.call_args_list) == 5 def test_get_public_status(self): - # Public url mock_response = MagicMock() - mock_response.status = 200 - with patch('osfexport.exporter.call_api') as mock_call_api: - mock_call_api.return_value = mock_response - result = is_public('url') - mock_call_api.assert_called_once_with( - 'url', pat='', method='GET' - ) - assert result is True - # Private url - mock_response = MagicMock() - mock_response.status = 401 - with patch('osfexport.exporter.call_api') as mock_call_api: - mock_call_api.return_value = mock_response - result = is_public('url') - mock_call_api.assert_called_once_with( - 'url', pat='', method='GET' - ) - assert result is False + valid_codes = [(200, True), (401, False), (403, False)] + for pair in valid_codes: + mock_response.status = pair[0] + with patch('osfexport.exporter.call_api') as mock_call_api: + mock_call_api.return_value = mock_response + result = is_public('url') + mock_call_api.assert_called_once_with( + 'url', pat='', method='GET' + ) + assert result == pair[1] + + bad_codes = [400, 500, -1] + for code in bad_codes: + with patch('osfexport.exporter.call_api') as mock_call_api: + if code != -1: + mock_call_api.side_effect = urllib.error.HTTPError( + url='https://test.osf.io', + code=code, + msg='HTTP Error', + hdrs={}, + fp=None + ) + else: + mock_call_api.side_effect = urllib.error.URLError( + reason="URL Error" + ) + with self.assertRaises(type(mock_call_api.side_effect)): + result = is_public('url') def test_explore_mock_file_tree(self): files = explore_file_tree( From e60e456fcdb1fe5661c81cdf457ecaa3b53dfc03 Mon Sep 17 00:00:00 2001 From: Benito Matischen Date: Wed, 3 Sep 2025 14:18:30 +0100 Subject: [PATCH 2/8] fix: Raise appropriate errors in is_public --- src/osfexport/exporter.py | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/osfexport/exporter.py b/src/osfexport/exporter.py index 8bd6b9f..540d284 100644 --- a/src/osfexport/exporter.py +++ b/src/osfexport/exporter.py @@ -2,7 +2,7 @@ import json import os import datetime -from urllib.error import HTTPError +from urllib.error import HTTPError, URLError import urllib.request as webhelper import importlib.metadata import time @@ -181,15 +181,31 @@ def is_public(url): ------------ url: str The URL to test. + + Returns + ---------------- + is_public: bool + Whether we can access the URL without a PAT (i.e. status code 200) + + Raises + ------------------- + HTTPError, URLError + If we get a HTTP error code that isn't 401/403, or a connection error. """ try: result = call_api( url, pat='', method='GET' ).status - except webhelper.HTTPError as e: - result = e - return result == 200 + except (webhelper.HTTPError, URLError) as e: + # Don't raise error if we get a HTTP error with certain codes + valid_error_codes = [401, 403] + if isinstance(e, webhelper.HTTPError) and e.code in valid_error_codes: + result = e.code + else: + raise e + is_public = result == 200 + return is_public def call_api( From 85e3cbd0755ee2375c6308f0c8eae76fbfe473a8 Mon Sep 17 00:00:00 2001 From: Benito Matischen Date: Wed, 3 Sep 2025 14:27:12 +0100 Subject: [PATCH 3/8] tests: Check error handling for errors from prompt_pat prompt_pat will get errors from is_public method. --- tests/test_clitool.py | 36 +++++++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/tests/test_clitool.py b/tests/test_clitool.py index 6aadd44..36976d8 100644 --- a/tests/test_clitool.py +++ b/tests/test_clitool.py @@ -1048,8 +1048,9 @@ def test_prompt_pat_if_exporting_all_projects(self, mock_obj): @patch('osfexport.cli.prompt_pat') @patch('osfexport.exporter.get_nodes') def test_export_projects_handles_http_url_errors(self, mock_func, mock_prompt): - codes = [401, 402, 403, 404, 429, 500, -1] - for code in codes: + # Handle errors from exporting nodes + export_codes = [401, 402, 403, 404, 429, 500, -1] + for code in export_codes: mock_prompt.return_value = '-' if code != -1: mock_func.side_effect = urllib.error.HTTPError( @@ -1071,7 +1072,36 @@ def test_export_projects_handles_http_url_errors(self, mock_func, mock_prompt): ], terminal_width=60 ) - assert "Exporting failed as an error occurred:" in result.output + assert "Exporting failed as an error occurred:" in result.output, ( + result.output + ) + + # Handle errors from is_public call when prompting for PAT + prompt_codes = [404, 500, -1] + for code in export_codes: + if code != -1: + mock_prompt.side_effect = urllib.error.HTTPError( + url='https://test.osf.io', + code=code, + msg='HTTP Error', + hdrs={}, + fp=None + ) + else: + mock_prompt.side_effect = urllib.error.URLError( + reason="URL Error" + ) + runner = CliRunner() + result = runner.invoke( + cli, [ + 'export-projects', + '--usetest' + ], + terminal_width=60 + ) + assert "Exporting failed as an error occurred:" in result.output, ( + result.output + ) def test_pull_projects_command_on_mocks(self): """Test generating a PDF from parsed project data. From b36d5b4f57a5529a0883570af248b4f43330e18f Mon Sep 17 00:00:00 2001 From: Benito Matischen Date: Wed, 3 Sep 2025 14:30:01 +0100 Subject: [PATCH 4/8] fix: Handle errors from prompt_pat raised by is_public method --- src/osfexport/cli.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/osfexport/cli.py b/src/osfexport/cli.py index 8147262..2a4f62c 100644 --- a/src/osfexport/cli.py +++ b/src/osfexport/cli.py @@ -26,6 +26,10 @@ def prompt_pat(project_id='', usetest=False): ----------------- pat: str Personal Access Token to use to authorise a user. + + Raises + ------------------- + HTTPError, URLError - passed on from is_public method. """ if usetest: @@ -81,11 +85,12 @@ def export_projects(folder, pat='', dryrun=False, url='', usetest=False): else: click.echo('No project ID provided, extracting all projects.') - if not pat and not dryrun: - pat = prompt_pat(project_id=project_id, usetest=usetest) - - click.echo('Downloading project data...') try: + if not pat and not dryrun: + pat = prompt_pat(project_id=project_id, usetest=usetest) + + click.echo('Downloading project data...') + projects, root_nodes = exporter.get_nodes( pat, dryrun=dryrun, project_id=project_id, usetest=usetest ) From 1152e7cfac70ef65eeecd37c0e2d04b658eb718f Mon Sep 17 00:00:00 2001 From: Benito Matischen Date: Wed, 3 Sep 2025 14:35:29 +0100 Subject: [PATCH 5/8] chore: Fix linting errors --- src/osfexport/cli.py | 2 +- src/osfexport/exporter.py | 2 +- tests/test_clitool.py | 5 ++--- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/osfexport/cli.py b/src/osfexport/cli.py index 2a4f62c..bb50f97 100644 --- a/src/osfexport/cli.py +++ b/src/osfexport/cli.py @@ -26,7 +26,7 @@ def prompt_pat(project_id='', usetest=False): ----------------- pat: str Personal Access Token to use to authorise a user. - + Raises ------------------- HTTPError, URLError - passed on from is_public method. diff --git a/src/osfexport/exporter.py b/src/osfexport/exporter.py index 540d284..241e053 100644 --- a/src/osfexport/exporter.py +++ b/src/osfexport/exporter.py @@ -186,7 +186,7 @@ def is_public(url): ---------------- is_public: bool Whether we can access the URL without a PAT (i.e. status code 200) - + Raises ------------------- HTTPError, URLError diff --git a/tests/test_clitool.py b/tests/test_clitool.py index 36976d8..3d360a7 100644 --- a/tests/test_clitool.py +++ b/tests/test_clitool.py @@ -274,7 +274,7 @@ def test_get_public_status(self): 'url', pat='', method='GET' ) assert result == pair[1] - + bad_codes = [400, 500, -1] for code in bad_codes: with patch('osfexport.exporter.call_api') as mock_call_api: @@ -1075,9 +1075,8 @@ def test_export_projects_handles_http_url_errors(self, mock_func, mock_prompt): assert "Exporting failed as an error occurred:" in result.output, ( result.output ) - + # Handle errors from is_public call when prompting for PAT - prompt_codes = [404, 500, -1] for code in export_codes: if code != -1: mock_prompt.side_effect = urllib.error.HTTPError( From 1daf8740bccce759ffd026a96515ecf7520b11b5 Mon Sep 17 00:00:00 2001 From: Benito Matischen Date: Wed, 3 Sep 2025 15:00:27 +0100 Subject: [PATCH 6/8] refactor: Simplify HTTPError import --- src/osfexport/exporter.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/osfexport/exporter.py b/src/osfexport/exporter.py index 241e053..ef8b83a 100644 --- a/src/osfexport/exporter.py +++ b/src/osfexport/exporter.py @@ -197,10 +197,10 @@ def is_public(url): result = call_api( url, pat='', method='GET' ).status - except (webhelper.HTTPError, URLError) as e: + except (HTTPError, URLError) as e: # Don't raise error if we get a HTTP error with certain codes valid_error_codes = [401, 403] - if isinstance(e, webhelper.HTTPError) and e.code in valid_error_codes: + if isinstance(e, HTTPError) and e.code in valid_error_codes: result = e.code else: raise e @@ -719,7 +719,7 @@ def get_project_data(nodes, **kwargs): parent['data']['attributes']['title'], parent['data']['links']['html'] ) - except (webhelper.HTTPError, ValueError): + except (HTTPError, ValueError): click.echo(f"Failed to load parent for {project_data['metadata']['title']}") click.echo("Try to give a PAT beforehand using the --pat flag.", "\n") From 35d1c65bedba45564f89669976e989776115fe5a Mon Sep 17 00:00:00 2001 From: Benito Matischen Date: Wed, 3 Sep 2025 15:00:51 +0100 Subject: [PATCH 7/8] refactor: Show expected/actual results for new tests --- tests/test_clitool.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/test_clitool.py b/tests/test_clitool.py index 3d360a7..ba6eeaa 100644 --- a/tests/test_clitool.py +++ b/tests/test_clitool.py @@ -273,7 +273,10 @@ def test_get_public_status(self): mock_call_api.assert_called_once_with( 'url', pat='', method='GET' ) - assert result == pair[1] + assert result == pair[1], ( + f"Expected: {pair}" + f"Actual code: {result}" + ) bad_codes = [400, 500, -1] for code in bad_codes: @@ -292,6 +295,7 @@ def test_get_public_status(self): ) with self.assertRaises(type(mock_call_api.side_effect)): result = is_public('url') + print(f"Code {code} should have failed by now!") def test_explore_mock_file_tree(self): files = explore_file_tree( From a5de019228260a1a6f54864f8b13f4050c9fb726 Mon Sep 17 00:00:00 2001 From: Benito Matischen Date: Wed, 3 Sep 2025 15:04:00 +0100 Subject: [PATCH 8/8] docs: Explain purpose of is_public method --- src/osfexport/exporter.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/osfexport/exporter.py b/src/osfexport/exporter.py index ef8b83a..87ee0f0 100644 --- a/src/osfexport/exporter.py +++ b/src/osfexport/exporter.py @@ -176,6 +176,7 @@ def get_host(is_test): def is_public(url): """Return boolean to indicate if a URL is public (True) or not (False). + This is mainly used for checking if a project is publicly accessible. Parameters ------------