Skip/remove dangling implicit associations

4 files changed, 155 insertions, 48 deletions

diff --git a/bpkg/database.cxx b/bpkg/database.cxx
index 45f4b29..1b7612f 100644
--- a/bpkg/database.cxx
+++ b/bpkg/database.cxx
@@ -339,7 +339,22 @@ namespace bpkg
       schema_catalog::migrate (*this);
 
       for (auto& c: query<configuration> (odb::query<configuration>::id != 0))
-        attach (c.make_effective_path (config)).migrate ();
+      {
+        dir_path d (c.effective_path (config));
+
+        // Remove the dangling implicit association.
+        //
+        if (!c.expl && !exists (d))
+        {
+          warn << "implicit association " << c.path << " of configuration "
+               << config_orig << " does not exist, removing";
+
+          erase (c);
+          continue;
+        }
+
+        attach (d).migrate ();
+      }
     }
   }
 
@@ -516,54 +531,56 @@ namespace bpkg
     //
     if (implicit_associations_.empty () && ath)
     {
+      implicit_associations_.push_back (*this);
+
       using q = odb::query<configuration>;
 
-      // Make sure the self-association (zero id) comes first.
-      //
-      for (const auto& ac: query<configuration> ("ORDER BY" + q::id))
+      for (const auto& ac: query<configuration> (q::id != 0))
       {
-        database& db (attach (ac.effective_path (config), sys_rep));
+        dir_path d (ac.effective_path (config));
 
-        // Verify the association integrity and pre-attach its explicit
-        // associations, if required.
+        // Skip the dangling implicit association.
         //
-        if (*ac.id != 0)
+        if (!ac.expl && !exists (d))
+          continue;
+
+        database& db (attach (d, sys_rep));
+
+        // Verify the association integrity.
+        //
+        verify_association (ac, db);
+
+        // If the association is explicit, also check if it is also implicit
+        // (see cfg_add() for details) and skip if it is not.
+        //
+        if (ac.expl)
         {
-          verify_association (ac, db);
+          shared_ptr<configuration> cf (
+            db.query_one<configuration> (q::uuid == uuid.string ()));
 
-          // If the association is explicit, also check if it is also implicit
-          // (see cfg_add() for details) and skip if it is not.
-          //
-          if (ac.expl)
-          {
-            shared_ptr<configuration> cf (
-              db.query_one<configuration> (q::uuid == uuid.string ()));
-
-            if (cf == nullptr)
-              fail << "configuration " << db.config_orig << " is associated "
-                   << "with " << config_orig << " but latter is not "
-                   << "implicitly associated with former";
-
-            // While at it, verify the integrity of the other end of the
-            // association.
-            //
-            db.verify_association (*cf, *this);
-
-            if (!cf->expl)
-              continue;
-          }
-
-          // If the explicitly associated databases are pre-attached, normally
-          // to make the selected packages loadable, then we also pre-attach
-          // explicit associations of the database being attached implicitly,
-          // by the same reason. Indeed, think of loading the package
-          // dependent from the implicitly associated database as a selected
-          // package.
+          if (cf == nullptr)
+            fail << "configuration " << db.config_orig << " is associated "
+                 << "with " << config_orig << " but latter is not "
+                 << "implicitly associated with former";
+
+          // While at it, verify the integrity of the other end of the
+          // association.
           //
-          if (!explicit_associations_.empty ())
-            db.attach_explicit (sys_rep);
+          db.verify_association (*cf, *this);
+
+          if (!cf->expl)
+            continue;
         }
 
+        // If the explicitly associated databases are pre-attached, normally
+        // to make the selected packages loadable, then we also pre-attach
+        // explicit associations of the database being attached implicitly, by
+        // the same reason. Indeed, think of loading the package dependent
+        // from the implicitly associated database as a selected package.
+        //
+        if (!explicit_associations_.empty ())
+          db.attach_explicit (sys_rep);
+
         implicit_associations_.push_back (db);
       }
     }
diff --git a/bpkg/database.hxx b/bpkg/database.hxx
index 53ca54a..735fe7f 100644
--- a/bpkg/database.hxx
+++ b/bpkg/database.hxx
@@ -143,11 +143,18 @@ namespace bpkg
 
     // By default attach and cache the implicitly associated configuration
     // databases on the first call and return them along with the self-
-    // association (comes first). If attach is false, then return an empty
-    // list if associations were not yet cached by this function's previous
-    // call.
-    //
-    // Note that for implicitly associated configurations the association
+    // association (comes first), silently skipping the dangling
+    // associations. If attach is false, then return an empty list if
+    // associations were not yet cached by this function's previous call.
+    //
+    // Note that we skip dangling associations without any warning since they
+    // can be quite common. Think of a shared host configuration with a bunch
+    // of implicitly associated configurations, which are removed and
+    // potentially recreated later during the host configuration lifetime.
+    // Note however, that we remove the dangling implicit associations during
+    // migration (see migrate() on details).
+    //
+    // Also note that for implicitly associated configurations the association
     // information (id, etc) is useless, thus we only return the databases
     // rather than the association information.
     //
@@ -311,7 +318,10 @@ namespace bpkg
     //
     // Note that since the whole associated databases cluster is migrated at
     // once, it is assumed that if migration is unnecessary for this database
-    // then it is also unnecessary for its associated databases.
+    // then it is also unnecessary for its associated databases. By this
+    // reason, we also drop the dangling implicit associations rather than
+    // skip them, as we do for normal operations (see implicit_associations ()
+    // for details).
     //
     void
     migrate ();
diff --git a/bpkg/package.cxx b/bpkg/package.cxx
index b1e9ab1..abfcd55 100644
--- a/bpkg/package.cxx
+++ b/bpkg/package.cxx
@@ -497,12 +497,56 @@ namespace bpkg
   lazy_shared_ptr<selected_package> _selected_package_ref::
   to_ptr (odb::database& db) &&
   {
+    database& pdb (static_cast<database&> (db));
+
     // Note that if this points to a different configuration, then it should
     // already be pre-attached since it must be explicitly associated.
     //
-    return lazy_shared_ptr<selected_package> (
-      static_cast<database&> (db).find_dependency_config (configuration),
-      move (prerequisite));
+    database& ddb (pdb.find_dependency_config (configuration));
+
+    // Make sure the prerequisite exists in the explicitly associated
+    // configuration, so that a subsequent load() call will not fail. This,
+    // for example, can happen in unlikely but possible situation when the
+    // implicitly associated configuration containing a dependent was
+    // temporarily renamed before its prerequisite was dropped.
+    //
+    // Note that the diagnostics lacks information about the dependent and its
+    // configuration. However, handling this situation at all the load()
+    // function call sites where this information is available, for example by
+    // catching the odb::object_not_persistent exception, feels a bit
+    // hairy. Given the situation is not common, let's keep it simple for now
+    // and see how it goes.
+    //
+    // @@ As a side note, the following code crashes. Is this a libodb bug or
+    //    just lack of my understanding?
+    //
+#if 0
+    if (ddb != pdb)
+    {
+      shared_ptr<selected_package> p (
+        ddb.find<selected_package> (prerequisite));
+
+      if (p != nullptr)
+      {
+        lazy_shared_ptr<selected_package> lp (ddb, move (p));
+        lp.database ().string (); // Ok.
+
+        lazy_shared_ptr<selected_package> lp2 (move (lp));
+        lp2.database ().string (); // Ok.
+
+        lazy_shared_ptr<selected_package> lp3;
+
+        lp3 = move (lp2); // lp2.i_.db_ is not copied (odb/lazy-ptr-impl.ixx:83)
+        lp3.database ().string (); // Crashes since lp3.i_.db_ is NULL.
+      }
+    }
+#endif
+
+    if (ddb != pdb && ddb.find<selected_package> (prerequisite) == nullptr)
+      fail << "unable to find prerequisite package " << prerequisite
+           << " in associated configuration " << ddb.config_orig;
+
+    return lazy_shared_ptr<selected_package> (ddb, move (prerequisite));
   }
 
   pair<shared_ptr<selected_package>, database*>
diff --git a/tests/pkg-drop.testscript b/tests/pkg-drop.testscript
index ff42e58..a9e8f4f 100644
--- a/tests/pkg-drop.testscript
+++ b/tests/pkg-drop.testscript
@@ -693,4 +693,40 @@ $* libfoo/1.0.0 2>>~%EOE% != 0
 
     $pkg_status -r libbar >'libbar available 1.0.0'
   }
+
+  : skip-deleted-dependency
+  :
+  {
+    $clone_cfg;
+    cp -pr ../cfg2 ./;
+
+    $pkg_build libbar --yes >! &cfg/lib*/*** &cfg/lib*;
+
+    mv cfg cfg.tmp;
+
+    $* -d cfg2 libbaz <<EOI 2>>/~%EOE%;
+      y
+      y
+      EOI
+      following dependent packages will have to be dropped as well:
+        foo (requires libbaz)
+      %drop dependent packages\? \[y.N\]   drop foo%
+        drop libbaz
+      %continue\? \[Y.n\] disfigured foo%
+      disfigured libbaz
+      purged foo
+      purged libbaz
+      EOE
+
+    # While at it, test that we properly handle the missing prerequisite
+    # situation.
+    #
+    mv cfg.tmp cfg;
+
+    $* libbar 2>>/EOE != 0;
+      error: unable to find prerequisite package foo in associated configuration cfg2/
+      EOE
+
+    $pkg_status -d cfg2 -r 2>'info: no held packages in the configuration'
+  }
 }