[database] Clean up mri_scan_type #9304

maximemulder · 2024-06-26T15:37:01Z

Summary of changes

Add missing foreign keys.
Rename Scan_type foreign key fields to MriScanTypeID.
Remove the default value of 0 for mri_protocol.MriScanTypeID (as this is a non-nullable foreign key, I don't think this should ever happen).
Adapt the existing queries and scripts in LORIS.

Testing instructions

Apply the patch
Reinstall Raisinbread
Test the associated LORIS feature

As we unfortunately have a very loose typing in LORIS, I may have missed a few bugs when adapting the scripts, but we're early on the next version development cycle so I guess it's the right time to do such changes.

Alternatives

The foreign key fields could just be named ScanTypeID (it's obvious it's an "MRI" scan type).
Should the files.AcquisitionTypeID also be renamed to files.MriScanTypeID ?
Should mri_scan_type.Scan_type be renamed to mri_scan_type.Name ?

Related issues

Resolves [imaging/database] Clean-up mri_protocol tables #9284
Complementary LORIS-MRI PR 1139

maximemulder · 2024-06-26T17:46:48Z

I would like to have some opinions on the alternatives before merging. Also I don't know who should review it so I'll just ping @ridz1208 and feel free to give your opinion or re-assign it.

maximemulder · 2024-07-04T15:19:30Z

After the MRI meeting today, we agreed on the following TODOs before reviewing/merging this PR :

Add a unique constraint on the scan type name as per [database] Scan types can have identical names #9306 .
Apply the alternatives 2 and 3.

maximemulder · 2024-07-16T17:55:32Z

Okay, I tested in LORIS as well as LORIS-MRI (imaging pipeline: dcm2mnc -> defacing -> mnc2bids) and everythings seem to wok, PR is ready for review/mege @cmadjar @nicolasbrossard. The PR is basically a SQL patch and renaming the fields in SQL queries (and some auto-formatting, feel free to ignore whitespace changes).

cmadjar

@maximemulder my initial review on the LORIS side for this.

One generic comment based on the spacing I see in your PR. In LORIS, we use the following convention: 1 tab = 4 spaces. I think you need to change your editor configuration to reflect that convention.

I also added some comments for @driusan and @nicolasbrossard to benefit from their wisdom on some of the changes :)

cmadjar · 2024-08-01T14:56:12Z

SQL/0000-00-00-schema.sql

@@ -675,27 +676,30 @@ CREATE TABLE `mri_protocol` (
  KEY `FK_mri_protocol_1` (`ScannerID`),
  CONSTRAINT `FK_mri_protocol_1` FOREIGN KEY (`ScannerID`) REFERENCES `mri_scanner` (`ID`),
  CONSTRAINT `FK_mri_protocol_2` FOREIGN KEY (`CenterID`) REFERENCES `psc` (`CenterID`),
+  CONSTRAINT `FK_mri_protocol_scan_type` FOREIGN KEY (`MriScanTypeID`) REFERENCES `mri_scan_type` (`ID`),


seriously, we never had a foreign key for this? Interesting...

cmadjar · 2024-08-01T15:01:29Z

SQL/0000-00-00-schema.sql

@@ -968,7 +972,7 @@ CREATE TABLE `mri_violations_log` (
  `CandID` int(6) DEFAULT NULL,
  `Visit_label` varchar(255) DEFAULT NULL,
  `CheckID` int(11) DEFAULT NULL,
-  `Scan_type` int(11) unsigned DEFAULT NULL,
+  `MriScanTypeID` int(11) unsigned DEFAULT NULL,


@nicolasbrossard @driusan I am curious what you think of the DEFAULT NULL for MriScanTypeID of table mri_violations_log. My understanding is that it can never be NULL based on the code since the violations log happen after scan identification. But before we remove the DEFAULT NULL from this table, I'd like your input. :)

Thank you!

@cmadjar I agree with you: I don't think a DEFAULT NULL makes sense here so I'd remove it.

@cmadjar I agree that if this can not be NULL we might just add a NOT NULL constraint, but should it really be in this (already quite large) PR ?

That's fine, it can be in a different PR. @maximemulder Could you create an issue for it so we do not forget about it?

cmadjar · 2024-08-01T15:02:27Z

SQL/0000-00-00-schema.sql

+  CONSTRAINT `FK_mri_violations_log_scan_type`
+    FOREIGN KEY (`MriScanTypeID`) REFERENCES `mri_scan_type` (`ID`),


fascinating, another foreign key I was sure was already there. 🤣 Aaah, the things you take for granted...

cmadjar · 2024-08-01T15:05:21Z

SQL/0000-00-00-schema.sql

+  `MriProtocolGroupTargetID` INT(11) UNSIGNED NOT NULL AUTO_INCREMENT,
+  `MriProtocolGroupID`       INT(4) UNSIGNED  NOT NULL,
+  `ProjectID`                INT(10) UNSIGNED DEFAULT NULL,
+  `CohortID`                 INT(10) UNSIGNED DEFAULT NULL,
+  `Visit_label`              VARCHAR(255)     DEFAULT NULL,
+  PRIMARY KEY (`MriProtocolGroupTargetID`),
+  CONSTRAINT `FK_mri_protocol_group_target_1` FOREIGN KEY (`MriProtocolGroupID`) REFERENCES `mri_protocol_group` (`MriProtocolGroupID`),
+  CONSTRAINT `FK_mri_protocol_group_target_2` FOREIGN KEY (`ProjectID`)          REFERENCES `Project` (`ProjectID`),
+  CONSTRAINT `FK_mri_protocol_group_target_3` FOREIGN KEY (`CohortID`)           REFERENCES `cohort` (`CohortID`),


That change should not show.

FYI - for LORIS code, one tab = 4 spaces so you might want to update your editor to use the same convention.

cmadjar · 2024-08-01T15:07:20Z

SQL/0000-00-00-schema.sql

+  CONSTRAINT `UK_mri_protocol_group_target`
+    UNIQUE (`ProjectID`, `CohortID`, `Visit_label`)


@nicolasbrossard is that new unique constraint assumption correct for that table? I don't remember if we said it would be something we would change.

@maximemulder the change with that new constraint is technically not related to the scan type renaming so I would tend to send this in a separate PR to keep things separate.

SQL/New_patches/2024-25-06-CleanMriScanType.sql

test/RBdata.sql

cmadjar · 2024-08-01T15:17:15Z

SQL/0000-00-00-schema.sql

@@ -530,8 +530,9 @@ SET SQL_MODE=@OLD_SQL_MODE;

 CREATE TABLE `mri_scan_type` (
  `ID` int(11) unsigned NOT NULL auto_increment,
-  `Scan_type` text NOT NULL,
-  PRIMARY KEY  (`ID`)
+  `Name` VARCHAR(255) NOT NULL,


Suggested change

`Name` VARCHAR(255) NOT NULL,

`MriScanTypeName` VARCHAR(255) NOT NULL,

I would tend to name the fields in that table as MriScanTypeID and MriScanTypeName. The first one is so that we can use USING in the queries. The second one is because when looking at the code change, if you need to look for something in the code with the Name field, it is really not specific. However, if we use MriScanTypeName, then you could grep that easily in the code.

I personally do not like the TableField naming convention as I find it verbose and repetitive (I often use SELECT t.field FROM table AS t in my queries).

The USING and grep arguments do make sense. However, I feel like those are answers to self-inflicted problems that are due to :

How widespread queries are in our codebase (they should ideally be constrained to some database files).

How raw SQL is a lot less searchable than (good) ORMs.

Hhhhm, our current SQL guidelines do not say anything about prefixing fields (outside of the ID) with the table name, so I assume not doing it would be the standard convention. Anyway, I think this is an issue that is broader than this PR, and that it should be discussed at a LORIS meeting. I am fine with blocking until a decision is taken.

I do want to say that I understand both point of views though, so although I have a preference I am not entirely dismissing your version.

I hate to disagree with both of you (joking, I love it) but I only like the ID to have the full table name (because its used in linking and as a reference) I find the full table name redundant in other fields, if we are looking for a query we would look for the table itself in the join clause

I would tend to name the fields in that table as MriScanTypeID and MriScanTypeName. The first one is so that we can use USING in the queries. The second one is because when looking at the code change, if you need to look for something in the code with the Name field, it is really not specific. However, if we use MriScanTypeName, then you could grep that easily in the code.

After some reflexion I am ok with applying these suggestions. I'll do it if / when I have some time to work again on this PR.

1. Drop the `DEFAULT 0` for foreign key fields 2. Change `DEFAULT '0'` (or `'1'`) to `DEFAULT 0` (or `1`) for other fields, which is more correct. Omitted the `mri_protocol.Scan_type` field as it is fixed in #9304 instead.

1. Drop the `DEFAULT 0` for foreign key fields 2. Change `DEFAULT '0'` (or `'1'`) to `DEFAULT 0` (or `1`) for other fields, which is more correct. Omitted the `mri_protocol.Scan_type` field as it is fixed in aces#9304 instead.

SQL/0000-00-00-schema.sql

cmadjar

Tested things:

Applying the SQL patch 🔴 => see attached comment with the error
Module testing after sourcing RB: not done due to error when applying the patch
Sourcing the new RB dataset 🟢
Module testing after sourcing RB:
- API 🟢
- Configuration 🟢
- DICOM Archive 🟢
- Imaging Browser 🔴
  - Error when loading viewSession - see attached comment with error - confirmed that this is working on main
  - QC Comments do not seem to be showing in the popup window - confirmed that this is working on main
- Imaging QC 🟢
- MRI Violations 🔴
  - Clicking on the Protocol Violation failed loading the violation due to a typo in the query - see attached comment with error
- Statistics 🟢

cmadjar · 2024-11-12T15:11:49Z

SQL/New_patches/2024-25-06-CleanMriScanType.sql

+-- Rename foreign key fields for consistency
+
+ALTER TABLE `mri_scan_type`
+  RENAME COLUMN `ID` TO `MriScanTypeID`;


I get the following error when trying to apply the patch:

$ mysql -A 'cecile_raisin_bread' < SQL/New_patches/2024-25-06-CleanMriScanType.sql ERROR 1064 (42000) at line 3: 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 'COLUMN `ID` TO `MriScanTypeID`' at line 2

Instead of RENAME, you should probably use MODIFY COLUMN to rename the field.

Oh interesting, which database are you using, it worked on mine (modern MariaDB) so I thought it was standard 🤔.

Apparently this is what is used in my MCIN sandbox:

mysql Ver 8.0.39-0ubuntu0.20.04.1 for Linux on x86_64 ((Ubuntu))

I copied #9270 and used the CHANGE syntax. The inconsistent but inconsequential types are copied from the existing schema.

cmadjar · 2024-11-12T15:14:01Z

SQL/0000-00-00-schema.sql

@@ -968,7 +972,7 @@ CREATE TABLE `mri_violations_log` (
  `CandID` int(6) DEFAULT NULL,
  `Visit_label` varchar(255) DEFAULT NULL,
  `CheckID` int(11) DEFAULT NULL,
-  `Scan_type` int(11) unsigned DEFAULT NULL,
+  `MriScanTypeID` int(11) unsigned DEFAULT NULL,


That's fine, it can be in a different PR. @maximemulder Could you create an issue for it so we do not forget about it?

cmadjar · 2024-11-12T15:35:27Z

modules/imaging_browser/php/viewsession.class.inc

            $extra_where_string
            ORDER BY files.OutputType,
-                     files.AcquisitionProtocolID,
+                     files.MriScanTypeID,
                     files.File",
            $args
        );


When testing view session page, I get the following error when accessing imaging_browser/viewSession/?sessionID=2144&outputType=native&backURL=/imaging_browser/:

[Tue Nov 12 10:32:35.067078 2024] [php:notice] [pid 57283:tid 57283] [client 192.168.122.1:51354] [ERROR] SQLSTATE[HY000]: General error: 2014 Cannot execute queries while there are pending result sets. Consider unsetting the previous PDOStatement or calling PDOStatement::closeCursor()#0 /var/www/loris/src/Database/Query.php(42): PDOStatement->execute()\n#1 /var/www/loris/src/Database/Query.php(33): LORIS\\Database\\Query->getIterator()\n#2 /var/www/loris/php/libraries/Database.class.inc(840): LORIS\\Database\\Query->getFirstRow()\n#3 /var/www/loris/php/libraries/Database.class.inc(979): Database->pselectRow()\n#4 /var/www/loris/php/libraries/Database.class.inc(1008): Database->pselectOne()\n#5 /var/www/loris/modules/imaging_browser/php/viewsession.class.inc(397): Database->pselectOneInt()\n#6 /var/www/loris/modules/imaging_browser/php/viewsession.class.inc(310): LORIS\\imaging_browser\\ViewSession->_getManualCaveatViolationsResolvedID()\n#7 /var/www/loris/modules/imaging_browser/php/viewsession.class.inc(116): LORIS\\imaging_browser\\ViewSession->_setFilesData()\n#8 /var/www/loris/php/libraries/NDB_Form.class.inc(133): LORIS\\imaging_browser\\ViewSession->setup()\n#9 /var/www/loris/src/Middleware/UserPageDecorationMiddleware.php(248): NDB_Form->handle()\n#10 /var/www/loris/src/Middleware/PageDecorationMiddleware.php(59): LORIS\\Middleware\\UserPageDecorationMiddleware->process()\n#11 /var/www/loris/php/libraries/NDB_Page.class.inc(726): LORIS\\Middleware\\PageDecorationMiddleware->process()\n#12 /var/www/loris/php/libraries/Module.class.inc(322): NDB_Page->process()\n#13 /var/www/loris/src/Middleware/ResponseGenerator.php(51): Module->handle()\n#14 /var/www/loris/src/Middleware/AuthMiddleware.php(64): LORIS\\Middleware\\ResponseGenerator->process()\n#15 /var/www/loris/src/Router/ModuleRouter.php(75): LORIS\\Middleware\\AuthMiddleware->process()\n#16 /var/www/loris/src/Middleware/ExceptionHandlingMiddleware.php(55): LORIS\\Router\\ModuleRouter->handle()\n#17 /var/www/loris/src/Router/BaseRouter.php(138): LORIS\\Middleware\\ExceptionHandlingMiddleware->process()\n#18 /var/www/loris/src/Middleware/ResponseGenerator.php(51): LORIS\\Router\\BaseRouter->handle()\n#19 /var/www/loris/src/Middleware/ContentLength.php(53): LORIS\\Middleware\\ResponseGenerator->process()\n#20 /var/www/loris/htdocs/index.php(74): LORIS\\Middleware\\ContentLength->process()\n#21 {main}, referer: https://cecile-dev-new.loris.ca/imaging_browser/?sequenceType=fMRI

Some viewSession pages are fine, others return this error.

Hhhhm, I've been having this error occasionally since #9334 got merged. Honestly it is hard to debug, and may be present on main ? Blows but I'll look into it when I have time.

let me check on main. I actually was about to do that but it was time of the LORIS meeting and then forgot to check...

@maximemulder I tested this bug and it is not present in main. I only see it in this PR.

modules/mri_violations/php/protocolcheckviolationprovisioner.class.inc

…lass.inc Co-authored-by: Cécile Madjar <cecile.madjar@mcin.ca>

…aximeMulder/Loris into improve-database-mri-scan-type

1. Drop the `DEFAULT 0` for foreign key fields 2. Change `DEFAULT '0'` (or `'1'`) to `DEFAULT 0` (or `1`) for other fields, which is more correct. Omitted the `mri_protocol.Scan_type` field as it is fixed in aces#9304 instead.

cmadjar

@maximemulder follow up review. I only tested what failed in the previous round

SQL patch fixed 🟢
MRI violation module fixed 🟢

All good and approving this PR.

@driusan @laemtl @maximemulder if this PR gets merged while I am away, can you also merge aces/Loris-MRI#1139 on the MRI side? Otherwise, the DB will be out of sync between the two repo since it touches SQL tables. Not sure what it will do to the tests.

stale

maximemulder mentioned this pull request Jun 26, 2024

Clean-up database scan type aces/Loris-MRI#1139

Merged

maximemulder changed the title ~~[cleanup] Improve mri_scan_type database~~ [database] Clean-up mri_scan_type Jun 26, 2024

maximemulder marked this pull request as ready for review June 26, 2024 17:46

cmadjar requested review from cmadjar and nicolasbrossard July 4, 2024 14:35

kongtiaowang closed this Jul 15, 2024

kongtiaowang reopened this Jul 15, 2024

maximemulder assigned maximemulder and unassigned maximemulder Jul 16, 2024

maximemulder mentioned this pull request Jul 17, 2024

[database] Improve schema defaults #9309

Merged

maximemulder assigned cmadjar and nicolasbrossard Jul 25, 2024

cmadjar requested changes Aug 1, 2024

View reviewed changes

maximemulder force-pushed the improve-database-mri-scan-type branch 6 times, most recently from b0e1c2b to e1545af Compare September 23, 2024 16:14

maximemulder changed the title ~~[database] Clean-up mri_scan_type~~ [database] Clean up mri_scan_type Sep 23, 2024

maximemulder force-pushed the improve-database-mri-scan-type branch from e1545af to e5ffba7 Compare September 23, 2024 19:02

maximemulder marked this pull request as draft September 23, 2024 19:58

maximemulder assigned maximemulder and unassigned cmadjar and nicolasbrossard Sep 23, 2024

maximemulder mentioned this pull request Sep 23, 2024

[imaging_browser] Error 500 caused by PDO error #9349

Closed

maximemulder force-pushed the improve-database-mri-scan-type branch from 9dd02fa to e5ffba7 Compare September 23, 2024 20:20

maximemulder mentioned this pull request Sep 23, 2024

[mri_violations] Broken query results serialization #9350

Closed

maximemulder force-pushed the improve-database-mri-scan-type branch from e5ffba7 to 90c10ef Compare September 24, 2024 18:55

maximemulder force-pushed the improve-database-mri-scan-type branch 2 times, most recently from 4dff326 to 52bea5f Compare September 26, 2024 16:30

maximemulder marked this pull request as ready for review September 26, 2024 16:32

nicolasbrossard previously requested changes Oct 23, 2024

View reviewed changes

SQL/0000-00-00-schema.sql Show resolved Hide resolved

maximemulder force-pushed the improve-database-mri-scan-type branch 2 times, most recently from cbd2b0a to 6deea72 Compare October 30, 2024 14:42

maximemuldermcgill and others added 3 commits November 11, 2024 16:18

add patch

4fe6ea4

address suggestions

95a6803

update single use tool

a1691df

maximemulder force-pushed the improve-database-mri-scan-type branch from 6deea72 to a1691df Compare November 11, 2024 21:18

cmadjar reviewed Nov 12, 2024

View reviewed changes

Update modules/mri_violations/php/protocolcheckviolationprovisioner.c…

c3ae03e

…lass.inc Co-authored-by: Cécile Madjar <cecile.madjar@mcin.ca>

maximemulder mentioned this pull request Nov 12, 2024

Make MRI violations log scan type non-nullable #9465

Open

maximemulder added 2 commits November 19, 2024 11:16

use change instead of rename

9b494d6

Merge branch 'improve-database-mri-scan-type' of https://github.com/M…

3b4c0ac

…aximeMulder/Loris into improve-database-mri-scan-type

maximemulder added Difficulty: Complex PR or issue that require a great effort to implementat, review, or test Area: Imaging PR or issue related to imaging Release: SQL patch PR that contains an SQL patch to apply labels Dec 2, 2024

cmadjar approved these changes Dec 3, 2024

View reviewed changes

CamilleBeau assigned driusan and unassigned maximemulder Jan 6, 2025

driusan merged commit a6b8648 into aces:main Jan 7, 2025
10 checks passed

		CONSTRAINT `FK_mri_violations_log_scan_type`
		FOREIGN KEY (`MriScanTypeID`) REFERENCES `mri_scan_type` (`ID`),

		CONSTRAINT `UK_mri_protocol_group_target`
		UNIQUE (`ProjectID`, `CohortID`, `Visit_label`)

	`Name` VARCHAR(255) NOT NULL,
	`MriScanTypeName` VARCHAR(255) NOT NULL,

[database] Clean up mri_scan_type #9304

[database] Clean up mri_scan_type #9304

Uh oh!

Conversation

maximemulder commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Testing instructions

Alternatives

Related issues

Uh oh!

maximemulder commented Jun 26, 2024

Uh oh!

maximemulder commented Jul 4, 2024

Uh oh!

maximemulder commented Jul 16, 2024

Uh oh!

cmadjar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximemulder Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximemulder Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximemulder Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximemulder Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cmadjar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximemulder Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

maximemulder commented Jun 26, 2024 •

edited

Loading

maximemulder Oct 23, 2024 •

edited

Loading

maximemulder Aug 1, 2024 •

edited

Loading

maximemulder Aug 1, 2024 •

edited

Loading

maximemulder Aug 19, 2024 •

edited

Loading

cmadjar left a comment •

edited

Loading

maximemulder Nov 12, 2024 •

edited

Loading

cmadjar left a comment •

edited

Loading