Loris: [imaging browser] Adding some scan types renders the module unloadable

[h-karim]

Additional context
This fails step 7 of the "main page" section of the test plan.

Describe the bug
When adding a tabulated scan type, selecting "t1-defaced" or "t2-defaced" and saving makes the module return an error when trying to load it

To Reproduce
Steps to reproduce the behavior:

1. Navigate to Admin>Configuration>Imaging Modules
2. Click on the "Add field" button under "Imaging Tabulated Scan Types"
3. Select "t1-defaced" or "t2-defaced"
4. Click on the Submit button.
5. Navigate to Imaging>Imaging Browser
6. Observe loading error

What did you expect to happen?
To observe the front page loading properly with the relevant columns.

Browser Environment (please complete the following information):

• OS: Ubuntu 18.04 LTS
• Browser:
  firefox 76.0.1
  Chrome 83.0.4103.61

Server Environment (if known):
This bug was observed on the Testing VM

[johnsaigle]

I was able to confirm this bug. It seems to be triggered by the change introduced by https://github.com/aces/Loris/pull/6089.

The error is:

[Tue May 26 14:28:07.251088 2020] [php7:warn] [pid 12987] [client 192.168.122.1:36364] PHP Warning: PDOStatement::execute(): SQLSTATE[42000] Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '-defaced_QC_Status,s.CenterID as CenterID,\n c.Entity_type as entity' at line 52 in /var/www/loris/src/Data/Provisioners/DBRowProvisioner.php on line 87, referer: https://jsaigle-dev.loris.ca/imaging_browser/
[Tue May 26 14:28:07.259500 2020] [php7:error] [pid 12987] [client 192.168.122.1:36364] PHP Fatal error: Uncaught Exception: Invalid SQL statement: SELECT \n p.Name as Site,\n c.PSCID as PSCID,\n c.CandID as DCCID,\n Project.Name as project,\n s.Visit_label as visitLabel,\n \n CASE s.MRIQCStatus\n WHEN 'Fail' THEN\n IF(s.MRIQCPending='Y', 'Pending Fail', 'Fail')\n WHEN 'Pass' THEN\n IF(s.MRIQCPending='Y', 'Pending Pass', 'Pass') \n ELSE s.MRIQCStatus\n END \n as Visit_QC_Status,\n DATE_FORMAT(MIN(pf.Value), "%Y-%m-%d") as First_Acquisition,\n FROM_UNIXTIME(MIN(f.InsertTime)) as First_Insertion,\n FROM_UNIXTIME(MAX(fqc.QCLastChangeTime)) as Last_QC,\n \n CASE \n COALESCE(Max(fqc.QCLastChangeTime), 'new')\n WHEN 'new' THEN IF(FIND_IN_SET(44,GROUP_CONCAT(\n DISTINCT AcquisitionProtocolID))>0 OR FIND_IN_SET(45,GROUP_CONCAT(\n in /var/www/loris/src/Data/Provisioners/DBRowProvisioner.php on line 90, referer: https://jsaigle-dev.loris.ca/imaging_browser/

[h-karim]

@johnsaigle Something seems wrong with the SQL query when "t1-defaced" or "t2-defaced" is introduced, but it doesn't seem to be related to the change in #6089. Reverting the change done to the query isn't getting rid of the error message.

[christinerogers]

Ok- what's the query you're executing in your sandbox? @h-karim (and the error, when you trying to execute it). Which variations on the query have you tried?

[h-karim]

@christinerogers The query, I'm trying to execute is: https://github.com/aces/Loris/blob/a5304c762454fa8241ce0699af94b2da15f8879d/modules/imaging_browser/php/imagingbrowserrowprovisioner.class.inc#L177-L212 I've trying executing it without any of the $subqueries and it works fine:

SELECT
    p.Name as Site,
    c.PSCID as PSCID,
    c.CandID as DCCID,
    Project.Name as project,
    s.Visit_label as visitLabel,

    DATE_FORMAT(MIN(pf.Value), "%Y-%m-%d") as First_Acquisition,
    FROM_UNIXTIME(MIN(f.InsertTime)) as First_Insertion,
    FROM_UNIXTIME(MAX(fqc.QCLastChangeTime)) as Last_QC,

    GROUP_CONCAT(DISTINCT OutputType) as Links,
    s.ID as sessionID,
    GROUP_CONCAT(DISTINCT modality.Scan_type) as sequenceType,

    s.CenterID as CenterID,
    c.Entity_type as entityType,
    s.ProjectID
FROM psc AS p 
    JOIN session s ON (s.CenterID=p.CenterID)
    JOIN candidate c ON (c.CandID=s.CandID)
    LEFT JOIN Project ON (s.ProjectID=Project.ProjectID)
    JOIN files f ON (f.SessionID=s.ID)
    LEFT JOIN files_qcstatus fqc ON (fqc.FileID=f.FileID)
    JOIN parameter_file pf ON (f.FileID=pf.FileID)
    JOIN parameter_type pt USING (ParameterTypeID)
    LEFT JOIN mri_scan_type modality ON
    (f.AcquisitionProtocolID=modality.ID)

WHERE
    s.Active = 'Y' AND 
    f.FileType='mnc'
GROUP BY s.ID
ORDER BY c.PSCID, s.Visit_label

[cmadjar]

https://github.com/aces/Loris/pull/6654

[h-karim]

@cmadjar #6654 doesn't resolve this issue unfortunately.

[christinerogers]

As @ridz1208 points out in this 6551 comment -- the _hyphen_ in -defaced is probably causing the error.

@h-karim can you confirm, do all other fields work in the query, excepting only those with hyphens? (this will pinpoint the strategy for the solution)

[h-karim]

@christinerogers Yes, all other scan type strings work. What's left is escaping table names with hyphens in them. The challenge is that the scan types are used as building blocks for different strings which are then concatenated into different column names in the SQL query.

Possible solutions:

There are php functions that can do this for the whole query:

• mysqli::mysqli_escape_string
• PDO::quote
  however these are non-static methods, in order to use them, the imagingbrowserrowprovisioner class would need access to the connection instance between php and mysql. Is there currently a way to obtain this without increasing coupling?

If there isn't a way to do this, then in imagingbrowserrowprovisioner, I need to find all the column names that are formed from the scan types and surround them with double quotes, something like this:

 $modalities_subquery = '';
        foreach ($case_desc as $key => $value) {
            $modalities_subquery .= "$value as \"$scan_id_types[$key]_QC_Status\",";
        }

@ridz1208 I would appreciate your input here since you suggested to use the first method.

[johnsaigle]

@h-karim We have a Database->quote() which is a wrapper around PDO->quote(). You can use \NDB_Factory::singleton()->database() to access the current database connection within LORIS and from this object use quote() to help build your query. Also see Database::escape() -- I'm not sure if it applies here but it may help with e.g. problematic characters in column names.