Bug 49578 - check for valid JSON in configuration files before loading them
check for valid JSON in configuration files before loading them
Status: CLOSED FIXED
Product: UCS@school
Classification: Unclassified
Component: Import scripts
UCS@school 4.4
Other Linux
: P5 normal (vote)
: UCS@school 4.4 v9-errata
Assigned To: Toni Röhmeyer
Tobias Wenzel
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2019-05-31 10:51 CEST by Christian Völker
Modified: 2021-05-06 14:11 CEST (History)
3 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 2: Improvement: Would be a product improvement
Who will be affected by this bug?: 2: Will only affect a few installed domains
How will those affected feel about the bug?: 2: A Pain – users won’t like this once they notice it
User Pain: 0.046
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional):
Max CVSS v3 score:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Völker univentionstaff 2019-05-31 10:51:24 CEST
Having created some sort of csv and some matching configuration file as shown:

==============================================================
{
    "csv": {
        "allowed_missing_columns": [],
        "header_lines": 1,
        "incell-delimiter": {
            "default": ","
        },
        "mapping": {
            "Klassen": "school_classes",
            "Mailadresse": "email",
            "Nachname": "lastname",
            "Schulen": "schools",
            "Username": "name",
            "Vorname": "firstname",
            "id": "record_uid",
            "phone": "phone",
            "src": "source_uid"
        }
    }
==============================================================

Running it produces traceback
==============================================================
/usr/share/ucs-school-import/scripts/ucs-school-user-import -i lehrer.csv.0  -u teacher 
------ UCS@school import tool starting ------
Reading configuration from '/usr/share/ucs-school-import/configs/global_defaults.json'...
Reading configuration from '/var/lib/ucs-school-import/configs/global.json'...
Reading configuration from '/usr/share/ucs-school-import/configs/user_import_defaults.json'...
Reading configuration from '/var/lib/ucs-school-import/configs/user_import.json'...
Error setting up or checking the configuration: Error in configuration file '/var/lib/ucs-school-import/configs/user_import.json': Expecting object: line 20 column 1 (char 363).
Traceback (most recent call last):
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/frontend/cmdline.py", line 86, in setup_config
    self.config = setup_configuration(configs, **self.args.settings)
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/configuration.py", line 52, in setup_configuration
    config = Configuration(conffiles)
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/configuration.py", line 188, in __new__
    cls._instance = cls.__SingleConf(filenames)
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/configuration.py", line 179, in __init__
    raise InitialisationError("Error in configuration file '{}': {}.".format(filename, ve))
InitialisationError: Error in configuration file '/var/lib/ucs-school-import/configs/user_import.json': Expecting object: line 20 column 1 (char 363).
Used configuration files: ['/usr/share/ucs-school-import/configs/global_defaults.json', '/var/lib/ucs-school-import/configs/global.json', '/usr/share/ucs-school-import/configs/user_import_defaults.json', '/var/lib/ucs-school-import/configs/user_import.json'].
Using command line arguments: {'input': {'filename': 'lehrer.csv.0'}, 'user_role': 'teacher'}
InitialisationError: Configuration not yet loaded.
Traceback (most recent call last):
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/frontend/cmdline.py", line 147, in main
    self.prepare_import()
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/frontend/cmdline.py", line 134, in prepare_import
    self.setup_config()
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/frontend/cmdline.py", line 93, in setup_config
    config = Configuration()
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/configuration.py", line 188, in __new__
    cls._instance = cls.__SingleConf(filenames)
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/configuration.py", line 168, in __init__
    raise InitialisationError("Configuration not yet loaded.")
InitialisationError: Configuration not yet loaded.
==============================================================


Testing config prints an error:
==============================================================
root@master:~# python -m json.tool <  /var/lib/ucs-school-import/configs/user_import.json
Expecting object: line 19 column 3 (char 362)
==============================================================

None of the above errors is at all informative or leading to the solution.

The error was a missing "}" at the end of the file.

I suggest to improve the above syntax check and include it into importer and print some useful information in case of errors.
Comment 1 Toni Röhmeyer univentionstaff 2020-02-13 13:03:23 CET
Removed traceback printing from exception blocks.
The error messages raised from any InitialisationError contain sufficient information.

Solution was pushed on branch troehmey/bug49578 with commit

660aac1bb4d957bb33869fc0bb30e66ecd1a7a31
Bug #49578: Removed tracebacks from error handling


waiting for QA
Comment 2 Tobias Wenzel univentionstaff 2021-04-14 09:12:51 CEST
QA 

> waiting for QA
Sorry for the long wait!

I could reproduce the error with the given configuration by running 

/usr/share/ucs-school-import/scripts/ucs-school-testuser-import --students 3 --classes 1 DEMOSCHOOL

→ InitialisationError: Configuration not yet loaded.


Why did you replace logger.exception -> logger.error?
logger.exception gives us a tracebace which might be valuable. 

We need this at least in line 150. I would agree replacing it in line 88, because this information is more or less redundant. The output would be something like

Error setting up or checking the configuration: Error in configuration file '/var/lib/ucs-school-import/configs/user_import.json': Expecting object: line 19 column 6 (char 494).
Used configuration files: ['/usr/share/ucs-school-import/configs/global_defaults.json', '/var/lib/ucs-school-import/configs/global.json', '/usr/share/ucs-school-import/configs/user_import_defaults.json', '/var/lib/ucs-school-import/configs/user_import.json', '/usr/share/ucs-school-import/configs/ucs-school-testuser-import.json'].
Using command line arguments: {'input': {'filename': 'test_users_2021-03-23_09:58:53.csv'}}
InitialisationError: Error in configuration file '/var/lib/ucs-school-import/configs/user_import.json': Expecting object: line 19 column 6 (char 494).
Traceback (most recent call last):
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/frontend/cmdline.py", line 147, in main
    self.prepare_import()
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/frontend/cmdline.py", line 134, in prepare_import
    self.setup_config()
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/frontend/cmdline.py", line 97, in setup_config
    raise exc
InitialisationError: Error in configuration file '/var/lib/ucs-school-import/configs/user_import.json': Expecting object: line 19 column 6 (char 494).
Comment 3 Toni Röhmeyer univentionstaff 2021-04-27 13:12:48 CEST
I pushed a fix on a new updated branch troehmey/bug49578_v2:

942e182db Bug #49578: remove unnecessary traceback


With this fix we only get the exception that really matters:

Error setting up or checking the configuration: Error in configuration file '/usr/share/ucs-school-import/configs/ucs-school-testuser-import.json': Expecting object: line 25 column 3 (char 504).
Used configuration files: ['/usr/share/ucs-school-import/configs/global_defaults.json', '/var/lib/ucs-school-import/configs/global.json', '/usr/share/ucs-school-import/configs/user_import_defaults.json', '/var/lib/ucs-school-import/configs/user_import.json', '/usr/share/ucs-school-import/configs/ucs-school-testuser-import.json'].
Using command line arguments: {'input': {'filename': 'test_users_2021-04-23_05:17:33.csv'}}
InitialisationError: Error in configuration file '/usr/share/ucs-school-import/configs/ucs-school-testuser-import.json': Expecting object: line 25 column 3 (char 504).
Traceback (most recent call last):
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/frontend/cmdline.py", line 247, in main
    self.prepare_import()
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/frontend/cmdline.py", line 201, in prepare_import
    self.setup_config()
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/frontend/cmdline.py", line 133, in setup_config
    raise exc
InitialisationError: Error in configuration file '/usr/share/ucs-school-import/configs/ucs-school-testuser-import.json': Expecting object: line 25 column 3 (char 504).


-> We don't get the second traceback "Configuration not yet loaded"
-> We still see what error occured and where it has been raised.
Comment 4 Tobias Wenzel univentionstaff 2021-04-28 09:50:03 CEST
Please use a minimal description for your branch next time, with the new branch this gets a little confusing.
Don't replace specific exceptions with broader exceptions.	
In my opinion, this is ok in this situation: We want to see the exception & it's message in any case. We should discuss this.
Other than that it looks OK.

Error setting up or checking the configuration: Error in configuration file '/var/lib/ucs-school-import/configs/user_import.json': Expecting object: line 24 column 1 (char 560).
Used configuration files: ['/usr/share/ucs-school-import/configs/global_defaults.json', '/var/lib/ucs-school-import/configs/global.json', '/usr/share/ucs-school-import/configs/user_import_defaults.json', '/var/lib/ucs-school-import/configs/user_import.json', '/usr/share/ucs-school-import/configs/ucs-school-testuser-import.json'].
Using command line arguments: {'input': {'filename': 'test_users_2021-03-23_22:39:57.csv'}}
InitialisationError: Error in configuration file '/var/lib/ucs-school-import/configs/user_import.json': Expecting object: line 24 column 1 (char 560).
Traceback (most recent call last):
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/frontend/cmdline.py", line 247, in main
    self.prepare_import()
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/frontend/cmdline.py", line 201, in prepare_import
    self.setup_config()
  File "/usr/lib/pymodules/python2.7/ucsschool/importer/frontend/cmdline.py", line 133, in setup_config
    raise exc
InitialisationError: Error in configuration file '/var/lib/ucs-school-import/configs/user_import.json': Expecting object: line 24 column 1 (char 560).
Comment 5 Tobias Wenzel univentionstaff 2021-04-28 10:31:48 CEST
As discussed, this is OK for me
please merge & build
Comment 6 Toni Röhmeyer univentionstaff 2021-04-28 15:44:18 CEST
Merged to 4.4 with:

89071e68d Bug #49578: advisories
24be18747 Bug #49578: added changelog entry
5e91b14e6 Bug #49578: Merge branch 'troehmey/bug49578_v2' into 4.4


Successful build:

Package: ucs-school-import
Version: 17.0.56A~4.4.0.202104281532
Branch: ucs_4.4-0
Scope: ucs-school-4.4
Comment 7 Tobias Wenzel univentionstaff 2021-05-04 10:52:07 CEST
import tests pass in Jenkins → verify

merge, changelog & yaml → OK
Comment 8 Tobias Wenzel univentionstaff 2021-05-06 14:11:07 CEST
Errata updates for UCS@school 4.4 v9 have been released.

https://docs.software-univention.de/changelog-ucsschool-4.4v9-de.html

If this error occurs again, please clone this bug.