Refactor try/excepts in domain (#11421)

* Refactor try/excepts * Remove too wide try/excepts * Fix backend tests * Add six to .isort.cfg * Address comments * Fix tests * Fix linting in python_utils * Fix instalation * Add TODO comment * Fix backend tests
oppia · Dec 30, 2020 · a7646d6 · a7646d6
1 parent 92cf007
commit a7646d6
Show file tree

Hide file tree

Showing 23 changed files with 141 additions and 97 deletions.
diff --git a/.isort.cfg b/.isort.cfg
@@ -2,6 +2,6 @@
 force_single_line=true
 force_sort_within_sections=true
 ignore_whitespace=true
-known_third_party=backports.functools_lru_cache,browsermobproxy,cloudstorage,contextlib2,elasticsearch,firebase_admin,google.api_core,google.appengine,google.cloud,google.protobuf,jinja2,mapreduce,mutagen,pipeline,pylatexenc,pylint,requests,selenium,skulpt,webapp2,webapp2_extras,webtest,yaml
+known_third_party=backports.functools_lru_cache,browsermobproxy,cloudstorage,contextlib2,elasticsearch,firebase_admin,google.api_core,google.appengine,google.cloud,google.protobuf,jinja2,mapreduce,mutagen,pipeline,pylatexenc,pylint,requests,selenium,six,skulpt,webapp2,webapp2_extras,webtest,yaml
 line_length=80
 sections=FUTURE,STDLIB,FIRSTPARTY,THIRDPARTY,LOCALFOLDER
diff --git a/core/domain/collection_domain.py b/core/domain/collection_domain.py
@@ -526,20 +526,22 @@ def _migrate_to_latest_yaml_version(cls, yaml_content):
             schema format.
 
         Raises:
-            Exception. The 'yaml_content' or the collection schema version is
-                not valid.
+            InvalidInputException. The 'yaml_content' or the schema version
+                is not specified.
+            Exception. The collection schema version is not valid.
         """
         try:
             collection_dict = utils.dict_from_yaml(yaml_content)
-        except Exception as e:
-            raise Exception(
+        except utils.InvalidInputException as e:
+            raise utils.InvalidInputException(
                 'Please ensure that you are uploading a YAML text file, not '
                 'a zip file. The YAML parser returned the following error: %s'
                 % e)
 
         collection_schema_version = collection_dict.get('schema_version')
         if collection_schema_version is None:
-            raise Exception('Invalid YAML file: no schema version specified.')
+            raise utils.InvalidInputException(
+                'Invalid YAML file: no schema version specified.')
         if not (1 <= collection_schema_version
                 <= feconf.CURRENT_COLLECTION_SCHEMA_VERSION):
             raise Exception(

diff --git a/core/domain/collection_domain_test.py b/core/domain/collection_domain_test.py
@@ -643,10 +643,11 @@ def test_yaml_import_and_export(self):
 
         # Should not be able to create a collection from no YAML content.
         with self.assertRaisesRegexp(
-            Exception, 'Please ensure that you are uploading a YAML text file, '
+            utils.InvalidInputException,
+            'Please ensure that you are uploading a YAML text file, '
             'not a zip file. The YAML parser returned the following error: '
-            '\'NoneType\' object has no attribute \'read\''):
-            collection_domain.Collection.from_yaml('collection3', None)
+        ):
+            collection_domain.Collection.from_yaml('collection3', '')
 
     def test_from_yaml_with_no_schema_version_specified_raises_error(self):
         collection = collection_domain.Collection(

diff --git a/core/domain/collection_services.py b/core/domain/collection_services.py
@@ -636,10 +636,12 @@ def apply_change_list(collection_id, change_list):
         Collection. The resulting collection domain object.
     """
     collection = get_collection_by_id(collection_id)
-    try:
-        changes = [collection_domain.CollectionChange(change_dict)
-                   for change_dict in change_list]
 
+    try:
+        changes = [
+            collection_domain.CollectionChange(change_dict)
+            for change_dict in change_list
+        ]
         for change in changes:
             if change.cmd == collection_domain.CMD_ADD_COLLECTION_NODE:
                 collection.add_node(change.exploration_id)
@@ -670,14 +672,15 @@ def apply_change_list(collection_id, change_list):
                 # latest schema version. As a result, simply resaving the
                 # collection is sufficient to apply the schema migration.
                 continue
+
         return collection
 
     except Exception as e:
         logging.error(
             '%s %s %s %s' % (
                 e.__class__.__name__, e, collection_id, change_list)
         )
-        raise
+        python_utils.reraise_exception()
 
 
 def validate_exps_in_collection_are_public(collection):

diff --git a/core/domain/collection_services_test.py b/core/domain/collection_services_test.py
@@ -407,8 +407,7 @@ def _mock_logging_function(msg, *_):
 
         logging_swap = self.swap(logging, 'error', _mock_logging_function)
 
-        self.save_new_valid_collection(
-            'collection_id', self.owner_id)
+        self.save_new_valid_collection('collection_id', self.owner_id)
 
         with self.assertRaisesRegexp(
             Exception, 'Command invalid command is not allowed'), logging_swap:

diff --git a/core/domain/email_manager.py b/core/domain/email_manager.py
@@ -629,26 +629,27 @@ def get_moderator_unpublish_exploration_email():
 
     try:
         require_moderator_email_prereqs_are_satisfied()
-        return config_domain.Registry.get_config_property(
-            'unpublish_exploration_email_html_body').value
-    except Exception:
+    except utils.ValidationError:
         return ''
 
+    return config_domain.Registry.get_config_property(
+        'unpublish_exploration_email_html_body').value
+
 
 def require_moderator_email_prereqs_are_satisfied():
     """Raises an exception if, for any reason, moderator emails cannot be sent.
 
     Raises:
-        Exception. The feconf.REQUIRE_EMAIL_ON_MODERATOR_ACTION is False.
-        Exception. The feconf.CAN_SEND_EMAILS is False.
+        ValidationError. The feconf.REQUIRE_EMAIL_ON_MODERATOR_ACTION is False.
+        ValidationError. The feconf.CAN_SEND_EMAILS is False.
     """
 
     if not feconf.REQUIRE_EMAIL_ON_MODERATOR_ACTION:
-        raise Exception(
+        raise utils.ValidationError(
             'For moderator emails to be sent, please ensure that '
             'REQUIRE_EMAIL_ON_MODERATOR_ACTION is set to True.')
     if not feconf.CAN_SEND_EMAILS:
-        raise Exception(
+        raise utils.ValidationError(
             'For moderator emails to be sent, please ensure that '
             'CAN_SEND_EMAILS is set to True.')
 

diff --git a/core/domain/exp_domain.py b/core/domain/exp_domain.py
@@ -4378,21 +4378,23 @@ def _migrate_to_latest_yaml_version(
             schema version provided in 'yaml_content'.
 
         Raises:
-            Exception. The 'yaml_content' or the exploration schema version is
-                not valid.
+            InvalidInputException. The 'yaml_content' or the schema version
+                is not specified.
+            Exception. The exploration schema version is not valid.
         """
         try:
             exploration_dict = utils.dict_from_yaml(yaml_content)
-        except Exception as e:
-            raise Exception(
+        except utils.InvalidInputException as e:
+            raise utils.InvalidInputException(
                 'Please ensure that you are uploading a YAML text file, not '
                 'a zip file. The YAML parser returned the following error: %s'
                 % e)
 
         exploration_schema_version = exploration_dict.get('schema_version')
         initial_schema_version = exploration_schema_version
         if exploration_schema_version is None:
-            raise Exception('Invalid YAML file: no schema version specified.')
+            raise utils.InvalidInputException(
+                'Invalid YAML file: no schema version specified.')
         if not (1 <= exploration_schema_version
                 <= cls.CURRENT_EXP_SCHEMA_VERSION):
             raise Exception(

diff --git a/core/domain/exp_jobs_one_off.py b/core/domain/exp_jobs_one_off.py
@@ -199,7 +199,8 @@ def map(item):
         while states_schema_version < current_state_schema_version:
             try:
                 exp_domain.Exploration.update_states_from_model(
-                    versioned_exploration_states, states_schema_version,
+                    versioned_exploration_states,
+                    states_schema_version,
                     item.id)
                 states_schema_version += 1
             except Exception as e:

diff --git a/core/domain/exp_services.py b/core/domain/exp_services.py
@@ -31,7 +31,6 @@
 import math
 import os
 import pprint
-import traceback
 import zipfile
 
 from constants import constants
@@ -414,8 +413,10 @@ def apply_change_list(exploration_id, change_list):
                         raise Exception(
                             'Expected hints_list to be a list,'
                             ' received %s' % change.new_value)
-                    new_hints_list = [state_domain.Hint.from_dict(hint_dict)
-                                      for hint_dict in change.new_value]
+                    new_hints_list = [
+                        state_domain.Hint.from_dict(hint_dict)
+                        for hint_dict in change.new_value
+                    ]
                     state.update_interaction_hints(new_hints_list)
                 elif (change.property_name ==
                       exp_domain.STATE_PROPERTY_INTERACTION_SOLUTION):
@@ -489,19 +490,17 @@ def apply_change_list(exploration_id, change_list):
                 elif change.property_name == 'param_specs':
                     exploration.update_param_specs(change.new_value)
                 elif change.property_name == 'param_changes':
-                    exploration.update_param_changes(
-                        list(python_utils.MAP(
-                            to_param_domain, change.new_value)))
+                    exploration.update_param_changes(list(
+                        python_utils.MAP(to_param_domain, change.new_value)))
                 elif change.property_name == 'init_state_name':
                     exploration.update_init_state_name(change.new_value)
                 elif change.property_name == 'auto_tts_enabled':
                     exploration.update_auto_tts_enabled(change.new_value)
                 elif change.property_name == 'correctness_feedback_enabled':
                     exploration.update_correctness_feedback_enabled(
                         change.new_value)
-            elif (
-                    change.cmd ==
-                    exp_domain.CMD_MIGRATE_STATES_SCHEMA_TO_LATEST_VERSION):
+            elif (change.cmd ==
+                  exp_domain.CMD_MIGRATE_STATES_SCHEMA_TO_LATEST_VERSION):
                 # Loading the exploration model from the datastore into an
                 # Exploration domain object automatically converts it to use
                 # the latest states schema version. As a result, simply
@@ -526,8 +525,7 @@ def apply_change_list(exploration_id, change_list):
                 e.__class__.__name__, e, exploration_id,
                 pprint.pprint(change_list))
         )
-        logging.error(traceback.format_exc())
-        raise
+        python_utils.reraise_exception()
 
 
 def _save_exploration(committer_id, exploration, commit_message, change_list):

diff --git a/core/domain/html_validation_service.py b/core/domain/html_validation_service.py
@@ -34,6 +34,7 @@
 from extensions.rich_text_components import components
 import feconf
 import python_utils
+import utils
 
 
 def escape_html(unescaped_html_data):
@@ -936,7 +937,7 @@ def validate_math_content_attribute_in_html(html_string):
             components.Math.validate({
                 'math_content-with-value': math_content_dict
             })
-        except Exception as e:
+        except utils.ValidationError as e:
             error_list.append({
                 'invalid_tag': python_utils.UNICODE(math_tag),
                 'error': python_utils.UNICODE(e)

diff --git a/core/domain/question_services.py b/core/domain/question_services.py
@@ -571,7 +571,7 @@ def apply_change_list(question_id, change_list):
             '%s %s %s %s' % (
                 e.__class__.__name__, e, question_id, change_list)
         )
-        raise
+        python_utils.reraise_exception()
 
 
 def _save_question(committer_id, question, change_list, commit_message):

diff --git a/core/domain/rating_services.py b/core/domain/rating_services.py
@@ -56,10 +56,10 @@ def assign_rating_to_exploration(user_id, exploration_id, new_rating):
     if new_rating not in ALLOWED_RATINGS:
         raise ValueError('Expected a rating 1-5, received %s.' % new_rating)
 
-    try:
-        exp_fetchers.get_exploration_by_id(exploration_id)
-    except:
-        raise Exception('Invalid exploration id %s' % exploration_id)
+    exploration = exp_fetchers.get_exploration_by_id(
+        exploration_id, strict=False)
+    if exploration is None:
+        raise ValueError('Invalid exploration id %s' % exploration_id)
 
     def _update_user_rating():
         """Updates the user rating of the exploration. Returns the old rating

diff --git a/core/domain/recommendations_services.py b/core/domain/recommendations_services.py
@@ -235,7 +235,7 @@ def validate_topic_similarities(data):
             similarity = topic_similarities_values[row_ind][col_ind]
             try:
                 similarity = float(similarity)
-            except:
+            except ValueError:
                 raise ValueError(
                     'Expected similarity to be a float, received %s' % (
                         similarity))

diff --git a/core/domain/skill_jobs_one_off_test.py b/core/domain/skill_jobs_one_off_test.py
@@ -199,14 +199,14 @@ def test_migration_job_converts_old_skill(self):
         self.assertEqual(expected, [ast.literal_eval(x) for x in output])
 
     def test_migration_job_skips_updated_skill_failing_validation(self):
-
-        def _mock_get_skill_by_id(unused_skill_id):
-            """Mocks get_skill_by_id()."""
-            return 'invalid_skill'
-
         skill = skill_domain.Skill.create_default_skill(
             self.SKILL_ID, 'A description', self.rubrics)
         skill_services.save_new_skill(self.albert_id, skill)
+        skill.description = ''
+
+        def _mock_get_skill_by_id(unused_skill_id):
+            """Mocks get_skill_by_id()."""
+            return skill
 
         get_skill_by_id_swap = self.swap(
             skill_fetchers, 'get_skill_by_id', _mock_get_skill_by_id)
@@ -223,9 +223,9 @@ def _mock_get_skill_by_id(unused_skill_id):
         # If the skill had been successfully migrated, this would include a
         # 'successfully migrated' message. Its absence means that the skill
         # could not be processed.
-        expected = [[u'validation_error',
-                     [u'Skill %s failed validation: \'unicode\' object has '
-                      'no attribute \'validate\'' % (self.SKILL_ID)]]]
+        expected = [['validation_error',
+                     ['Skill %s failed validation: Description '
+                      'field should not be empty' % (self.SKILL_ID)]]]
         self.assertEqual(
             expected, [ast.literal_eval(x) for x in output])
 

diff --git a/core/domain/skill_services.py b/core/domain/skill_services.py
@@ -658,7 +658,7 @@ def apply_change_list(skill_id, change_list, committer_id):
             '%s %s %s %s' % (
                 e.__class__.__name__, e, skill_id, change_list)
         )
-        raise
+        python_utils.reraise_exception()
 
 
 def _save_skill(committer_id, skill, commit_message, change_list):

diff --git a/core/domain/story_jobs_one_off_test.py b/core/domain/story_jobs_one_off_test.py
@@ -221,17 +221,18 @@ def test_migration_job_converts_old_story(self):
         self.assertEqual(expected, [ast.literal_eval(x) for x in output])
 
     def test_migration_job_skips_updated_story_failing_validation(self):
-
-        def _mock_get_story_by_id(unused_story_id):
-            """Mocks get_story_by_id()."""
-            return 'invalid_story'
-
         story = story_domain.Story.create_default_story(
             self.STORY_ID, 'A title', 'Description', self.TOPIC_ID,
             'title-three')
         story_services.save_new_story(self.albert_id, story)
         topic_services.add_canonical_story(
             self.albert_id, self.TOPIC_ID, story.id)
+        story.description = 123
+
+        def _mock_get_story_by_id(unused_story_id):
+            """Mocks get_story_by_id()."""
+            return story
+
         get_story_by_id_swap = self.swap(
             story_fetchers, 'get_story_by_id', _mock_get_story_by_id)
 
@@ -241,14 +242,14 @@ def _mock_get_story_by_id(unused_story_id):
             story_jobs_one_off.StoryMigrationOneOffJob.enqueue(job_id)
             self.process_and_flush_pending_mapreduce_tasks()
 
-        output = story_jobs_one_off.StoryMigrationOneOffJob.get_output(
-            job_id)
+        output = story_jobs_one_off.StoryMigrationOneOffJob.get_output(job_id)
 
         # If the story had been successfully migrated, this would include a
         # 'successfully migrated' message. Its absence means that the story
         # could not be processed.
         for x in output:
-            self.assertRegexpMatches(x, 'object has no attribute \'validate\'')
+            self.assertRegexpMatches(
+                x, 'Expected description to be a string, received 123')
 
 
 class RegenerateStorySummaryOneOffJobTests(test_utils.GenericTestBase):

diff --git a/core/domain/story_services.py b/core/domain/story_services.py
@@ -38,6 +38,7 @@
 from core.domain import topic_fetchers
 from core.platform import models
 import feconf
+import python_utils
 import utils
 
 (exp_models, story_models, user_models,) = models.Registry.import_models(
@@ -219,7 +220,7 @@ def apply_change_list(story_id, change_list):
             '%s %s %s %s' % (
                 e.__class__.__name__, e, story_id, change_list)
         )
-        raise
+        python_utils.reraise_exception()
 
 
 def does_story_exist_with_url_fragment(url_fragment):

diff --git a/core/domain/topic_services.py b/core/domain/topic_services.py
@@ -303,6 +303,7 @@ def apply_change_list(topic_id, change_list):
             topic_id, existing_subtopic_page_ids_to_be_modified))
     for subtopic_page in modified_subtopic_pages_list:
         modified_subtopic_pages[subtopic_page.id] = subtopic_page
+
     try:
         for change in change_list:
             if change.cmd == topic_domain.CMD_ADD_SUBTOPIC:
@@ -451,7 +452,7 @@ def apply_change_list(topic_id, change_list):
             '%s %s %s %s' % (
                 e.__class__.__name__, e, topic_id, change_list)
         )
-        raise
+        python_utils.reraise_exception()
 
 
 def _save_topic(committer_id, topic, commit_message, change_list):