From 77a425df273f43c613015a71604813f5e03cd756 Mon Sep 17 00:00:00 2001 From: Gio <43544549+romer8@users.noreply.github.com> Date: Mon, 5 Jun 2023 15:27:14 -0600 Subject: [PATCH] Fix Custom JSON setting error when installing from file (#965) * fix for json error of getting long path instead of file not found * possible fix to test, not uncometting yet. * deleted comments and improved one of the tests * fixed the file installation bug for json setting --- environment.yml | 2 +- .../test_tethys_cli/test_install_commands.py | 126 +++++++++++++++--- tethys_cli/install_commands.py | 43 ++++-- 3 files changed, 142 insertions(+), 29 deletions(-) diff --git a/environment.yml b/environment.yml index 4c7bf2349..df603b59d 100644 --- a/environment.yml +++ b/environment.yml @@ -27,7 +27,7 @@ dependencies: # database dependencies - postgresql - psycopg2 - - sqlalchemy=1.* # TODO: what will it take to support sqlalchemy 2.0? + - sqlalchemy=1.* # TODO: what will it take to support sqlalchemy 2.0? - geoalchemy2 # plotting dependencies diff --git a/tests/unit_tests/test_tethys_cli/test_install_commands.py b/tests/unit_tests/test_tethys_cli/test_install_commands.py index 13655dd0c..c8b033366 100644 --- a/tests/unit_tests/test_tethys_cli/test_install_commands.py +++ b/tests/unit_tests/test_tethys_cli/test_install_commands.py @@ -406,19 +406,23 @@ def setUp(self): @mock.patch("tethys_cli.install_commands.get_app_settings") @mock.patch("tethys_cli.install_commands.find_and_link") @mock.patch("tethys_cli.cli_colors.pretty_output") + @mock.patch("tethys_cli.install_commands.json.loads") @mock.patch("tethys_cli.install_commands.json.load") @mock.patch( "tethys_cli.install_commands.open", new_callable=lambda: mock.mock_open(read_data='{"fake_json": "{}"}'), ) + @mock.patch("tethys_cli.install_commands.os.path.isfile") @mock.patch("tethys_apps.models.CustomSettingBase") @mock.patch("tethys_apps.models.TethysApp") def test_configure_services_from_file( self, mock_TethysApp, mock_CustomSetting, + mock_isfile, mock_open, mock_json_load, + mock_json_loads, mock_pretty_output, mock_find_and_link, mock_gas, @@ -431,12 +435,66 @@ def test_configure_services_from_file( json_custom_setting_name = "json_setting" json_custom_setting_value = "fake/path/to/file" - json_custom_setting_wrong_path_name = "wrong_path_json_setting" - json_custom_setting_wrong_path_value = {"fake_json": "fake_value"} - json_custom_setting_type_error_name = "type_error_json_setting" - json_custom_setting_type_error_value = {"json": "value"} - + json_custom_setting_wrong_path_value = '{"name": "John", "age": 30, "city": "New York", "interests": ["music", "sports", "reading"], "education": {"degree": "Bachelor", "major": "Computer Science", "university": "ABC University"}, "work_experience": [{"position": "Software Engineer", "company": "XYZ Inc.", "duration": "3 years"}, {"position": "Senior Developer", "company": "ABC Corp.", "duration": "2 years"}], "projects": [{"name": "Project A", "description": "Lorem ipsum dolor sit amet", "status": "completed"}, {"name": "Project B", "description": "Consectetur adipiscing elit", "status": "in progress"}], "family_members": [{"name": "Jane", "relationship": "Spouse", "age": 28}, {"name": "Sarah", "relationship": "Sister", "age": 32}, {"name": "Michael", "relationship": "Brother", "age": 26}], "address": {"street": "123 Main Street", "city": "New York", "state": "NY", "postal_code": "10001"}, "phone_numbers": {"home": "555-1234", "work": "555-5678", "cell": "555-9876"}}' + json_custom_setting_value_error_name = "value_error_json_setting" + json_custom_setting_value_error_value = '{"name": "John", "age": 30,}' + json_custom_setting_value_json_name = "json_custom_setting_which_is_json" + json_custom_setting_value_json_value = { + "name": "John", + "age": 30, + "city": "New York", + "interests": ["music", "sports", "reading"], + "education": { + "degree": "Bachelor", + "major": "Computer Science", + "university": "ABC University", + }, + "work_experience": [ + { + "position": "Software Engineer", + "company": "XYZ Inc.", + "duration": "3 years", + }, + { + "position": "Senior Developer", + "company": "ABC Corp.", + "duration": "2 years", + }, + ], + "projects": [ + { + "name": "Project A", + "description": "Lorem ipsum dolor sit amet", + "status": "completed", + }, + { + "name": "Project B", + "description": "Consectetur adipiscing elit", + "status": "in progress", + }, + ], + "family_members": [ + {"name": "Jane", "relationship": "Spouse", "age": 28}, + {"name": "Sarah", "relationship": "Sister", "age": 32}, + {"name": "Michael", "relationship": "Brother", "age": 26}, + ], + "address": { + "street": "123 Main Street", + "city": "New York", + "state": "NY", + "postal_code": "10001", + }, + "phone_numbers": { + "home": "555-1234", + "work": "555-5678", + "cell": "555-9876", + }, + } + json_custom_setting_not_dict_or_file_path_name = ( + "json_custom_setting_which_is_not_a_dict_or_file_path" + ) + json_custom_setting_not_dict_or_file_path_value = 2 secret_custom_setting_name = "secret_setting" secret_custom_setting_value = "SECRET:XXXX235235RSDGSDGAF_23523" @@ -453,7 +511,9 @@ def test_configure_services_from_file( "custom_setting_dne": 1, json_custom_setting_name: json_custom_setting_value, json_custom_setting_wrong_path_name: json_custom_setting_wrong_path_value, - json_custom_setting_type_error_name: json_custom_setting_type_error_value, + json_custom_setting_value_error_name: json_custom_setting_value_error_value, + json_custom_setting_value_json_name: json_custom_setting_value_json_value, + json_custom_setting_not_dict_or_file_path_name: json_custom_setting_not_dict_or_file_path_value, secret_custom_setting_name: secret_custom_setting_value, }, "persistent": { @@ -484,11 +544,20 @@ def test_configure_services_from_file( value=None, type_custom_setting="JSON" ) - # Json setting with type error - mock_json_custom_setting_type_error = mock.MagicMock( + # Json setting with value error + mock_json_custom_setting_value_error = mock.MagicMock( + value=None, type_custom_setting="JSON" + ) + + # Json setting with valid dict + mock_json_custom_setting_value_dict = mock.MagicMock( value=None, type_custom_setting="JSON" ) + # # json setting invalid value, in this case a number + mock_json_custom_setting_invalid_value = mock.MagicMock( + value=None, type_custom_setting="JSON" + ) # valid custom Secret setting mock_secret_custom_setting = mock.MagicMock( value=None, type_custom_setting="SECRET" @@ -500,12 +569,22 @@ def test_configure_services_from_file( ObjectDoesNotExist, #: Setting not found mock_json_custom_setting, mock_json_custom_setting_wrong_path, - mock_json_custom_setting_type_error, + mock_json_custom_setting_value_error, + mock_json_custom_setting_value_dict, + mock_json_custom_setting_invalid_value, mock_secret_custom_setting, ] - mock_open.side_effect = (mock_open.return_value, FileNotFoundError, TypeError) - mock_json_load.return_value = '{"fake_json": "{}"}' + # mock_open.side_effect = (mock_open.return_value, FileNotFoundError, TypeError) + mock_open.side_effect = mock_open.return_value + + mock_isfile.side_effect = (True, False, False) + + mock_json_loads.side_effect = [ + json_custom_setting_wrong_path_value, + ValueError, + ] + mock_json_load.side_effect = ['{"fake_json": "{}"}'] # This persistent setting exists and is listed in the file mock_persistent_database_setting = mock.MagicMock() @@ -533,7 +612,6 @@ def test_configure_services_from_file( ], } install_commands.configure_services_from_file(services_file_contents, app_name) - po_call_args = mock_pretty_output().__enter__().write.call_args_list self.assertIn( @@ -556,28 +634,40 @@ def test_configure_services_from_file( ) self.assertEqual( - "The current file path was not found, assuming you provided a valid JSON", + 'CustomSetting: "wrong_path_json_setting" was assigned the value: "{"name": "John", "age": 30, "city": "New York", "interests": ["music", "sports", "reading"], "education": {"degree": "Bachelor", "major": "Computer Science", "university": "ABC University"}, "work_experience": [{"position": "Software Engineer", "company": "XYZ Inc.", "duration": "3 years"}, {"position": "Senior Developer", "company": "ABC Corp.", "duration": "2 years"}], "projects": [{"name": "Project A", "description": "Lorem ipsum dolor sit amet", "status": "completed"}, {"name": "Project B", "description": "Consectetur adipiscing elit", "status": "in progress"}], "family_members": [{"name": "Jane", "relationship": "Spouse", "age": 28}, {"name": "Sarah", "relationship": "Sister", "age": 32}, {"name": "Michael", "relationship": "Brother", "age": 26}], "address": {"street": "123 Main Street", "city": "New York", "state": "NY", "postal_code": "10001"}, "phone_numbers": {"home": "555-1234", "work": "555-5678", "cell": "555-9876"}}"', po_call_args[4][0][0], ) self.assertEqual( - "CustomSetting: \"wrong_path_json_setting\" was assigned the value: \"{'fake_json': 'fake_value'}\"", + 'The current file path/JSON string: {"name": "John", "age": 30,} is not a file path or does not contain a valid JSONstring.', po_call_args[5][0][0], ) self.assertEqual( - "CustomSetting: \"type_error_json_setting\" was assigned the value: \"{'json': 'value'}\"", + 'Custom setting named "value_error_json_setting" is not valid in app "foo". Skipping...', po_call_args[6][0][0], ) self.assertEqual( - 'CustomSetting: "secret_setting" was assigned the value: "SECRET:XXXX235235RSDGSDGAF_23523"', + "CustomSetting: \"json_custom_setting_which_is_json\" was assigned the value: \"{'name': 'John', 'age': 30, 'city': 'New York', 'interests': ['music', 'sports', 'reading'], 'education': {'degree': 'Bachelor', 'major': 'Computer Science', 'university': 'ABC University'}, 'work_experience': [{'position': 'Software Engineer', 'company': 'XYZ Inc.', 'duration': '3 years'}, {'position': 'Senior Developer', 'company': 'ABC Corp.', 'duration': '2 years'}], 'projects': [{'name': 'Project A', 'description': 'Lorem ipsum dolor sit amet', 'status': 'completed'}, {'name': 'Project B', 'description': 'Consectetur adipiscing elit', 'status': 'in progress'}], 'family_members': [{'name': 'Jane', 'relationship': 'Spouse', 'age': 28}, {'name': 'Sarah', 'relationship': 'Sister', 'age': 32}, {'name': 'Michael', 'relationship': 'Brother', 'age': 26}], 'address': {'street': '123 Main Street', 'city': 'New York', 'state': 'NY', 'postal_code': '10001'}, 'phone_numbers': {'home': '555-1234', 'work': '555-5678', 'cell': '555-9876'}}\"", po_call_args[7][0][0], ) + self.assertEqual( + "The current value: 2 is not a dict or a valid file path", + po_call_args[8][0][0], + ) + self.assertEqual( + 'Custom setting named "json_custom_setting_which_is_not_a_dict_or_file_path" is not valid in app "foo". Skipping...', + po_call_args[9][0][0], + ) + self.assertEqual( + 'CustomSetting: "secret_setting" was assigned the value: "SECRET:XXXX235235RSDGSDGAF_23523"', + po_call_args[10][0][0], + ) self.assertEqual( f'No service given for setting "{no_val_persistent_setting_name}". Skipping...', - po_call_args[8][0][0], + po_call_args[11][0][0], ) self.assertIn( - "already configured or does not exist in app", po_call_args[9][0][0] + "already configured or does not exist in app", po_call_args[12][0][0] ) mock_find_and_link.assert_called_with( diff --git a/tethys_cli/install_commands.py b/tethys_cli/install_commands.py index 2daea3791..8b59ac885 100644 --- a/tethys_cli/install_commands.py +++ b/tethys_cli/install_commands.py @@ -2,9 +2,7 @@ import json import os import getpass - from pathlib import Path - from subprocess import call, Popen, PIPE, STDOUT from argparse import Namespace from django.core.exceptions import ObjectDoesNotExist, ValidationError @@ -23,6 +21,7 @@ ) from .gen_commands import download_vendor_static_files +from collections.abc import Mapping has_conda = False try: @@ -542,16 +541,15 @@ def configure_services_from_file(services, app_name): try: if custom_setting.type_custom_setting == "JSON": - try: - with open(current_services[setting_name]) as json_file: - custom_setting.value = json.load(json_file) - except TypeError: - custom_setting.value = current_services[setting_name] - except FileNotFoundError: - custom_setting.value = current_services[setting_name] + custom_setting.value = assign_json_value( + current_services[setting_name] + ) + if custom_setting.value is None: write_warning( - "The current file path was not found, assuming you provided a valid JSON" + f'Custom setting named "{setting_name}" is not valid in app "{app_name}". ' + f"Skipping..." ) + continue else: custom_setting.value = current_services[setting_name] custom_setting.clean() @@ -875,3 +873,28 @@ def validate_schema(check_str, check_list): def successful_exit(app_name, action="installed"): write_success(f"Successfully {action} {app_name}.") exit(0) + + +def assign_json_value(value): + # Check if the value is a file path + if isinstance(value, str): + try: + if os.path.isfile(value): + with open(value) as file: + json_data = json.load(file) + return json_data + else: + # Check if the value is a valid JSON string + json_data = json.loads(value) + return json_data + except ValueError: + write_error( + f"The current file path/JSON string: {value} is not a file path or does not contain a valid JSONstring." + ) + return None + if isinstance(value, Mapping): + # when the dict is read from the portal_config.yaml, if it is not a proper dict, the portal will fail, so it is not possible to test it + return value + else: + write_error(f"The current value: {value} is not a dict or a valid file path") + return None