Fixing artifact REST api

qiita_db/handlers/artifact.py

@@ -6,6 +6,8 @@
 # The full license is in the file LICENSE, distributed with this software.
 # -----------------------------------------------------------------------------
 
+from tornado.web import HTTPError
+
 import qiita_db as qdb
 from .oauth2 import OauthBaseHandler, authenticate_oauth
 
@@ -20,19 +22,25 @@ def _get_artifact(a_id):
 
     Returns
     -------
-    qiita_db.artifact.Artifact, bool, string
-        The requested artifact or None
-        Whether if we could get the artifact or not
-        Error message in case we couldn't get the artifact
+    qiita_db.artifact.Artifact
+        The requested artifact
+
+    Raises
+    ------
+    HTTPError
+        If the artifact does not exist, with error code 404
+        If there is a problem instantiating the artifact, with error code 500
     """
     try:
+        a_id = int(a_id)
         artifact = qdb.artifact.Artifact(a_id)
     except qdb.exceptions.QiitaDBUnknownIDError:
-        return None, False, 'Artifact does not exist'
-    except qdb.exceptions.QiitaDBError as e:
-        return None, False, 'Error instantiating the artifact: %s' % str(e)
+        raise HTTPError(404)
+    except Exception as e:
+        raise HTTPError(500, 'Error instantiating artifact %s: %s'
+                             % (a_id, str(e)))
 
-    return artifact, True, ''
+    return artifact
 
 
 class ArtifactFilepathsHandler(OauthBaseHandler):
@@ -49,23 +57,14 @@ def get(self, artifact_id):
         Returns
         -------
         dict
-            Format:
-            {'success': bool,
-             'error': str,
-             'filepaths': list of (str, str)}
-            - success: whether the request is successful or not
-            - error: in case that success is false, it contains the error msg
-            - filepaths: the filepaths attached to the artifact and their
-            filepath types
+            {'filepaths': list of (str, str)}
+            The filepaths attached to the artifact and their filepath types
         """
         with qdb.sql_connection.TRN:
-            artifact, success, error_msg = _get_artifact(artifact_id)
-            fps = None
-            if success:
-                fps = [(fp, fp_type) for _, fp, fp_type in artifact.filepaths]
-
-            response = {'success': success, 'error': error_msg,
-                        'filepaths': fps}
+            artifact = _get_artifact(artifact_id)
+            response = {
+                'filepaths': [(fp, fp_type)
+                              for _, fp, fp_type in artifact.filepaths]}
 
         self.write(response)
 
@@ -77,15 +76,6 @@ def patch(self, artifact_id):
         ---------
         artifact_id : str
             The id of the artifact whose filepaths information is being updated
-
-        Returns
-        -------
-        dict
-            Format:
-            {'success': bool,
-             'error': str}
-            - success: whether the request is successful or not
-            - error: in case that success is false, it contains the error msg
         """
         req_op = self.get_argument('op')
         req_path = self.get_argument('path')
@@ -94,20 +84,18 @@ def patch(self, artifact_id):
         if req_op == 'add':
             req_path = [v for v in req_path.split('/') if v]
             if len(req_path) != 1 or req_path[0] != 'html_summary':
-                success = False
-                error_msg = 'Incorrect path parameter value'
+                raise HTTPError(400, 'Incorrect path parameter value')
             else:
-                artifact, success, error_msg = _get_artifact(artifact_id)
-                if success:
+                artifact = _get_artifact(artifact_id)
+                try:
                     artifact.html_summary_fp = req_value
+                except Exception as e:
+                    raise HTTPError(500, str(e))
         else:
-            success = False
-            error_msg = ('Operation "%s" not supported. Current supported '
-                         'operations: add' % req_op)
+            raise HTTPError(400, 'Operation "%s" not supported. Current '
+                                 'supported operations: add' % req_op)
 
-        response = {'success': success, 'error': error_msg}
-
-        self.write(response)
+        self.finish()
 
 
 class ArtifactMappingHandler(OauthBaseHandler):
@@ -124,32 +112,24 @@ def get(self, artifact_id):
         Returns
         -------
         dict
-            Format:
-            {'success': bool,
-             'error': str,
-             'mapping': str}
-             - success: whether the request is successful or not
-             - error: in case that success is false, it contains the error msg
-             - mapping: the filepath to the mapping file
+            {'mapping': str}
+            The filepath to the mapping file
         """
         with qdb.sql_connection.TRN:
-            artifact, success, error_msg = _get_artifact(artifact_id)
-            fp = None
-            if success:
-                # In the current system, we don't have any artifact that
-                # is the result of two other artifacts, and there is no way
-                # of generating such artifact. This operation will be
-                # eventually supported, but in interest of time we are not
-                # going to implement that here.
-                prep_templates = artifact.prep_templates
-                if len(prep_templates) > 1:
-                    raise NotImplementedError(
-                        "Artifact %d has more than one prep template")
-
-                fp = prep_templates[0].qiime_map_fp
-
-            response = {'success': success, 'error': error_msg,
-                        'mapping': fp}
+            artifact = _get_artifact(artifact_id)
+            # In the current system, we don't have any artifact that
+            # is the result of two other artifacts, and there is no way
+            # of generating such artifact. This operation will be
+            # eventually supported, but in interest of time we are not
+            # going to implement that here.
+            prep_templates = artifact.prep_templates
+            if len(prep_templates) > 1:
+                raise NotImplementedError(
+                    "Artifact %d has more than one prep template")
+
+            fp = prep_templates[0].qiime_map_fp
+
+            response = {'mapping': fp}
 
         self.write(response)
 
@@ -167,21 +147,11 @@ def get(self, artifact_id):
         Returns
         -------
         dict
-            Format:
-            {'success': bool,
-             'error': str,
-             'type': str}
-            - success: whether the request is successful or not
-            - error: in case that success is false, it contains the error msg
-            - type: the artifact type
+            {'type': str}
+            The artifact type
         """
         with qdb.sql_connection.TRN:
-            artifact, success, error_msg = _get_artifact(artifact_id)
-            atype = None
-            if success:
-                atype = artifact.artifact_type
-
-            response = {'success': success, 'error': error_msg,
-                        'type': atype}
+            artifact = _get_artifact(artifact_id)
+            response = {'type': artifact.artifact_type}
 
         self.write(response)

qiita_db/handlers/oauth2.py

@@ -133,8 +133,8 @@ def write_error(self, status_code, **kwargs):
             return
         # log the error
         exc_info = kwargs['exc_info']
-        trace_info = ''.join(['%s\n' % line for line in
-                             format_exception(*exc_info)])
+        error_lines = ['%s\n' % line for line in format_exception(*exc_info)]
+        trace_info = ''.join(error_lines)
         req_dict = self.request.__dict__
         # must trim body to 1024 chars to prevent huge error messages
         req_dict['body'] = req_dict.get('body', '')[:1024]
@@ -147,6 +147,8 @@ def write_error(self, status_code, **kwargs):
             'ERROR:\n%s\nTRACE:\n%s\nHTTP INFO:\n%s\n' %
             (error, trace_info, request_info))
 
+        self.finish(exc_info[1].log_message)
+
     def head(self):
         """Adds proper response for head requests"""
         self.finish()

qiita_db/handlers/tests/test_artifact.py

@@ -9,30 +9,33 @@
 from unittest import main, TestCase
 from json import loads
 from functools import partial
-from os.path import join
+from os.path import join, exists
+from os import close, remove
+from tempfile import mkstemp
+
+from tornado.web import HTTPError
 
-from qiita_core.util import qiita_test_checker
 from qiita_db.handlers.tests.oauthbase import OauthTestingBase
 import qiita_db as qdb
 from qiita_db.handlers.artifact import _get_artifact
 
 
-@qiita_test_checker()
 class UtilTests(TestCase):
     def test_get_artifact(self):
-        obs = _get_artifact(-1)
-        exp = (None, False, 'Artifact does not exist')
+        obs = _get_artifact(1)
+        exp = qdb.artifact.Artifact(1)
         self.assertEqual(obs, exp)
 
+        # It does not exist
+        with self.assertRaises(HTTPError):
+            _get_artifact(100)
+
 
 class ArtifactFilepathsHandlerTests(OauthTestingBase):
     def test_get_artifact_does_not_exist(self):
         obs = self.get('/qiita_db/artifacts/100/filepaths/',
                        headers=self.header)
-        self.assertEqual(obs.code, 200)
-        exp = {'success': False, 'error': 'Artifact does not exist',
-               'filepaths': None}
-        self.assertEqual(loads(obs.body), exp)
+        self.assertEqual(obs.code, 404)
 
     def test_get_artifact(self):
         obs = self.get('/qiita_db/artifacts/1/filepaths/', headers=self.header)
@@ -44,34 +47,79 @@ def test_get_artifact(self):
              "raw_forward_seqs"],
             [path_builder('1_s_G1_L001_sequences_barcodes.fastq.gz'),
              "raw_barcodes"]]
-        exp = {'success': True, 'error': '',
-               'filepaths': exp_fps}
-        self.assertEqual(loads(obs.body), exp)
+        self.assertEqual(loads(obs.body), {'filepaths': exp_fps})
 
     def test_get_no_header(self):
         obs = self.get('/qiita_db/artifacts/1/filepaths/')
         self.assertEqual(obs.code, 400)
 
+
+class ArtifactFilepathsHandlerTestsReadWrite(OauthTestingBase):
+    database = True
+
+    def setUp(self):
+        super(ArtifactFilepathsHandlerTestsReadWrite, self).setUp()
+
+        fd, self.html_fp = mkstemp(suffix=".html")
+        close(fd)
+        self._clean_up_files = [self.html_fp]
+
+    def tearDown(self):
+        super(ArtifactFilepathsHandlerTestsReadWrite, self).tearDown()
+        for fp in self._clean_up_files:
+            if exists(fp):
+                remove(fp)
+
     def test_patch(self):
-        # TODO: issue #1682
-        pass
+        arguments = {'op': 'add', 'path': '/html_summary/',
+                     'value': self.html_fp}
+        self.assertIsNone(qdb.artifact.Artifact(1).html_summary_fp)
+        obs = self.patch('/qiita_db/artifacts/1/filepaths/',
+                         headers=self.header,
+                         data=arguments)
+        self.assertEqual(obs.code, 200)
+        self.assertIsNotNone(qdb.artifact.Artifact(1).html_summary_fp)
+
+        # Wrong operation
+        arguments = {'op': 'wrong', 'path': '/html_summary/',
+                     'value': self.html_fp}
+        obs = self.patch('/qiita_db/artifacts/1/filepaths/',
+                         headers=self.header,
+                         data=arguments)
+        self.assertEqual(obs.code, 400)
+        self.assertEqual(obs.body, 'Operation "wrong" not supported. Current '
+                                   'supported operations: add')
+
+        # Wrong path parameter
+        arguments = {'op': 'add', 'path': '/wrong/',
+                     'value': self.html_fp}
+        obs = self.patch('/qiita_db/artifacts/1/filepaths/',
+                         headers=self.header,
+                         data=arguments)
+        self.assertEqual(obs.code, 400)
+        self.assertEqual(obs.body, 'Incorrect path parameter value')
+
+        # Wrong value parameter
+        arguments = {'op': 'add', 'path': '/html_summary/',
+                     'value': self.html_fp}
+        obs = self.patch('/qiita_db/artifacts/1/filepaths/',
+                         headers=self.header,
+                         data=arguments)
+        self.assertEqual(obs.code, 500)
+        self.assertIn('No such file or directory', obs.body)
 
 
 class ArtifactMappingHandlerTests(OauthTestingBase):
     def test_get_artifact_does_not_exist(self):
         obs = self.get('/qiita_db/artifacts/100/mapping/', headers=self.header)
-        self.assertEqual(obs.code, 200)
-        exp = {'success': False, 'error': 'Artifact does not exist',
-               'mapping': None}
-        self.assertEqual(loads(obs.body), exp)
+        self.assertEqual(obs.code, 404)
 
     def test_get(self):
         obs = self.get('/qiita_db/artifacts/1/mapping/', headers=self.header)
         self.assertEqual(obs.code, 200)
         db_dir = qdb.util.get_mountpoint('templates')[0][1]
         exp_fp = join(db_dir, "1_prep_1_qiime_19700101-000000.txt")
-        exp = {'success': True, 'error': '',
-               'mapping': exp_fp}
+        exp = {'mapping': exp_fp}
         self.assertEqual(loads(obs.body), exp)
 
     def test_get_no_header(self):
@@ -82,16 +130,12 @@ def test_get_no_header(self):
 class ArtifactTypeHandlerTests(OauthTestingBase):
     def test_get_artifact_does_not_exist(self):
         obs = self.get('/qiita_db/artifacts/100/type/', headers=self.header)
-        self.assertEqual(obs.code, 200)
-        exp = {'success': False, 'error': 'Artifact does not exist',
-               'type': None}
-        self.assertEqual(loads(obs.body), exp)
+        self.assertEqual(obs.code, 404)
 
     def test_get(self):
         obs = self.get('/qiita_db/artifacts/1/type/', headers=self.header)
         self.assertEqual(obs.code, 200)
-        exp = {'success': True, 'error': '',
-               'type': "FASTQ"}
+        exp = {'type': "FASTQ"}
         self.assertEqual(loads(obs.body), exp)
 
     def test_get_no_header(self):

qiita_db/handlers/tests/test_oauth2.py

@@ -34,20 +34,15 @@ def setUp(self):
         super(OAuth2BaseHandlerTests, self).setUp()
 
     def test_authenticate_header_client(self):
-        obs = self.get('/qiita_db/artifacts/100/mapping/', headers={
+        obs = self.get('/qiita_db/artifacts/1/mapping/', headers={
             'Authorization': 'Bearer ' + self.client_token})
         self.assertEqual(obs.code, 200)
-        exp = {'success': False, 'error': 'Artifact does not exist',
-               'mapping': None}
-        self.assertEqual(loads(obs.body), exp)
 
     def test_authenticate_header_username(self):
-        obs = self.get('/qiita_db/artifacts/100/mapping/', headers={
+        obs = self.get('/qiita_db/artifacts/1/mapping/', headers={
             'Authorization': 'Bearer ' + self.user_token})
         self.assertEqual(obs.code, 200)
-        exp = {'success': False, 'error': 'Artifact does not exist',
-               'mapping': None}
-        self.assertEqual(loads(obs.body), exp)
+
         # Check rate limiting works
         self.assertEqual(int(r_client.get(self.user_rate_key)), 1)
         r_client.setex('testuser_test@foo.bar_daily_limit', 0, 2)