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

@@ -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

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(

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