[Imaging browser] Fix loading error for t1-defaced and t2-defaced scan types #6668

h-karim · 2020-06-03T18:16:51Z

Adding 't1-defaced' or 't2-defaced' scan types to the imaging browser was causing an SQL error due to the hyphen in the name.

Brief summary of changes

~~I replaced all hyphen occurrences in the array containing the scan type names with underscores. This does not change how the scan type name appears on the frontend.~~
I used Database->quote() to quote scan type names, or surrounded them with backticks if they needed to be interpreted as identifiers rather than string literals.

Testing instructions (if applicable)

Go to Admin>Configuration>Imaging types>
Add t1-defaced and/or t2-defaced scan types next to the tabulated scan types and save
Go to the imaging module front page
Observe the module loading properly, with t1-defaced/t2-defaced QC columns

Link(s) to related issue(s)

Resolves [imaging browser] Adding some scan types renders the module unloadable #6616

driusan · 2020-06-03T19:20:07Z

modules/imaging_browser/php/imagingbrowserrowprovisioner.class.inc

@@ -43,7 +43,7 @@ class ImagingBrowserRowProvisioner extends \LORIS\Data\Provisioners\DBRowProvisi
        // ===========================================================

        // Grep all the different scan types present in mri_scan_type
-        $all_scan_types = \Utility::getScanTypeList();
+        $all_scan_types = str_replace('-', '_', \Utility::getScanTypeList());


There's a Database->quote function for quoting field (or table) names which may have special characters.

@driusan I just pushed a commit where I added Database->quote calls where needed. However where the names with special characters are used as parts of column names / identifiers, Database->quote won't work because they'll be interpreted as string literals. For those cases I had to use backticks ` `. Wouldn't the previous approach (replacing hyphens with underscores) be better since it introduces much less code change?

edit: and would it not be better practice to avoid using special characters in field names/strings rather than working around them?

@h-karim The quoting is better practice since you can't always control what users input as fields in a table. the not-so-ideal issue is that we are using field values as column names in another table... that's not great but it's not a problem solvable in this PR.

as far as the quote not working, I think it's better if you terminate the string, add the quote and re-concatenate the string instead. I made a suggestion

quote was the wrong function (as per my other comment), it should be escape to escape table/column names.

modules/imaging_browser/php/imagingbrowserrowprovisioner.class.inc

driusan · 2020-06-04T12:27:25Z

modules/imaging_browser/php/imagingbrowserrowprovisioner.class.inc

+              JOIN files_qcstatus USING (FileID)
+              WHERE files.AcquisitionProtocolID= $key
+                AND files_qcstatus.QCStatus IN (1, 2)
+              GROUP BY files.SessionID) `$pass[$key]`


Why isn't this using $DB->quote like the other places? Simply adding "" isn't robust. For instance, it will still error out if the value has a "" in it.

I pushed a commit to call quote() at the top of the file for a cleaner look (L115 for the pass array). This makes all the elements inside the array quoted, let me know if this works.

We still need the backticks however, removing them makes $pass[$key] get interpreted as a literal instead of an identifier, and spits out an error:

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 '''t2-defacedpass')'\n ON ('t2-defacedpass'.SessionID=f.SessionID' at line 69

sorry to have sent you down the wrong path.. $DB->quote is the wrong function, it's for quoting strings. It should be $DB->escape to escape table or column names.

/** * Escapes a string properly for mysql and appends and prepends backticks. * Any backticks you append or prepend will be escaped. * * @param string $tableName The column or table name that needs to be escaped * * @return string surrounded by backticks and with special characters escaped. */

(luckily, it should be a simple change to change the places you called quote on a table/column name to escape..)

christinerogers · 2020-06-04T18:31:25Z

@cmadjar just to check- is this a valid issue we should be trying hard to solve? --> Enabling t1-defaced and t2-defaced as types to be displayed as valid QC Status columns in the ImgBrowser data table.

Again @h-karim I'm not sure that the -defaced columns are the practical/desirable to show in this context.

cmadjar · 2020-06-05T13:18:26Z

modules/imaging_browser/php/imagingbrowserrowprovisioner.class.inc

+              GROUP BY files.SessionID) `$pass[$key]`
+                ON (`$pass[$key]`.SessionID=f.SessionID


Couldn't it be re-written like that instead of using backtic?

Suggested change

GROUP BY files.SessionID) `$pass[$key]`

ON (`$pass[$key]`.SessionID=f.SessionID

GROUP BY files.SessionID) " . $DB->quote($pass[$key])

. "ON (" . $DB->quote($pass[$key]) . ".SessionID=f.SessionID

when I tried this, it returned the following error:

[Fri Jun 05 09:51:14.495832 2020] [php7:warn] [pid 24948] [client ::1:56090] 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 ''t1'ON ('t1'.SessionID=f.SessionID\n ) \n LEFT JOIN (\n ' at line 73 in /var/www/loris/src/Data/Provisioners/DBRowProvisioner.php

I think it's because the single quotes around a word without any back tics wrapped gets it to be interpreted as a string literal instead of column/table name ¹.
Also, in the latest commit, the single quotes are used on L115: https://github.com/aces/Loris/pull/6668/files#diff-246d74737579a11922903fcadb7015ffL112-R117 , so the back tics would be wrapped around the table name resulting in `'t1pass'` for example which would escape any special characters as well as force it to be interpreted as a table name.

See https://stackoverflow.com/questions/11321491/when-to-use-single-quotes-double-quotes-and-backticks-in-mysql for descriptions of what the difference between "'" and "`" is (which should be generated by quote and escape in the database class, respectively in LORIS..) it's possible some places in this PR should be quoted and others need to be escaped, I haven't looked into the details of what context each is used in in this PR.

cmadjar · 2020-06-05T13:21:13Z

In general, scan types could have - characters, regardless of defaced or not. Open science projects will only have *-defaced scan types for anatomicals since only defaced data can be present on the LORIS instance.

It would be nice if it can get in the release. If not it could always goes into bug fix.

driusan · 2020-06-05T15:05:05Z

modules/imaging_browser/php/imagingbrowserrowprovisioner.class.inc

              s.Active = 'Y' AND
-              f.FileType IN ('mnc', 'nii')
+              f.FileType='mnc'


This is a regression of #6593

Sorry, I don't know how this slipped, I amended the latest commit to change that line to how it was.

h-karim · 2020-06-05T15:05:43Z

Update on everything: Database->quote is used to escape special characters for the elements of the array qc and the array coalesc_desc because the elements in those arrays are used as strings. Database->escape is used for the elements of the array pass because those elements are used as identifiers.

cmadjar

Tested the latest code changes and it works.

@driusan time for the final review

h-karim requested a review from christinerogers June 3, 2020 18:16

h-karim added 23.0.0-testing Critical to release PR or issue is key for the release to which it has been assigned labels Jun 3, 2020

h-karim requested review from laemtl and ridz1208 June 3, 2020 19:02

driusan reviewed Jun 3, 2020

View reviewed changes

h-karim requested a review from driusan June 4, 2020 00:50

h-karim force-pushed the 2020-05-27-Issue6616-SQL branch 3 times, most recently from 30d9179 to 5e71c3c Compare June 4, 2020 02:15

ridz1208 reviewed Jun 4, 2020

View reviewed changes

modules/imaging_browser/php/imagingbrowserrowprovisioner.class.inc Outdated Show resolved Hide resolved

ridz1208 reviewed Jun 4, 2020

View reviewed changes

modules/imaging_browser/php/imagingbrowserrowprovisioner.class.inc Outdated Show resolved Hide resolved

h-karim force-pushed the 2020-05-27-Issue6616-SQL branch from a5ee8e1 to fd70cf5 Compare June 4, 2020 05:22

h-karim requested a review from ridz1208 June 4, 2020 06:03

driusan requested changes Jun 4, 2020

View reviewed changes

driusan mentioned this pull request Jun 4, 2020

[imaging Browser] Fix wrong data being displayed under QC columns #6654

Merged

h-karim force-pushed the 2020-05-27-Issue6616-SQL branch from 2ac6652 to a86d40f Compare June 4, 2020 16:27

h-karim requested a review from driusan June 4, 2020 20:33

cmadjar reviewed Jun 5, 2020

View reviewed changes

h-karim marked this pull request as ready for review June 5, 2020 14:10

23.0 pull to fix travis mistake

700fee5

h-karim force-pushed the 2020-05-27-Issue6616-SQL branch from a86d40f to 700fee5 Compare June 5, 2020 14:22

h-karim added a commit to h-karim/Loris that referenced this pull request Jun 5, 2020

Add database->quote for easy merge with aces#6668

cefffde

driusan requested changes Jun 5, 2020

View reviewed changes

Remove backtics, escape pass variable

ef4ee6e

h-karim force-pushed the 2020-05-27-Issue6616-SQL branch from 8b79d9e to ef4ee6e Compare June 5, 2020 15:09

h-karim requested a review from driusan June 5, 2020 15:13

cmadjar added the Passed manual tests PR has been successfully tested by at least one peer label Jun 5, 2020

cmadjar approved these changes Jun 5, 2020

View reviewed changes

driusan merged commit edc545b into aces:23.0-release Jun 5, 2020

h-karim added a commit to h-karim/Loris that referenced this pull request Jun 5, 2020

Add database->quote for easy merge with aces#6668

47bce78

ridz1208 added this to the 23.0.0 milestone Jun 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Imaging browser] Fix loading error for t1-defaced and t2-defaced scan types #6668

[Imaging browser] Fix loading error for t1-defaced and t2-defaced scan types #6668

h-karim commented Jun 3, 2020 •

edited

Loading

driusan Jun 3, 2020

h-karim Jun 4, 2020 •

edited

Loading

ridz1208 Jun 4, 2020

driusan Jun 5, 2020

driusan Jun 4, 2020

h-karim Jun 4, 2020

driusan Jun 5, 2020

christinerogers commented Jun 4, 2020

cmadjar Jun 5, 2020

h-karim Jun 5, 2020 •

edited

Loading

driusan Jun 5, 2020 •

edited

Loading

cmadjar commented Jun 5, 2020

driusan Jun 5, 2020

h-karim Jun 5, 2020

h-karim commented Jun 5, 2020

cmadjar left a comment

		GROUP BY files.SessionID) `$pass[$key]`
		ON (`$pass[$key]`.SessionID=f.SessionID

[Imaging browser] Fix loading error for t1-defaced and t2-defaced scan types #6668

[Imaging browser] Fix loading error for t1-defaced and t2-defaced scan types #6668

Conversation

h-karim commented Jun 3, 2020 • edited Loading

Brief summary of changes

Testing instructions (if applicable)

Link(s) to related issue(s)

Choose a reason for hiding this comment

h-karim Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christinerogers commented Jun 4, 2020

Choose a reason for hiding this comment

h-karim Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

driusan Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

cmadjar commented Jun 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-karim commented Jun 5, 2020

cmadjar left a comment

Choose a reason for hiding this comment

h-karim commented Jun 3, 2020 •

edited

Loading

h-karim Jun 4, 2020 •

edited

Loading

h-karim Jun 5, 2020 •

edited

Loading

driusan Jun 5, 2020 •

edited

Loading