Patch suggesting approaches to #915118, #902176, #895622, #915254. #913820

Okay, so I thought maybe I should explain some of these rewrites, additions, inline. I added line numbers to the patch to make it easier to pinpoint sections:

Create form widgets for uuid generation version selector and the default uuid namespace (to be displayed, probably not necessary). Set this to collapsed so as to not get in the way.

I did choose to make the default uuid version v4 (pseudo random, see line 237).

 12 +  $form['format'] = array(
 13 +  	'#type' => 'fieldset',
 14 +  	'#title' => t('UUID Format'),
 15 +  	'#collapsible' => TRUE,
 16 +  	'#collapsed' => TRUE
 17 +  );
 18 +  $form['format']['uuid_namespace'] = array(
 19 +  	'#type' => 'textfield',
 20 +  	'#title' => t('UUID Namespace'),
 21 +  	'#default_value' => variable_get('uuid_namespace', uuid_uuid()),
 22 +  	'#description' => t('The namespace must be in a RFC 4211 compliant UUID format. Most people will want to leave this alone as it is generated during the installation of the module. It is only relevant for v3 and v5 UUIDs.')
 23 +  );
 24 +	$form['format']['uuid_version'] = array(
 25 +		'#type' => 'select',
 26 +		'#options' => array(
 27 +			'v3' => t('Version 3 (namespace and prefix)'), 
 28 +			'v4' => t('Version 4 (pseudo random)'), 
 29 +			'v5' => t('Version 5 (namespace and prefix)')
 30 +		),
 31 +		'#title' => t('UUID Version'),
 32 +		'#default_value' => variable_get('uuid_version', UUID_VERSION_DEFAULT),
 33 +	);

Added a @todo. Sync only adds uuids. It does not remove them from content types that have been de-activated. I can see good points with both approaches, but wanted to make a note of it in the code.

 34    $form['content'] = array(
 35      '#type' => 'fieldset',
 36      '#title' => t('Content settings'),
 37 @@ -25,6 +48,15 @@ function uuid_admin() {
 38      '#description' => t('Content of these types will have UUIDs automatically generated.'),
 39      '#required' => FALSE,
 40    );
 41 +  /** @todo currently, the sync method is only additive. If
 42 +   * a node type is deactivated, revisions of the nodes will
 43 +   * still have uuids. Shouldn't the sync method remove uuids
 44 +   * for content that has been unchecked, including
 45 +   * revisions?
 46 +   * Proposed: Add a submit handler to this element, and
 47 +   * expand the sync function to remove uuids for types that
 48 +   * have been deactivated.
 49 +   */
 50  

The "NOT IN (potentially massive subselect)" syntax will collapse on a site with a sizeable data collection. In fact, there is a limit to how many parameters can go in the NOT IN () parens. And yes, I have hit that limit before. So, rewriting with the faster and more scaleable LEFT JOIN. You'll see a few more of these rewrites.

 56      // Nodes.
 57 -    $result = db_query("SELECT nid FROM {node} WHERE type IN (" . $placeholders . ") AND nid NOT IN (SELECT nid FROM {uuid_node})", $types);
 58 +    $result = db_query('SELECT n.nid FROM node AS n	LEFT JOIN uuid_node AS un ON un.nid=n.nid	WHERE type IN ('. $placeholders .') AND un.nid IS NULL;', $types);

Again, rewriting the subselect into the more efficient and scalable LEFT JOIN

Also, note the inserts are rewritten (line 69) to handle the new schema for the uuid_node_revisions table. The nid column was added (though redundant) in order to allow revisions uuids to be deleted when nodes are deleted. (You'll see why in my ticket on the issue http://drupal.org/node/915254).

 63      // Revisions.
 64 -    $result = db_query("SELECT nr.vid AS vid FROM {node_revisions} AS nr INNER JOIN {node} n ON nr.nid = n.nid WHERE n.type IN (" . $placeholders . ") AND nr.vid NOT IN (SELECT vid FROM {uuid_node_revisions})", $types);
 65 +    $result = db_query('SELECT nr.vid AS vid, nr.nid AS nid FROM {node_revisions} AS nr INNER JOIN {node} n ON nr.nid = n.nid LEFT JOIN {uuid_node_revisions} AS unr ON unr.nid=nr.nid AND unr.vid=nr.vid WHERE 
 66 +		n.type IN ('. $placeholders .') AND	unr.nid IS NULL', $types);
 67      while ($item = db_fetch_object($result)) {
 68 -      db_query("INSERT INTO {uuid_node_revisions} (vid, uuid) VALUES(%d, '%s')", $item->vid, uuid_uuid());
 69 +      db_query("INSERT INTO {uuid_node_revisions} (vid, uuid, nid) VALUES(%d, '%s', %d)", $item->vid, uuid_uuid(), $item->nid);
 70      }
 71    }

Added a uuid generating class file to encapsulate different methods of generating uuids. I think we could add methods for mysql or PECL to this class.

 73 Only in uuid: uuid.class.inc

The setting for the site's uuid namespace should be set when someone enables the module.

 90 diff -urp uuid.beta2/uuid.install uuid/uuid.install
 91 --- uuid.beta2/uuid.install	2010-09-08 12:07:14.000000000 -0400
 92 +++ uuid/uuid.install	2010-09-21 21:29:02.000000000 -0400
 93 @@ -11,6 +11,7 @@
 94   */
 95  function uuid_enable() {
 96    module_load_include('inc', 'uuid', 'uuid.admin');
 97 +  uuid_init_namespace();
 98    uuid_sync();

Of course, the schema for uuid_node_revisions needs so support a column with nids, so the schema needs to be customized. So I broke that out into its own function, which wraps around the default uuid_table_schema() and just extends it:

101 @@ -28,7 +29,7 @@ function uuid_install() {
102  function uuid_schema() {
103    return array(
104      'uuid_node' => uuid_table_schema('node', 'nid'),
105 -    'uuid_node_revisions' => uuid_table_schema('node_revisions', 'vid'),
106 +    'uuid_node_revisions' => uuid_node_revisions_table_schema('vid'),

Handle issue http://drupal.org/node/913820

114 -        'type' => 'varchar',
115 +        'type' => 'char',

Custom schema for the uuid_node_revisions table. As you see, it first calls the default schema, and then just adds fields to it, so the basics remain the same.

131 +function uuid_node_revisions_table_schema($key = 'key') {
132 +	// get the standard schema
133 +	$schema = uuid_table_schema('uuid_node_revisions', $key);
134 +	
135 +	// add the node id column
136 +	if (is_array($schema) && isset($schema['fields'])) {
137 +		// no index needed on this column as it will only 
138 +		// be used during a node delete.
139 +		$schema['fields']['nid'] = array(
140 +			'type' => 'int',
141 +			'length' => 10,
142 +			'unsigned' => TRUE,
143 +			'not null' => TRUE,
144 +			'default' => 0,
145 +			'description' => 'The revision\'s node id. Needed for delete node operations.',
146 +		);
147 +	}
148 +	return $schema;
149 +}

Of course, I have to delete the new variables for uuid_version and uuid_namespace when the module is uninstalled

152   * Implementation of hook_uninstall().
153   */
154  function uuid_uninstall() {
155 @@ -78,11 +108,19 @@ function uuid_uninstall() {
156    variable_del('uuid_automatic_for_users');
157    variable_del('uuid_automatic_for_taxonomy');
158    variable_del('uuid_automatic_for_comments');
159 +  variable_del('uuid_version');
160 +  variable_del('uuid_namespace');

This should get called when the module is enabled in order to set a default uuid namespace.

166 +function uuid_init_namespace() {
167 +	if (!variable_get('uuid_namespace', '')) {
168 +		variable_set('uuid_namespace', uuid_uuid());
169 +	}
170 +}
171 +

The changes in the schema necessitated a lot of rewriting of the forthcoming update_6003() hook.

I think it's pretty self-explanatory.

179 - * Create uuid_comment table.
180 + * 1. Create uuid_comment table.
181 + * 
182 + * 2. fix the column definitions for uuid columns in all tables
183 + * to use the more efficient char spec.
184   */
185  function uuid_update_6003() {
186    $ret = array();
187  

First, I just wrapped the create uuid_comments in a "if table doesn't exist" statement, cause it's easy to do and makes for fewer SQL errors if the hook is re-run.

188 -  db_create_table($ret, 'uuid_comments', uuid_table_schema('comments', 'cid'));
189 +	if (!db_table_exists('uuid_comments')) {
190 +	  db_create_table($ret, 'uuid_comments', uuid_table_schema('comments', 'cid'));
191 +  }

Change the column def from varchar to char for any existing schema tables

192 +
193 +	$tables = uuid_schema();//
194 +	$tables = array_keys($tables);
195 +	// use what's in uuid_table_schema in order to be consistent
196 +	$field_specs = uuid_table_schema('uuid_node');
197 +	$spec = $field_specs['fields']['uuid'];
198 +
199 +	while(list(,$table) = each($tables)) {
200 +		if (db_table_exists($table)) {
201 +		  db_change_field($ret, $table, 'uuid', 'uuid', $spec);
202 +		}
203 +	}

Add the nid column to any previously existing uuid_node_revisions table

204 +
205 +	if (db_table_exists('uuid_node_revisions')) {
206 +		// use what's in uuid_table_schema in order to be consistent
207 +		$field_specs = uuid_node_revisions_table_schema('vid');
208 +		$spec = $field_specs['fields']['nid'];
209 +		db_add_field($ret, 'uuid_node_revisions', 'nid', $spec);

Add node ids to the new column, for revisions that exist, but now have a default value of 0 in uuid_node_revisions due to the new column having been added.

212 +		$result = db_query('SELECT nr.nid, nr.vid FROM {node_revisions} AS nr	LEFT JOIN {uuid_node_revisions} AS unr ON unr.vid=nr.vid WHERE unr.nid=%d', 0);
213 +    while ($item = db_fetch_object($result)) {
214 +      $ret[] = update_sql('UPDATE {uuid_node_revisions} SET nid='.(int) $item->nid .' WHERE vid='. (int) $item->vid);
215 +    }

Add uuid_node_revision rows for rows that don't exist, but should (fixes node syncing with regard to the new column)

217 +		$result = db_query('SELECT nr.nid, nr.vid FROM {node_revisions} AS nr	LEFT JOIN {uuid_node_revisions} AS unr ON unr.vid=nr.vid WHERE unr.nid IS NULL');
218 +    while ($item = db_fetch_object($result)) {
219 +      $ret[] = update_sql(sprintf("INSERT INTO {uuid_node_revisions} (vid, uuid, nid) VALUES(%d, '%s', %d)", $item->vid, uuid_uuid(), $item->nid));
220 +    }

Delete any orphaned revision vid, uuid pairs. there is no way to know which nodes they used to belong to, and any revisions that they used to be attached to are now long gone, so they cannot be re-attached. Best to delete them. This should not be a problem moving forward since implementing http://drupal.org/node/915254

222 +    $ret[] = update_sql('DELETE FROM {uuid_node_revisions} WHERE nid=0');

Setting the default UUID version to v4 (pseudo-random). Probably a lot of discussion to be had about this.

235   * Main module functions for the uuid module.
236   */
237 +define('UUID_VERSION_DEFAULT', 'v4');

Efficiency: especially/critically important on the load op of nodeapi.

243    switch ($op) {
244      case 'load':
245 -      return array(
246 -        'uuid' => db_result(db_query('SELECT uuid FROM {uuid_node} WHERE nid = %d', $node->nid)),
247 -        'revision_uuid' => db_result(db_query('SELECT uuid FROM {uuid_node_revisions} WHERE vid = %d', $node->vid))
248 -      );
249 +			/**
250 +			 * Rewrote this into one query. Each separate query
251 +			 * takes 8.9ms = 18.8ms. A join takes 9.1ms, so we save
252 +			 * 50% over the previous 2 query code. On nodeapi 'load'
253 +			 * that can add up a lot in a long node list.
254 +			 */
255 +			if ($arr = db_fetch_array(db_query('SELECT un.uuid AS uuid, unr.uuid AS revision_uuid FROM {uuid_node} un LEFT JOIN {uuid_node_revisions} unr ON unr.vid=%d WHERE un.nid=%d', $node->vid, $node->nid))) {
256 +				return $arr;
257 +			}
258 +			else {
259 +				return array('uuid'=>FALSE, 'revision_uuid'=>FALSE);
260 +			}

Account for the changes in the uuid_node_revisions schema (the added nid column).

Also, in appropriate situations (a node is being loaded or may likely be re-used down the line, if a uuid is generated for its revision, attach it to the node object so it's available later on, don't just stick in the database but leave the node unaware of its new property (line 274, 282 etc)

268 -          db_query("INSERT INTO {uuid_node_revisions} (vid, uuid) VALUES (%d, '%s')", $node->vid, $node->revision_uuid);
269 +          db_query("INSERT INTO {uuid_node_revisions} (vid, uuid, nid) VALUES (%d, '%s', %d)", $node->vid, $node->revision_uuid, $node->nid);
270          }
271          else {
272 -          db_query("INSERT INTO {uuid_node_revisions} (vid, uuid) VALUES (%d, '%s')", $node->vid, uuid_uuid());
273 +					//attach new revision uuid to node, don't just insert
274 +					$node->revision_uuid = uuid_uuid();
275 +          db_query("INSERT INTO {uuid_node_revisions} (vid, uuid, nid) VALUES (%d, '%s', %d)", $node->vid, $node->revision_uuid, $node->nid);
276          }
277        }
278        else if (in_array($node->type, $automatic_types)) {
279          db_query("INSERT INTO {uuid_node} (nid, uuid) VALUES (%d, '%s')", $node->nid, uuid_uuid());
280 -        db_query("INSERT INTO {uuid_node_revisions} (vid, uuid) VALUES (%d, '%s')", $node->vid, uuid_uuid());
281 +				//attach new revision uuid to node, don't just insert
282 +				$node->revision_uuid = uuid_uuid();
283 +        db_query("INSERT INTO {uuid_node_revisions} (vid, uuid, nid) VALUES (%d, '%s', %d)", $node->vid, $node->revision_uuid, $node->nid);
284        }
285        break;
286      case 'update':

Delete from uuid_node_revision before updating a node revision. There should only ever be one vid::uuid pair, so get rid of any pre-existing ones before doing the insert. We could avail ourselves of UPDATE... and check for an error and if not updated, then do a insert, but this seemed cleaner to me.

287 -      if (isset($node->revision)) {
288 -        if (isset($node->revision_uuid) && uuid_is_valid($node->revision_uuid)) {
289 -          db_query("INSERT INTO {uuid_node_revisions} (vid, uuid) VALUES (%d, '%s')", $node->vid, $node->revision_uuid);
290 -        }
291 -        else {
292 -          db_query("INSERT INTO {uuid_node_revisions} (vid, uuid) VALUES (%d, '%s')", $node->vid, uuid_uuid());
293 +			if (isset($node->revision)) {
294 +				db_query("DELETE FROM {uuid_node_revisions} WHERE vid=%d", $node->vid);
295 +
296 +        if (!isset($node->revision_uuid) || !uuid_is_valid($node->revision_uuid)) {
297 +					$node->revision_uuid = uuid_uuid();
298          }
299 +
300 +        db_query("INSERT INTO {uuid_node_revisions} (vid, uuid, nid) VALUES (%d, '%s', %d)", $node->vid, $node->revision_uuid, $node->nid);

Here's where we get the payoff from having added the nid column to the uuid_node_revisions table: We can finally delete all the rows that belong to the revisions of a deleted node. As you'll see in http://drupal.org/node/915254, these cannot be deleted by any other means than having the nid column due to uuid coming fairly late in the nodeapi cycle, and there being no array of available revision ids attached to the node.

301        }
302        break;
303      case 'delete':
304        db_query('DELETE FROM {uuid_node} WHERE nid = %d', $node->nid);
305 +      //delete the revision uuids for the node.
306 +      db_query('DELETE FROM {uuid_node_revisions} WHERE nid = %d', $node->nid);
307        break;
308      case 'delete revision':
309        db_query('DELETE FROM {uuid_node_revisions} WHERE vid = %d', $node->vid);

Added some documentation, and rewrote the query in order to take advantage of the new schema in the uuid_node_revisions table.

The @todo raises a somewhat thorny question about how this function should behave. I left it default at its current implementation, but rewrote it to allow for another possibility.

310 @@ -102,17 +118,34 @@ function node_get_by_uuid($uuid) {
311  /**
312   * Returns the node associated with a revision UUID.
313   *
314 + * @todo One thing to ascertain here is whether this function
315 + * should only return the node if the revision's uuid is the
316 + * active revision for that node, or if it should also return
317 + * the node even if the revision is not the active one. We 
318 + * make this the default implementation for now, just to be on
319 + * the safe side.
320 + *
321 + * @param string $revision_uuid The uuid to use for the node
322 + * lookup
323 + * @param bool $active_only Should a node only be returned if
324 + * the passed revision uuid is the currently active one for
325 + * the node? Default: TRUE
326   * @return
327   *   Either the $node object, or FALSE on failure.
328   */
329 -function node_get_by_revision_uuid($revision_uuid) {
330 -  $node = db_fetch_object(db_query(db_rewrite_sql(
331 -    "SELECT n.nid, n.vid
332 -     FROM {node} AS n
333 -     INNER JOIN {uuid_node_revisions} AS unr ON n.vid = unr.vid
334 -     WHERE unr.uuid = '%s'"),
335 -    $revision_uuid
336 -  ));
337 +function node_get_by_revision_uuid($revision_uuid, $active_only=TRUE) {
338 +	if ($active_only) {
339 +	  $node = db_fetch_object(db_query(db_rewrite_sql(
340 +			"SELECT n.nid, n.vid
341 +			 FROM {node} AS n
342 +			 INNER JOIN {uuid_node_revisions} AS unr ON n.vid = unr.vid
343 +			 WHERE unr.uuid = '%s'"),
344 +			$revision_uuid
345 +		));
346 +	}
347 +	else {
348 +	  $node = db_fetch_object(db_query("SELECT nid, vid FROM {uuid_node_revisions} WHERE uuid = '%s'", $revision_uuid));
349 +	}

Rewrite uuid_uuid() to use the uuid generator class. I like some of the subsequent thoughts I read about the utility of the MYSQL function, so perhaps the approach is to expand the class methods. The mysql uuid() function only generates v4 uuids, though, right?

376 -function uuid_uuid() {
377 -  // If we're not using postgres, the database will give us a UUID.
378 -  if ($GLOBALS['db_type'] != 'pgsql') {
379 -    return db_result(db_query('SELECT UUID()'));
380 -  }
381 -
382 -  // If the function provided by the uuid pecl extension is available, use that.
383 -  if(function_exists('uuid_create')) {
384 -    return uuid_create(UUID_TYPE_DEFAULT);
385 -  }
386 -
387 -  // Fall back to generating a Universally Unique IDentifier version 4.
388 -  // Base on http://php.net/manual/en/function.uniqid.php#65879
389 -  // The field names refer to RFC 4122 section 4.1.2.
390 -  return sprintf('%04x%04x-%04x-%03x4-%04x-%04x%04x%04x',
391 -    // 32 bits for "time_low".
392 -    mt_rand(0, 65535), mt_rand(0, 65535),
393 -    // 16 bits for "time_mid".
394 -    mt_rand(0, 65535),
395 -    // 12 bits before the 0100 of (version) 4 for "time_hi_and_version".
396 -    mt_rand(0, 4095),
397 -    bindec(substr_replace(sprintf('%016b', mt_rand(0, 65535)), '01', 6, 2)),
398 -    // 8 bits, the last two of which (positions 6 and 7) are 01, for "clk_seq_hi_res"
399 -    // (hence, the 2nd hex digit after the 3rd hyphen can only be 1, 5, 9 or d)
400 -    // 8 bits for "clk_seq_low" 48 bits for "node".
401 -    mt_rand(0, 65535), mt_rand(0, 65535), mt_rand(0, 65535)
402 -  );
403 +function uuid_uuid($uuid_version='') {
404 +	//static so fewer system calls via php_uname()
405 +	static $prefix = '';
406 +  if (empty($uuid_version)) {
407 +  	$uuid_version = variable_get('uuid_version', UUID_VERSION_DEFAULT);
408 +  }
409 +
410 +  module_load_include('inc', 'uuid', 'uuid.class');
411 +  
412 +  switch($uuid_version) {
413 +  	case 'v3':
414 +  	case 'v5':
415 +  		//in the off-chance there is no namespace set, fallback
416 +  		//to v4 namespace
417 +  		$namespace = variable_get('uuid_namespace', uuid::v4());
418 +  		if (empty($prefix)) {
419 +  			$prefix = @php_uname('n');
420 +  		}
421 +  		$uuid = uuid::$uuid_version($namespace, uniqid($prefix, TRUE));
422 +  		break;
423 +  	case 'v4':
424 +  		$uuid = uuid::v4();
425 +  		break;
426 +  	default:
427 +  		$uuid = uuid_uuid(UUID_VERSION_DEFAULT);
428 +  		break;
429 +  }
430 +  
431 +  return $uuid;
432  }

Again, sorry this got so long, but I think you'll see how all these modifications are chained together and dependent on each other.

Take what you like. Hopefully you will find something useful in here. I

If you'd rather I gen the patch against the dev release rather than beta2, I can do that, or I could run it against latest in CVS.

Class for generating uuids in 3 versions with pure PHP. from php.net, but with no attribution of origin.

Woops, better coding standards for the class. Passes coder now.