revision uuids are not deleted upon deleting all revisions

Confirmed: When deleting all revisions, the 'delete revision' sequence never gets called in uuid_nodeapi()

The solution for how to deal with deletion of all node_revision uuids upon deleting a node will be complicated.

During the 'delete' part of the nodeapi, by the time we get to uuid, the revisions are already gone from the $node object, so there's no query we can build to find out an array of revision ids that we need to scan uuid_node_revision for.

The only $node properties of any interest to us that are set are:
["vid"]=>
string(2) "15"
["current_revision_id"]=>
string(2) "15"
["num_revisions"]=>
string(1) "2"

This information allows us to *at least* delete the latest revision uuid for the node, but we also see that there are 2 revisions, and they both need to be deleted. However, the node does not have any array of revision ids attached to it. further, as stated above, the rows are already gone from the node_revision table, so we can't query for them.

A quick improvement would be at the very least to add to the nodeapi 'delete' sequence a call to DELETE the last known revision. That would take care of the vast majority of cases.

The *right* fix for this is to ask why 'delete revision' isn't being fired by nodeapi before the 'delete' sequence is activated. But this adds expense to nodeapi.

Without waiting for fixes from on-high in this case, we are probably better off modifying the uuid_node_revision table by including the nid as a non-unique, non-indexed column in the table. No index: speeds things up, and we *only* ever scan for nid values on delete, so let's keep loads lean and mean by leaving indexes only on the other two columns.

Modify the queries which write data to the uuid_node_revision table to take account of the schema changes.

Modify the uuid.install file to account for the changes as well.

This way we can *guarantee* we can delete all the revision uuids for a node regardless of the various lacunae in the nodeapi implementation.

A fix for this is in the patch on this thread: http://drupal.org/node/918888

     case 'delete':
       db_query('DELETE FROM {uuid_node} WHERE nid = %d', $node->nid);
+      //delete the revision uuids for the node.
+      db_query('DELETE FROM {uuid_node_revisions} WHERE nid = %d', $node->nid);
       break;
     case 'delete revision':
       db_query('DELETE FROM {uuid_node_revisions} WHERE vid = %d', $node->vid);

Requires mods to uuid.install and the underlying schema, also detailed in the patch.

Here's a patch that sets up the schema and modifies the relevant queries such that uuid_node_revisions rows can be deleted on a node delete action.

I created the patch against uuid-DRUPAL-6--1 HEAD.

Any input on this fix for this bug from anyone?

Edited my comments and rolled a new patch against latest in CVS DRUPAL-6--1.

Are the code styling issues resolved with the new patch?

Any other thoughts on this? Anything blocking this? Would love to put this to bed so I can roll out new patches with this in place.

1. Since uuid_node_revisions_table_schema() generate table schema exclusively to node revisions, is there really a need for the $key argument? Will not it be always set to 'vid'? Also, can't we move the logic in uuid_node_revisions_table_schema() to hook_schema(). I'm not seeing much benefit from this being a separate function.

Ok. Integrated it all into uuid_schema.

2. It doesn't make sense changing uuid_update_6003() function, instead you should implement uuid_update_6004() and add your changes to database there.

Done. Although, of course, this will conflict with the other patches in which I'm also writing a _6004 function.

3. There are still some codestyle issues with inline comments. They should start with a space, capital letter and end with a period, they also need to wrap at 80 chars.

Never read about the 80 char requirement before, and it was not caught by the coder module. I have changed my page guide to 80 chars from 72, and changed the comment lengths.

4. Can you explain the need for the $active_only parameter added to node_get_by_revision_uuid()?

I wrote the following in a comment above that function:
* @todo One thing to ascertain here is whether this function
* should only return the node if the revision's uuid is the
* active revision for that node, or if it should also return
* the node even if the revision is not the active one. We
* make active-only the default implementation for now, just
* to be on the safe side.

So basically, I added that parameter as a future-oriented parameter, whereby which the function *could* support returning the node even if the revision passed is not the active revision. Right now, it can only return the node if the passed revision uuid is the current revision.

The "else" part of the function supported returning a node even if the revision uuid passed was NOT active, if a person were to also pass the $active_only as FALSE.

However, since there is no part of the code that relies on this, I have ripped it out of the current function.

I do think, however, that it should be discussed whether inactive revision uuids should be supported. Any such discussion would, of course, require the implementation of this patch first, since the nid column in the uuid_node_revisions table is necessary.

any thoughts from anyone?

@apmspooner. The behavior you are mentioning has nothing to do with what this patch is for. This patch is meant to address the fact that when a node is deleted, or all revisions are deleted (same thing), the node_revisions table keeps all the old vid<>uuid relationships, now orphaned and useless.

What you are writing about is a completely separate issue.

This thread is about the fact that uuid module has a bug that causes node_revisions table to never get cleaned out.

FYI @klktrk, this issue is not related to the deletion of nodes at all. I only edited existing nodes or created new ones and the error pops up. No nodes were deleted in the process of my testing.

EXACTLY, so why are you posting your comments on your issue in the wrong thread? Please don't hijack another issue. (Please take a look at some of the discussion here http://drupal.org/node/895622 and here http://drupal.org/node/918888. There is a fundamental misconception on the part of the uuid developers when it comes to updates to a node with revisions. A good workaround patch was written and comitted, but it's not the real solution, as the real conceptual work behind the idea of updating a revision has not been worked out.)

Now, can we get back to the topic at hand? I take it everyone is content to leave this glaring bug in the uuid module? Nobody is bothered by the fact that entries in the uuid_node_revisions table don't get deleted? Or am I simply the only person who has ever looked at the contents of that table? :-)

Thanks @recidive. Glad to see it checked in. It was a fairly far-reaching patch, touching a lot of different lines of code, so this helps a lot. No worries about the commit message.

Forgive my ignorance if I'm dead wrong on this but shouldn't the UUID be the primary key in the uuid_node and uuid_node_revisions tables? As it stands nid is primary in uuid_node and vid is primary in uuid_node_revisions which I think is whats causing my bug on node creation/edit:

@apmsooner: You may want to add a new issue for this. The reason the current uuid_node.nid and uuid_node_revisions.vid fields are primary is that those are the fields used in the lookups in queries, not the uuid fields. While on a pure theory level, perhaps there are other fields that should be primary, if you look through the selects and joins in the uuid module, you'll see .vid and .nid referenced and used for almost all lookups. That's why they're primary, and not some other field.

If you still disagree, please feel free to open a new issue and discuss it there.

user warning: Duplicate entry '766' for key 'PRIMARY' query: INSERT INTO uuid_node_revisions (vid, uuid, nid) VALUES (766, '75c9fdfc-d55a-11df-be90-002219221c49', 752) in /home/autop5/public_html/webmanager/sites/all/modules/uuid/uuid.module on line 72.

This has nothing to do with this patch. You yourself have opened a ticket to address it elsewhere: http://drupal.org/node/934142

Thanks for your reply.