Update db/project.xml per review

Make external_ids unique per project regardless of external org (name)
and adjust tests accordingly.
Make the new tables' audit_log_id field non-NULL, and cons up a new
audit_log entry for the external_id migrations to refer to.
Add comments clarifying PRIMARY_EXTERNAL_ORG.
Drop the old external_id fields' not-NULL constraint before clearing their
values -- previously omitted as this was tested only on empty tables!
This is in a separate changeSet as system_versioning_alter_history=1 is
needed, and we'd prefer not to set that during the main changeSet.

Author: jmarshall

db/project.xml

@@ -1302,7 +1302,7 @@
 			<column name="meta" type="LONGTEXT" />
 			<column name="audit_log_id" type="INT">
 				<constraints
-					nullable="true"
+					nullable="false"
 					foreignKeyName="FK_PARTICIPANT_EXTERNAL_ID_CHANGELOG_ID"
 					references="audit_log(id)" />
 			</column>
@@ -1315,7 +1315,7 @@
 		/>
 		<addUniqueConstraint
 			tableName="participant_external_id"
-			columnNames="project,name,external_id"
+			columnNames="project,external_id"
 			constraintName="UK_PARTICIPANT_EXTERNAL_ID_UNIQUE_EIDS"
 			validate="true"
 		/>
@@ -1347,7 +1347,7 @@
 			<column name="meta" type="LONGTEXT" />
 			<column name="audit_log_id" type="INT">
 				<constraints
-					nullable="true"
+					nullable="false"
 					foreignKeyName="FK_SAMPLE_EXTERNAL_ID_CHANGELOG_ID"
 					references="audit_log(id)" />
 			</column>
@@ -1360,7 +1360,7 @@
 		/>
 		<addUniqueConstraint
 			tableName="sample_external_id"
-			columnNames="project,name,external_id"
+			columnNames="project,external_id"
 			constraintName="UK_SAMPLE_EXTERNAL_ID_UNIQUE_EIDS"
 			validate="true"
 		/>
@@ -1375,10 +1375,28 @@
 		<sql>ALTER TABLE participant_external_id ADD SYSTEM VERSIONING;</sql>
 		<sql>ALTER TABLE sample_external_id ADD SYSTEM VERSIONING;</sql>
 
-		<sql>INSERT INTO participant_external_id (project, participant_id, name, external_id)
-		       SELECT project, id, '', external_id
-		       FROM participant
+		<!-- Migrate existing external_ids to the new tables, keyed by PRIMARY_EXTERNAL_ORG, i.e. '' -->
+		<sql>INSERT INTO audit_log (author, on_behalf_of, ar_guid, comment, auth_project, meta)
+		     VALUES ('liquibase', NULL, NULL, 'participant/sample external_id migration', NULL, NULL)
+		     RETURNING @audit_log_id := id;
+
+		     INSERT INTO participant_external_id (project, participant_id, name, external_id, audit_log_id)
+		       SELECT project, id, '', external_id, @audit_log_id
+		       FROM participant;
+
+		     INSERT INTO sample_external_id (project, sample_id, name, external_id, audit_log_id)
+		       SELECT project, id, '', external_id, @audit_log_id
+		       FROM sample;
 		</sql>
+	</changeSet>
+	<changeSet id="2024-06-11-drop-old-external-id-columns" author="john.marshall">
+		<sql>SET @@system_versioning_alter_history = 1;</sql>
+
+		<dropNotNullConstraint
+			tableName="participant"
+			columnName="external_id"
+			columnDataType="VARCHAR(255)"
+		/>
 		<sql>UPDATE participant SET external_id = NULL</sql>
 		<renameColumn
 			tableName="participant"
@@ -1387,10 +1405,12 @@
 			columnDataType="VARCHAR(255)"
 			remarks="Migration of external IDs to separate table"
 		/>
-		<sql>INSERT INTO sample_external_id (project, sample_id, name, external_id)
-		       SELECT project, id, '', external_id
-		       FROM sample
-		</sql>
+
+		<dropNotNullConstraint
+			tableName="sample"
+			columnName="external_id"
+			columnDataType="VARCHAR(255)"
+		/>
 		<sql>UPDATE sample SET external_id = NULL</sql>
 		<renameColumn
 			tableName="sample"

models/base.py

@@ -7,6 +7,7 @@
 
 
 # The primary {sample,participant}_external_id entry, previously {sample,participant}.external_id
+# TODO This will eventually be removed when all analysis endpoints have been updated to record an external org
 PRIMARY_EXTERNAL_ORG = ''
 
 

test/test_external_id.py

@@ -36,10 +36,16 @@ async def setUp(self):
     async def insert(self, participant_id, org, external_id):
         """Directly insert into participant_external_id table"""
         query = """
-        INSERT INTO participant_external_id (project, participant_id, name, external_id)
-        VALUES (:project, :id, :org, :external_id)
+        INSERT INTO participant_external_id (project, participant_id, name, external_id, audit_log_id)
+        VALUES (:project, :id, :org, :external_id, :audit_log_id)
         """
-        values = {'project': self.project_id, 'id': participant_id, 'org': org, 'external_id': external_id}
+        values = {
+            'project': self.project_id,
+            'id': participant_id,
+            'org': org,
+            'external_id': external_id,
+            'audit_log_id': await self.audit_log_id(),
+        }
         await self.connection.connection.execute(query, values)
 
     @run_as_sync
@@ -53,9 +59,10 @@ async def test_constraints(self):
         with self.assertRaises(IntegrityError):
             await self.insert(self.p1.id, 'CONTROL', 'Maxwell')
 
-        # Can have eids in lots of organisations, even if the eid duplicates one in a different org
+        # Can have eids in lots of organisations, but not if the eid duplicates one in a different org
         await self.insert(self.p1.id, 'OTHER1', 'Maxwell')
-        await self.insert(self.p1.id, 'OTHER2', '86')
+        with self.assertRaises(IntegrityError):
+            await self.insert(self.p1.id, 'OTHER2', '86')
 
         # Another participant can't have the same eid
         with self.assertRaises(IntegrityError):
@@ -76,11 +83,11 @@ async def test_insert(self):
 
         result = await self.player.upsert_participant(
             ParticipantUpsertInternal(
-                external_ids={PRIMARY_EXTERNAL_ORG: 'P10', 'A': 'A1', 'B': None, 'C': 'A1'},
+                external_ids={PRIMARY_EXTERNAL_ORG: 'P10', 'A': 'A1', 'B': None, 'C': 'C1'},
                 )
             )
         participants = await self.player.get_participants_by_ids([result.id])
-        self.assertDictEqual(participants[0].external_ids, {PRIMARY_EXTERNAL_ORG: 'P10', 'a': 'A1', 'c': 'A1'})
+        self.assertDictEqual(participants[0].external_ids, {PRIMARY_EXTERNAL_ORG: 'P10', 'a': 'A1', 'c': 'C1'})
 
     @run_as_sync
     async def test_update(self):
@@ -215,10 +222,16 @@ async def setUp(self):
     async def insert(self, sample_id, org, external_id):
         """Directly insert into sample_external_id table"""
         query = """
-        INSERT INTO sample_external_id (project, sample_id, name, external_id)
-        VALUES (:project, :id, :org, :external_id)
+        INSERT INTO sample_external_id (project, sample_id, name, external_id, audit_log_id)
+        VALUES (:project, :id, :org, :external_id, :audit_log_id)
         """
-        values = {'project': self.project_id, 'id': sample_id, 'org': org, 'external_id': external_id}
+        values = {
+            'project': self.project_id,
+            'id': sample_id,
+            'org': org,
+            'external_id': external_id,
+            'audit_log_id': await self.audit_log_id(),
+        }
         await self.connection.connection.execute(query, values)
 
     @run_as_sync
@@ -232,9 +245,10 @@ async def test_constraints(self):
         with self.assertRaises(IntegrityError):
             await self.insert(self.s1.id, 'CONTROL', 'Maxwell')
 
-        # Can have eids in lots of organisations, even if the eid duplicates one in a different org
+        # Can have eids in lots of organisations, but not if the eid duplicates one in a different org
         await self.insert(self.s1.id, 'OTHER1', 'Maxwell')
-        await self.insert(self.s1.id, 'OTHER2', '86')
+        with self.assertRaises(IntegrityError):
+            await self.insert(self.s1.id, 'OTHER2', '86')
 
         # Another sample can't have the same eid
         with self.assertRaises(IntegrityError):
@@ -255,11 +269,11 @@ async def test_insert(self):
 
         result = await self.slayer.upsert_sample(
             SampleUpsertInternal(
-                external_ids={PRIMARY_EXTERNAL_ORG: 'S10', 'A': 'A1', 'B': None, 'C': 'A1'},
+                external_ids={PRIMARY_EXTERNAL_ORG: 'S10', 'A': 'A1', 'B': None, 'C': 'C1'},
                 )
             )
         sample = await self.slayer.get_sample_by_id(result.id)
-        self.assertDictEqual(sample.external_ids, {PRIMARY_EXTERNAL_ORG: 'S10', 'a': 'A1', 'c': 'A1'})
+        self.assertDictEqual(sample.external_ids, {PRIMARY_EXTERNAL_ORG: 'S10', 'a': 'A1', 'c': 'C1'})
 
     @run_as_sync
     async def test_update(self):