#1152 Change exception information for errors when parsing a modulemd file
Merged 5 months ago by mprahl. Opened 5 months ago by vmaljulin.
vmaljulin/fm-orchestrator FACTORY-3909  into  master

@@ -699,7 +699,19 @@ 

          # If the modulemd was v1, it will be upgraded to v2

          mmd.upgrade()

      except Exception:

-         error = 'The following invalid modulemd was encountered: {0}'.format(yaml)

+         if is_file:

+             error = 'The modulemd {} is invalid. Please verify the syntax is correct'.format(

+                 os.path.basename(yaml)

+             )

+             if os.path.exists(yaml):

+                 with open(yaml, 'rt') as yaml_hdl:

+                     log.debug('Modulemd content:\n%s', yaml_hdl.read())

+             else:

+                 error = 'The modulemd file {} not found!'.format(os.path.basename(yaml))

The contents of this are still raised in raise UnprocessableEntity(error), thus exposing the path to the user.

+                 log.error('The modulemd file %s not found!', yaml)

+         else:

+             error = 'The modulemd is invalid. Please verify the syntax is correct'

+             log.debug('Modulemd content:\n%s', yaml)

          log.exception(error)

          raise UnprocessableEntity(error)

  

@@ -33,6 +33,7 @@ 

  import hashlib

  import pytest

  from module_build_service.utils import to_text_type

+ import re

  

  from tests import app, init_data, clean_database, reuse_component_init_data

  from tests import read_staged_data

@@ -972,7 +973,8 @@ 

              {'branch': 'master', 'scmurl': 'https://src.stg.fedoraproject.org/modules/'

                  'testmodule.git?#68931c90de214d9d13feefbd35246a81b6cb8d49'}))

          data = json.loads(rv.data)

-         assert data['message'].startswith('The following invalid modulemd was encountered') is True

+         assert re.match(r'The modulemd .* is invalid\. Please verify the syntax is correct',

+                         data['message'])

          assert data['status'] == 422

          assert data['error'] == 'Unprocessable Entity'

  

@@ -1596,7 +1598,8 @@ 

          data = json.loads(rv.data)

  

          assert data['error'] == 'Unprocessable Entity'

-         assert data['message'].startswith('The following invalid modulemd was encountered')

+         assert re.match(r'The modulemd .* is invalid\. Please verify the syntax is correct',

+                         data['message'])

  

      @pytest.mark.parametrize('api_version', [1, 2])

      @patch('module_build_service.auth.get_user', return_value=import_module_user)

This fixes #1149

Signed-off-by: Valerij Maljulin vmaljuli@redhat.com

Can you add a new line after Modulemd content:? That way the yaml is on a new line in the logs and it's easier to read.

Can you log the path where it wasn't found?

It might be useful to move this check up in the function. It makes sense to me to verify the path exists before running mmd = Modulemd.Module().new_from_file(yaml). It'd also make the code a bit cleaner.

rebased onto 0f7c515652d39ed71bbce232a4decfddeccee8fb

5 months ago

Can you add a new line after Modulemd content:? That way the yaml is on a new line in the logs and it's easier to read.

Done

Can you log the path where it wasn't found?

Done

It might be useful to move this check up in the function. It makes sense to me to verify the path exists before running mmd = Modulemd.Module().new_from_file(yaml). It'd also make the code a bit cleaner.

It wouldn't make the code much cleaner because there still will be a check before log.debug as long as you don't want to have an exception in the except block.

The path should be logged, but not returned to the user.

rebased onto f6a4bef

5 months ago

The path should be logged, but not returned to the user.

Done

The contents of this are still raised in raise UnprocessableEntity(error), thus exposing the path to the user.

The contents of this are still raised in raise UnprocessableEntity(error), thus exposing the path to the user.

But there is only the basename in the exception message. Or should I delete it also?

@vmaljulin you're right. I misread it. This looks good.

Pull-Request has been merged by mprahl

5 months ago