[folks] Bug 652660 — Make Individual.id more stable and well-defined

[Philip Withnall <pwithnall src gnome org>]

commit 6ffb9775808386369c9a1fd45a072f404d410330
Author: Philip Withnall <philip tecnocode co uk>
Date:   Wed Jun 15 18:44:18 2011 +0100

    Bug 652660 â Make Individual.id more stable and well-defined

 NEWS                                       |    1 +
 folks/individual.vala                      |   60 +++++++++++++++++++++++---
 tests/folks/aggregation.vala               |   64 +++++++++++++++++++---------
 tests/telepathy/individual-properties.vala |   17 ++++----
 tests/telepathy/individual-retrieval.vala  |   37 +++++++++++-----
 5 files changed, 132 insertions(+), 47 deletions(-)
---
diff --git a/NEWS b/NEWS
index e792986..29253fb 100644
--- a/NEWS
+++ b/NEWS
@@ -9,6 +9,7 @@ Bugs fixed:
 * Bug 648071 â Add support for presence status from Telepathy
 * Bug 652472 â Handle failure in getting the logger service better
 * Bug 629716 â Migrate Folks to GDBus
+* Bug 652660 â Make Individual.id more stable and well-defined
 
 API changes:
 * Swf.Persona retains and exposes its libsocialweb Contact
diff --git a/folks/individual.vala b/folks/individual.vala
index e6f998d..c33b78d 100644
--- a/folks/individual.vala
+++ b/folks/individual.vala
@@ -163,9 +163,17 @@ public class Folks.Individual : Object,
    * A unique identifier for the Individual.
    *
    * This uniquely identifies the Individual, and persists across
-   * { link IndividualAggregator} instances.
+   * { link IndividualAggregator} instances. It may not persist across linking
+   * the Individual with other Individuals.
    *
-   * FIXME: Will this.id actually be the persistent ID for storage?
+   * This is an opaque string and has no structure.
+   *
+   * If an identifier is required which will be used for a long-lived link
+   * between different stored data, it may be more desirable to use the
+   * { link Persona.uid} of the most relevant { link Persona} in the Individual
+   * instead. For example, if storing references to Individuals who are tagged
+   * in a photo, it may be safer to store the UID of the Persona whose backend
+   * provided the photo (e.g. Facebook).
    */
   public string id { get; private set; }
 
@@ -1517,14 +1525,54 @@ public class Folks.Individual : Object,
           return;
         }
 
-      /* TODO: Base this upon our ID in permanent storage, once we have that. */
-      if (this.id == null && this._persona_set.size > 0)
+      /* Update the ID. We choose the most interesting Persona in the
+       * Individual and hash their UID. This is guaranteed to be globally
+       * unique, and may not change (for one of the two Individuals) if we link
+       * two Individuals together, which is nice though we can't rely on this
+       * behaviour.
+       *
+       * This method of constructing an ID ensures that it'll be unique and
+       * stable for a given Individual once the IndividualAggregator reaches
+       * a quiescent state after startup. It guarantees that the ID will be
+       * the same every time folks is used, until the Individual is linked
+       * or unlinked to another Individual.
+       *
+       * We choose the most interesting Persona by ranking all the Personas
+       * in the Individual by:
+       *  1. store.is-writeable
+       *  2. store.trust-level
+       *  3. store.id (alphabetically)
+       *
+       * Note that this heuristic shouldn't be changed without careful thought,
+       * since stored references to IDs may be broken by the change.
+       */
+      if (this._persona_set.size > 0)
         {
+          Persona? chosen_persona = null;
+
           foreach (var persona in this._persona_set)
             {
-              this.id = persona.uid;
-              break;
+              if (chosen_persona == null ||
+                  (chosen_persona.store.is_writeable == false &&
+                      persona.store.is_writeable == true) ||
+                  (chosen_persona.store.is_writeable ==
+                          persona.store.is_writeable &&
+                      chosen_persona.store.trust_level >
+                          persona.store.trust_level) ||
+                  (chosen_persona.store.is_writeable ==
+                          persona.store.is_writeable &&
+                      chosen_persona.store.trust_level ==
+                          persona.store.trust_level &&
+                      chosen_persona.store.id > persona.store.id)
+                 )
+               {
+                 chosen_persona = persona;
+               }
             }
+
+          // Hash the chosen persona's UID
+          this.id = Checksum.compute_for_string (ChecksumType.SHA1,
+              chosen_persona.uid);
         }
 
       /* Update our aggregated fields and notify the changes */
diff --git a/tests/folks/aggregation.vala b/tests/folks/aggregation.vala
index 21eddd6..a79e911 100644
--- a/tests/folks/aggregation.vala
+++ b/tests/folks/aggregation.vala
@@ -6,7 +6,7 @@ public class AggregationTests : Folks.TestCase
 {
   private KfTest.Backend _kf_backend;
   private TpTest.Backend _tp_backend;
-  private HashSet<string> _default_individuals;
+  private HashSet<string> _default_personas;
   private int _test_timeout = 3;
 
   public AggregationTests ()
@@ -17,16 +17,16 @@ public class AggregationTests : Folks.TestCase
       this._tp_backend = new TpTest.Backend ();
 
       /* Create a set of the individuals we expect to see */
-      this._default_individuals = new HashSet<string> (str_hash, str_equal);
+      this._default_personas = new HashSet<string> (str_hash, str_equal);
 
-      this._default_individuals.add ("travis example com");
-      this._default_individuals.add ("olivier example com");
-      this._default_individuals.add ("guillaume example com");
-      this._default_individuals.add ("sjoerd example com");
-      this._default_individuals.add ("christian example com");
-      this._default_individuals.add ("wim example com");
-      this._default_individuals.add ("helen example com");
-      this._default_individuals.add ("geraldine example com");
+      this._default_personas.add ("travis example com");
+      this._default_personas.add ("olivier example com");
+      this._default_personas.add ("guillaume example com");
+      this._default_personas.add ("sjoerd example com");
+      this._default_personas.add ("christian example com");
+      this._default_personas.add ("wim example com");
+      this._default_personas.add ("helen example com");
+      this._default_personas.add ("geraldine example com");
 
       /* Set up the tests */
       this.add_test ("IID", this.test_iid);
@@ -69,9 +69,37 @@ public class AggregationTests : Folks.TestCase
       void* account2_handle = this._tp_backend.add_account ("protocol",
           "me2 example com", "cm", "account2");
 
+      /* IDs of the individuals we expect to see.
+       * These are externally opaque, but internally are SHA-1 hashes of the
+       * concatenated UIDs of the Personas in the Individual. In these cases,
+       * each default_individual contains two Personas with the same IID.
+       * e.g.
+       *  telepathy:/org/freedesktop/Telepathy/Account/cm/protocol/account2:sjoerd example com
+       * and
+       *  telepathy:/org/freedesktop/Telepathy/Account/cm/protocol/account:sjoerd example com
+       * in a single Individual. */
+      var default_individuals = new HashSet<string> ();
+
+      /* guillaume example com */
+      default_individuals.add ("6380b17dc511b21a1defd4811f1add97b278f92c");
+      /* sjoerd example com */
+      default_individuals.add ("6b08188cb2ef8cbaca140b277780069b5af8add6");
+      /* travis example com */
+      default_individuals.add ("60c91326018f6a60604f8d260fc24a60a5b8512c");
+      /* olivier example com */
+      default_individuals.add ("0e46c5e74f61908f49550d241f2a1651892a1695");
+      /* christian example com */
+      default_individuals.add ("07b913b8977c04d2f2011e26a46ea3e3dcfe3e3d");
+      /* geraldine example com */
+      default_individuals.add ("f948d4d2af79085ab860f0ef67bf0c201c4602d4");
+      /* helen example com */
+      default_individuals.add ("f34529a442577b840a75271b464e90666c38c464");
+      /* wim example com */
+      default_individuals.add ("467d13f955e62bf30ebf9620fa052aaee2160260");
+
       /* Work on a copy of the set of individuals so we can mangle it */
       HashSet<string> expected_individuals = new HashSet<string> ();
-      foreach (var id in this._default_individuals)
+      foreach (var id in default_individuals)
         {
           expected_individuals.add (id);
         }
@@ -84,14 +112,11 @@ public class AggregationTests : Folks.TestCase
            * individuals (if they were originally on it) */
           foreach (Individual i in removed)
             {
-              var parts = i.id.split (":");
-              var id = parts[2];
-
               if (!i.is_user &&
                   i.personas.size == 2 &&
-                  this._default_individuals.contains (id))
+                  default_individuals.contains (i.id))
                 {
-                  expected_individuals.add (id);
+                  expected_individuals.add (i.id);
                 }
             }
 
@@ -99,15 +124,12 @@ public class AggregationTests : Folks.TestCase
            * from the set of expected individuals. */
           foreach (Individual i in added)
             {
-              var parts = i.id.split (":");
-              var id = parts[2];
-
               var personas = i.personas;
 
               /* We're not testing the user here */
               if (!i.is_user && personas.size == 2)
                 {
-                  assert (expected_individuals.remove (id));
+                  assert (expected_individuals.remove (i.id));
 
                   string iid = null;
                   foreach (var persona in personas)
@@ -441,7 +463,7 @@ public class AggregationTests : Folks.TestCase
        * from a different persona store. */
       var expected_personas1 = new HashSet<string> ();
       var expected_personas2 = new HashSet<string> ();
-      foreach (var id in this._default_individuals)
+      foreach (var id in this._default_personas)
         {
           expected_personas1.add (id);
           expected_personas2.add (id);
diff --git a/tests/telepathy/individual-properties.vala b/tests/telepathy/individual-properties.vala
index ccb6fa2..7edea22 100644
--- a/tests/telepathy/individual-properties.vala
+++ b/tests/telepathy/individual-properties.vala
@@ -9,8 +9,6 @@ public class IndividualPropertiesTests : Folks.TestCase
 {
   private TpTest.Backend tp_backend;
   private void* _account_handle;
-  private string individual_id_prefix =
-      "telepathy:/org/freedesktop/Telepathy/Account/cm/protocol/account:";
   private int _test_timeout = 3;
 
   public IndividualPropertiesTests ()
@@ -53,8 +51,9 @@ public class IndividualPropertiesTests : Folks.TestCase
         {
           foreach (Individual i in added)
             {
-              /* We only check one */
-              if (i.id != (this.individual_id_prefix + "olivier example com"))
+              /* We only check one (singleton Individual containing just
+               * olivier example com) */
+              if (i.id != "0e46c5e74f61908f49550d241f2a1651892a1695")
                 {
                   continue;
                 }
@@ -104,8 +103,9 @@ public class IndividualPropertiesTests : Folks.TestCase
 
           foreach (Individual i in added)
             {
-              /* We only check one */
-              if (i.id != (this.individual_id_prefix + "olivier example com"))
+              /* We only check one (singleton Individual containing just
+               * olivier example com) */
+              if (i.id != "0e46c5e74f61908f49550d241f2a1651892a1695")
                 {
                   continue;
                 }
@@ -172,8 +172,9 @@ public class IndividualPropertiesTests : Folks.TestCase
 
           foreach (Individual i in added)
             {
-              /* We only check one */
-              if (i.id != (this.individual_id_prefix + "olivier example com"))
+              /* We only check one (singleton Individual containing just
+               * olivier example com) */
+              if (i.id != "0e46c5e74f61908f49550d241f2a1651892a1695")
                 {
                   continue;
                 }
diff --git a/tests/telepathy/individual-retrieval.vala b/tests/telepathy/individual-retrieval.vala
index 3c01685..612e3fa 100644
--- a/tests/telepathy/individual-retrieval.vala
+++ b/tests/telepathy/individual-retrieval.vala
@@ -10,8 +10,6 @@ public class IndividualRetrievalTests : Folks.TestCase
   private TpTest.Backend tp_backend;
   private void* _account_handle;
   private HashSet<string> default_individuals;
-  private string individual_id_prefix =
-      "telepathy:/org/freedesktop/Telepathy/Account/cm/protocol/account:";
   private int _test_timeout = 3;
 
   public IndividualRetrievalTests ()
@@ -20,18 +18,33 @@ public class IndividualRetrievalTests : Folks.TestCase
 
       this.tp_backend = new TpTest.Backend ();
 
-      /* Create a set of the individuals we expect to see */
+      /* IDs of the individuals we expect to see.
+       * These are externally opaque, but internally are SHA-1 hashes of the
+       * concatenated UIDs of the Personas in the Individual. In these cases,
+       * each default_individual contains one Persona.
+       * e.g.
+       *  telepathy:/org/freedesktop/Telepathy/Account/cm/protocol/account:me example com
+       * only in each Individual. */
       this.default_individuals = new HashSet<string> (str_hash, str_equal);
 
-      var prefix = this.individual_id_prefix;
-      default_individuals.add (prefix + "travis example com");
-      default_individuals.add (prefix + "olivier example com");
-      default_individuals.add (prefix + "guillaume example com");
-      default_individuals.add (prefix + "sjoerd example com");
-      default_individuals.add (prefix + "christian example com");
-      default_individuals.add (prefix + "wim example com");
-      default_individuals.add (prefix + "helen example com");
-      default_individuals.add (prefix + "geraldine example com");
+      /* me example com */
+      default_individuals.add ("48fa372a81026063187255e3f5c184665d2ed7ce");
+      /* travis example com */
+      default_individuals.add ("60c91326018f6a60604f8d260fc24a60a5b8512c");
+      /* guillaume example com */
+      default_individuals.add ("6380b17dc511b21a1defd4811f1add97b278f92c");
+      /* olivier example com */
+      default_individuals.add ("0e46c5e74f61908f49550d241f2a1651892a1695");
+      /* sjoerd example com */
+      default_individuals.add ("6b08188cb2ef8cbaca140b277780069b5af8add6");
+      /* geraldine example com */
+      default_individuals.add ("f948d4d2af79085ab860f0ef67bf0c201c4602d4");
+      /* helen example com */
+      default_individuals.add ("f34529a442577b840a75271b464e90666c38c464");
+      /* wim example com */
+      default_individuals.add ("467d13f955e62bf30ebf9620fa052aaee2160260");
+      /* christian example com */
+      default_individuals.add ("07b913b8977c04d2f2011e26a46ea3e3dcfe3e3d");
 
       this.add_test ("aggregator", this.test_aggregator);
       this.add_test ("aggregator:add", this.test_aggregator_add);