Revision ID in {node} and {node_revision} can get out of sync

Not sure if this a migration or views issue so putting it in the migration component with VDC tag to get both sets of eyes on it.

Hi, I'm facing the same problem right now. Some nodes are not deletable by bulk delete. Can't get to content because of the same error. It also happened before on some content pages where any of that broken nodes were printed. And yes, that's content migrated from d6 -> d7 -> d8.

Same problem here. After migrate from D7 to D8.1

hint: for a fast solution made some changes at the view:content which gives the /admin/content overview out. I added a content type filter and excluded some of my content types, then it works. For me my migrated content type "basic page" was broken, i dont know why so far but when i exclude them the view shows the /admin/content page.

Related, or duplicate: #2540876: WSOD with listing content after removing language

We ran into this error and it was related to revisions. The version ids did not match in the node and node_revisions table.

Example node: 13223

vid = 15431 in node_revision table
vid = 27810 in node

I believe it may have to do that our d7 site revisions were changing after migration. Then our d8 site was changing and then we migrated again.

If you're still diagnosing this issue, I will echo #10. We had this same exact problem after performing updates to content from one environment to another, and discovered that the vid wasn't getting updated to the latest version on the node_revisions table.

One node in the admin/content list can break the entire page. Make sure all nodes' vid lines up.

I've also seen this caused by migrate, but migrate is just using the entity API? The API shouldn't allow you to get to that point I think, so it is probably a bug/to be fixed in the entity storage.

OK let's move this to the entity system. However since migrate might be the only way to trigger this in core, leaving the migrate critical tag. Wild guess would be that it's to do with the revision migration.

Yeah, pretty sure it happens when trying to save a node with an explicitly provided revision ID but the node already exists with a different revision. We had this when we migrated nodes (default revision only) and kept the nid and revision_id while users also created new content on the site, so sometimes the nodes that migrate created already existed as manually created content, with a different revision. In some cases, even with a different node type, which resulted in additional fun behavior ;)

@Berdir: #2748609: [meta] Preserving auto-increment IDs on migration is fragile

This is a data-integrity issue, so should be critical whether it turns out to be migrate or not.

Question - has anyone seen this problem in a scenario other than the following (i.e., is #2748609: [meta] Preserving auto-increment IDs on migration is fragile the only known trigger)?

1. Run a Drupal-to-Drupal migration, preserving IDs.
2. Manually add content to the D8 site.
3. Run the Drupal-to-Drupal migration incrementally, importing new content.

Ok, this is pretty evil. Took me a while to make it fail and I'm not sure if it's something that needs to be fixed in entity storage or migrate.

To get to this point, what you have to do is load an existing entity, tell it that this is not a new revision and the default revision (the default value of those two flags when loading the default revision) but change the revision ID to a different value.

I'm not sure if that is a valid operation? Do we allow changing the revision ID of an existing revision? We don't support re-saving an entity with another ID either (no idea what would happen...)

This partially fixes it, but it currently leaves orphaned records with the old revision_id in the revision data table. I'm using the original revision id when updating the revisions table, then we match the old record and change its revision ID.

Note that there are various scenarios where saving fails, like trying to save as a new revision with a revision ID that already exists, both in another entity and in the existing one. Migrate currently doesn't protect against that in any way.

ive just ran into the same thing, i guess. im working on a custom d6>d8 migration.

although was able to build a custom view displaying my content - only the standard admin view didnt work. my view, though, didnt show anything as the content type of the nodes. titles were shown, but no type - although in the database itself, i could see types.

now, since i needed to move forward, i manually deleted all the rows in node and node_revision - but the error persists.

so now, this is not cool, haha. any hint how to recover my site without having to have a fresh installation?

ok, solved it myself. im sorry for this offtopic stuff in a thread about a feature. just in case anyone else stops by: ive just deleted any row in any table representing any form of node content. hence, revisions, fields etc. this did it.

ycle: Can you confirm whether your problem was triggered by the known scenario in #2748609: [meta] Preserving auto-increment IDs on migration is fragile? I.e., are your migrations attempting to preserve IDs (e.g., your node migrations have a nid: nid mapping), and did you manually create any content on the site before the last time your ran migration?

Discussed this with @xjm, @alexpott, @webchick, @effulgentsia and @cilifen.

We agreed this is critical.

Just the capability to mess things up with the API might not be critical on its own, but since migrate is doing it in core that tips it over the edge. We generally treat data-integrity and security issues in experimental modules as critical anyway (and this isn't reversible by uninstalling migrate).

@mikeryan i do not have nid: nid mappings in my migrations, but im actually working on a migration into a system with no existing entities for certain of the migrated types. means i have some taxonomy stuff, but the problem i faced was definitely related to nodes and/or paragraphs (paragraphs project) - while not having any preexisting nodes nor paragraphs.
although, good point, i did manually add one paragraph to one of the nodes in between two migration runs. after that, i performed a rollback, but could then not import again.
unfortunately i dont remember well enough for insisting that this caused the problem.
but i do remember that my drush migrate-... actions did not show any errors.

Forgot to add the tag.

I think we should detect this case, but rather than trying to fix it throw an exception explaining the problem. That will prevent data getting into this state, and at worst direct people here after some googling.

@ycle what you described sounds like exactly what ought to trigger this bug - editing content on a site in between migrations.

re #28: I'm not sure what you are doing with paragraphs, but you might try taking a look at #2809793: EntityReference migrate destination. Currently, none of the core destinations support the composite relationship required by paragraphs via its dependency on ERR.

Here's a new version that throws an exception instead. Lets see how many fails we have.

I started fixing those fails but noticed a problem. I don't fully understand \Drupal\KernelTests\Core\Entity\ContentEntityChangedTest::testRevisionChanged(), it is pretty strange in that it clones an entity and then mixes saving the old translation and the new one. But since those objects are no longer connected, I *think* that test is weird but while my fix makes at least partial sense, it's not the actual problem.-

What's actually the problem is that both the previous code and this new exception fail when updating a revision other than the default. Because we load the original, which is the default revision and compare against that. No idea how we didn't find that stuff earlier, but that basically means that #2809227: Store revision id in originalRevisionId property to use after revision id is updated is a blocker for this. \Drupal\KernelTests\Core\Entity\EntityRevisionTranslationTest::testTranslationValuesWhenSavingForwardRevisions() shows that perfectly.

Should we postpone on #2809227: Store revision id in originalRevisionId property to use after revision id is updated then?

Still postponed but the other issue is RTBC and wanted to verify that it solves our problem here as well.

Adding a combined patch, a separate patch for reviews and a test-only.patch

Also further simplified the test to the required minimum and added a similar check for preventing an ID change as well. That's actually tricky because if the ID changes it means we possibly fail to load ->original unless we have an ID that is another entity and then we might load something that's completely bogus, so included both checks.

test-only will fail obviously right now without the other patch committed but we can re-test when that is in.

See #2809227-119: Store revision id in originalRevisionId property to use after revision id is updated for an explanation of those test fails. This is a relatively simple workaround but we might want to do something else there. Also, ContentEntityChangedTest is kinda weird.

Rebased and removed the condition again for not having a loaded revision, as we now ensure it directly above. Lets see if this works. Also 8.3.x only for now, as it has only been committed there.

Forgot to update one renamed method.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -295,7 +295,22 @@ protected function doPreSave(EntityInterface $entity) {
   +    $id = parent::doPreSave($entity);
   

   I don't see where this is used below, so do we really need to assign it here?

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityChangedTest.php
   @@ -491,6 +491,10 @@ public function testRevisionChanged() {
   +    // Since this above a clone of the entity was saved and then this entity is
   

   The first part of this comment doesn't make too much sense :)

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -295,7 +295,22 @@ protected function doPreSave(EntityInterface $entity) {
-    return parent::doPreSave($entity);
+    $id = parent::doPreSave($entity);
+
...
+
+    return $id;
   }

We don't use it, but preSave() must return the ID :)

Will fix the comment.

Code looks good, nits on the comments:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -295,7 +295,22 @@ protected function doPreSave(EntityInterface $entity) {
   +      }
   +      // Do not allow to change the revision ID when resaving the current
   +      // revision.
   +      if (!$entity->isNewRevision() && $entity->getRevisionId() != $entity->getLoadedRevisionId()) {
   

   'Do not allow changing the revision ID' reads easier.

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityChangedTest.php
   @@ -491,6 +491,10 @@ public function testRevisionChanged() {
    
   +    // Since this above a clone of the entity was saved and then this entity is
   +    // saved again, we must update the revision ID to the current one.
   

   s/Since this/Since.

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityApiTest.php
   @@ -175,4 +175,38 @@ public function testEntityStorageExceptionHandling() {
   +   * Tests that resaving a revision with different ID throws an exception.
   

   'with a different revision ID' might be more explicit?

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityApiTest.php
   @@ -175,4 +175,38 @@ public function testEntityStorageExceptionHandling() {
   +   * Tests that resaving an entity with different ID throws an exception.
   

   Same here 'with a different entity ID'.

Then presumably people running migrations are going to start hitting exceptions, but I guess that's better than mis-matched revision IDs and we have open issues about incremental migrations anyway.

#45, what I meant is that I don't see why we're changing the existing return parent::doPreSave($entity); line if we're not using $id in the new if blocks.

Because the code needs to run *after* the parent, as that's where ->original is loaded. Maybe we can use ->getOriginalId() but nothing so far uses that for content entities afaik and I'm not sure if it is 1005 correct.

yeah, doing it afterwards is definitely required because Entity::getOriginalId() has this:

  /**
   * {@inheritdoc}
   */
  public function getOriginalId() {
    // By default, entities do not support renames and do not have original IDs.
    return NULL;
  }

Meaning, we can't use it. Not sure why the methods exists there in the first place and not just on config entities. Oh well ;)

Fixed the comments. The problem with the test method descriptions is that entity/revision ID means we're over 80 characters, leaving that out was my attempt at keeping it below that.

Cool, it just seemed weird to me but the explanation makes sense :)

So whilst this does not fix the issue it will prevent entities with mismatching ids and revision IDs from being saved and therefore prevent data integrity issues. I think that committing this might give us more error reports but at least then we'll be able to fix this before breaking someones site.

I was going to commit this but then I noticed that the issue summary update that @catch requested has never been done and it certainly is a little sparse. Let's get that done and then get this in.

Updated the issue summary - only a couple of sentences but I think that's probably OK? Bump back if not.

Committed cc3f37c and pushed to 8.3.x. Thanks!

As discussed in #2809227-149: Store revision id in originalRevisionId property to use after revision id is updated we were not sure if we should commit this to 8.2 as well, to quote alexpott in IRC: "It's against the BC policy to introduce the new exceptions. But the new exceptions are better than corrupt data."

Re-opening and postponing this on that other issue for now and re-opening that.

Ok, we decided to not commit that dependency, which means we also can't get this in. Back to fixed for 8.3.

I also encountered this migrating from D6 -> D8.
I tried to roolback the migration but ended up with two entries in node and node_revision.
Reading #25 I checked which tables belonged to my content type and deleted everything with the nid / entity_id found in node/ node_revision from:

• node
• node_revision
• node__field_XY and node_revision__field_XY (for evry field in the content type)
• node_access
• node_field_data
• node_field_revision

(I had no rows in node__body)

That helped.