Bug #9034

Unknown foreign key breaks arMigration0121 (» 2.2.0)

Added by Jesús García Crespo about 5 years ago. Updated almost 5 years ago.

Status:VerifiedStart date:10/05/2015
Priority:CriticalDue date:
Assignee:José Raddaoui Marín% Done:

0%

Category:Data model / ORM
Target version:Release 2.2.1
Google Code Legacy ID: Tested version:
Sponsored:No Requires documentation:

Description

User report here: https://groups.google.com/forum/#!topic/ica-atom-users/aFArUyYgLoY

It seems that PR106 added a migration class called arMigration0121.class.php but accidentally introduced the same changes in an earlier migration: arMigration0098.class.php.

I haven't checked that that's the actual issue causing the migration to fail but there are big chances. I'll ask the user to send us a database dump, that would help.

Please notice that PR106 introduced a different issue but perhaps related, described in #8784.

History

#1 Updated by Jesús García Crespo about 5 years ago

  • Subject changed from Database migration fails for user upgrading to 2.1.2 » 2.2.0 to Database migration fails for user upgrading 2.1.2 » 2.2.0

#2 Updated by Jesús García Crespo about 5 years ago

  • Subject changed from Database migration fails for user upgrading 2.1.2 » 2.2.0 to Unknown foreign key breaks arMigration0121 (» 2.2.0)

It looks like some databases have duplicated constraints for reasons that I don't know yet, e.g. the following is the list of foreign key constraints in the event table of one of our users.

`event_FK_1` FOREIGN KEY (`id`) REFERENCES `object` (`id`) ON DELETE CASCADE,
`event_FK_2` FOREIGN KEY (`type_id`) REFERENCES `term` (`id`) ON DELETE CASCADE,
`event_FK_3` FOREIGN KEY (`information_object_id`) REFERENCES `information_object` (`id`) ON DELETE CASCADE,
`event_FK_4` FOREIGN KEY (`actor_id`) REFERENCES `actor` (`id`),
`event_ibfk_1` FOREIGN KEY (`id`) REFERENCES `object` (`id`) ON DELETE CASCADE,
`event_ibfk_2` FOREIGN KEY (`type_id`) REFERENCES `term` (`id`) ON DELETE CASCADE,
`event_ibfk_3` FOREIGN KEY (`information_object_id`) REFERENCES `information_object` (`id`) ON DELETE CASCADE,
`event_ibfk_4` FOREIGN KEY (`actor_id`) REFERENCES `actor` (`id`)

arMigration0121 tries deletes the event_FK_3 foreign key before deleting the .information_object_id columnt, but event_ibfk_3 still being there causes the issue.

We should:

1) Investigate why some databases have unknown constraint names. I have confirmed that we've never used the _ibfk naming system before, the name could perhaps be assigned by MySQL when we don't give it one. That could have happened in previous migrations, it'd be good to check that. Across multiple database, I've found that a FK constraint named information_object_ibfk_1 is pretty common in the information_object table, but not always.

2) We should probably abstract away the removal of constraints, maybe as part of dropColumn() in QubitMigrate? information_schema seems to be the right way to do reflection.

#3 Updated by Jesús García Crespo about 5 years ago

  • Status changed from New to In progress

#4 Updated by Nick Wilkinson about 5 years ago

  • Assignee changed from Jesús García Crespo to José Raddaoui Marín

#5 Updated by José Raddaoui Marín about 5 years ago

  • Status changed from In progress to QA/Review
  • Assignee changed from José Raddaoui Marín to Jesús García Crespo

Hi Sevein, I don't really know why that event table has all those duplications, but I've found some answers to the two TODO points:

1) It looks like that naming system is used when you add the foreing key without adding the constraint name:

// With CONSTRAINT name
ALTER TABLE `rights` ADD CONSTRAINT `rights_FK_3` FOREIGN KEY (`rights_holder_id`) REFERENCES `actor` (`id`) ON DELETE SET NULL;

// Without CONSTRAINT name
ALTER TABLE `rights` ADD FOREIGN KEY (`rights_holder_id`) REFERENCES `actor` (`id`) ON DELETE SET NULL;

Something similar happens for the indexes, where it will use the column name for the key name:

// With INDEX name
CREATE INDEX `rights_FI_3` ON `rights` (`rights_holder_id`);

// Without INDEX name
ALTER TABLE `rights` ADD INDEX (`rights_holder_id`);

Both queries without name are used in QubitMigrate::addColumn(), I think that's why you see one in the information_object table, it was added here:

https://github.com/artefactual/atom/blob/qa/2.3.x/lib/task/migrate/migrations/arMigration0094.class.php#L41

I've added a new one using the same method (test_id) and it used that naming system:

CREATE TABLE `information_object` (
  `id` int(11) NOT NULL,
  `identifier` varchar(1024) DEFAULT NULL,
  `oai_local_identifier` int(11) NOT NULL AUTO_INCREMENT,
  `level_of_description_id` int(11) DEFAULT NULL,
  `collection_type_id` int(11) DEFAULT NULL,
  `repository_id` int(11) DEFAULT NULL,
  `parent_id` int(11) DEFAULT NULL,
  `description_status_id` int(11) DEFAULT NULL,
  `description_detail_id` int(11) DEFAULT NULL,
  `description_identifier` varchar(1024) DEFAULT NULL,
  `source_standard` varchar(1024) DEFAULT NULL,
  `test_id` int(11) DEFAULT NULL,
  `display_standard_id` int(11) DEFAULT NULL,
  `lft` int(11) NOT NULL,
  `rgt` int(11) NOT NULL,
  `source_culture` varchar(7) NOT NULL,
  PRIMARY KEY (`id`),
  UNIQUE KEY `information_object_U_1` (`oai_local_identifier`),
  KEY `information_object_FI_2` (`level_of_description_id`),
  KEY `information_object_FI_3` (`collection_type_id`),
  KEY `information_object_FI_4` (`repository_id`),
  KEY `information_object_FI_5` (`parent_id`),
  KEY `information_object_FI_6` (`description_status_id`),
  KEY `information_object_FI_7` (`description_detail_id`),
  KEY `display_standard_id` (`display_standard_id`),
  KEY `test_id` (`test_id`),
  CONSTRAINT `information_object_ibfk_2` FOREIGN KEY (`test_id`) REFERENCES `term` (`id`) ON DELETE SET NULL,
  CONSTRAINT `information_object_FK_1` FOREIGN KEY (`id`) REFERENCES `object` (`id`) ON DELETE CASCADE,
  CONSTRAINT `information_object_FK_2` FOREIGN KEY (`level_of_description_id`) REFERENCES `term` (`id`) ON DELETE SET NULL,
  CONSTRAINT `information_object_FK_3` FOREIGN KEY (`collection_type_id`) REFERENCES `term` (`id`),
  CONSTRAINT `information_object_FK_4` FOREIGN KEY (`repository_id`) REFERENCES `repository` (`id`),
  CONSTRAINT `information_object_FK_5` FOREIGN KEY (`parent_id`) REFERENCES `information_object` (`id`),
  CONSTRAINT `information_object_FK_6` FOREIGN KEY (`description_status_id`) REFERENCES `term` (`id`) ON DELETE SET NULL,
  CONSTRAINT `information_object_FK_7` FOREIGN KEY (`description_detail_id`) REFERENCES `term` (`id`) ON DELETE SET NULL,
  CONSTRAINT `information_object_ibfk_1` FOREIGN KEY (`display_standard_id`) REFERENCES `term` (`id`) ON DELETE SET NULL
) ENGINE=InnoDB AUTO_INCREMENT=28079 DEFAULT CHARSET=utf8

2) That's already done in that function, the problem is that we weren't using it. It will remove all constraints and indexes related to the column, regardless of their name.

https://github.com/artefactual/atom/blob/qa/2.3.x/lib/task/migrate/QubitMigrate.class.php#L718
https://github.com/artefactual/atom/blob/qa/2.3.x/lib/task/migrate/QubitMigrate.class.php#L733

I think the solution is always use those functions and don't care about the indexes and constraints names.

#6 Updated by Jesús García Crespo almost 5 years ago

That's probably right, Radda.
The nice thing about having consistent constraint names is that it's easier to identify the constraint when you see a constraint violation error in the logs, e.g. when you see an error like the following:

SQLSTATE[HY000]: General error: 1553 Cannot drop index 'information_object_ibfk_4': needed in a foreign key constraint

... you'll have to go to the database and figure out the constraint details. That's okay most of the times, but if we have a user reporting a problem like this we'll have to ask him to run some extra steps to discover the constraint details. We should probably add some notes in this regards in atom-docs.git.

So I think it's okay to give up on constraint names, but I would recommend you to CC looking for consensus. Also it wouldn't hurt to go over the existing migrations and see if it's required do some changes, e.g. I know that both arMigration0113 and arMigration0134 tried to drop a column without using dropColumn()... big mess!

#7 Updated by Jesús García Crespo almost 5 years ago

  • Status changed from QA/Review to Feedback
  • Assignee changed from Jesús García Crespo to José Raddaoui Marín

#8 Updated by Dan Gillean almost 5 years ago

  • Status changed from Feedback to Verified

Also available in: Atom PDF