From cba07157d5589e485fc2376336cc18c93c8cdb40 Mon Sep 17 00:00:00 2001 From: James Turner Date: Wed, 28 Feb 2018 09:52:26 +0000 Subject: [PATCH] Packages: better version handling & migration Add test coverage for disabling a catalog due to version, and also for auto-migrating to a new version. Expose disabled catalogs on the Root, so they can be checked for. --- simgear/package/Catalog.cxx | 29 ++- simgear/package/Catalog.hxx | 6 + simgear/package/CatalogTest.cxx | 191 +++++++++++++++++++ simgear/package/Root.cxx | 68 ++++--- simgear/package/Root.hxx | 8 +- simgear/package/catalogTest1/catalog-v10.xml | 178 +++++++++++++++++ simgear/package/catalogTest1/catalog-v2.xml | 5 + simgear/package/catalogTest1/catalog-v9.xml | 178 +++++++++++++++++ 8 files changed, 623 insertions(+), 40 deletions(-) create mode 100644 simgear/package/catalogTest1/catalog-v10.xml create mode 100644 simgear/package/catalogTest1/catalog-v9.xml diff --git a/simgear/package/Catalog.cxx b/simgear/package/Catalog.cxx index 1fe5746c..21d25bb5 100644 --- a/simgear/package/Catalog.cxx +++ b/simgear/package/Catalog.cxx @@ -84,7 +84,7 @@ bool checkVersion(const std::string& aVersion, SGPropertyNode_ptr props) std::string redirectUrlForVersion(const std::string& aVersion, SGPropertyNode_ptr props) { - BOOST_FOREACH(SGPropertyNode* v, props->getChildren("alternate-version")) { + for (SGPropertyNode* v : props->getChildren("alternate-version")) { std::string s(v->getStringValue("version")); if (checkVersionString(aVersion, s)) { return v->getStringValue("url");; @@ -139,7 +139,7 @@ protected: std::string ver(m_owner->root()->applicationVersion()); if (!checkVersion(ver, props)) { - SG_LOG(SG_GENERAL, SG_WARN, "downloaded catalog " << m_owner->url() << ", version required " << ver); + SG_LOG(SG_GENERAL, SG_WARN, "downloaded catalog " << m_owner->url() << ", but version required " << ver); // check for a version redirect entry std::string url = redirectUrlForVersion(ver, props); @@ -169,7 +169,13 @@ protected: m_owner->writeTimestamp(); m_owner->refreshComplete(Delegate::STATUS_REFRESHED); } - + + void onFail() override + { + // network level failure + SG_LOG(SG_GENERAL, SG_WARN, "catalog network failure for:" << m_owner->url()); + m_owner->refreshComplete(Delegate::FAIL_DOWNLOAD); + } private: CatalogRef m_owner; @@ -215,7 +221,8 @@ CatalogRef Catalog::createFromPath(Root* aRoot, const SGPath& aPath) bool versionCheckOk = checkVersion(aRoot->applicationVersion(), props); if (!versionCheckOk) { - SG_LOG(SG_GENERAL, SG_INFO, "catalog at:" << aPath << " failed version check: need" << aRoot->applicationVersion()); + SG_LOG(SG_GENERAL, SG_INFO, "catalog at:" << aPath << " failed version check: need version: " + << aRoot->applicationVersion()); // keep the catalog but mark it as needing an update } else { SG_LOG(SG_GENERAL, SG_DEBUG, "creating catalog from:" << aPath); @@ -540,6 +547,20 @@ Delegate::StatusCode Catalog::status() const return m_status; } +bool Catalog::isEnabled() const +{ + switch (m_status) { + case Delegate::STATUS_SUCCESS: + case Delegate::STATUS_REFRESHED: + case Delegate::STATUS_IN_PROGRESS: + // this is important so we can use Catalog aircraft in offline mode + case Delegate::FAIL_DOWNLOAD: + return true; + default: + return false; + } +} + } // of namespace pkg } // of namespace simgear diff --git a/simgear/package/Catalog.hxx b/simgear/package/Catalog.hxx index 8c6e2efc..513d5b43 100644 --- a/simgear/package/Catalog.hxx +++ b/simgear/package/Catalog.hxx @@ -134,6 +134,12 @@ public: SGPropertyNode* properties() const; Delegate::StatusCode status() const; + + /** + * is this Catalog usable? This may be false if the catalog is currently + * failing a version check or cannot be updated + */ + bool isEnabled() const; typedef boost::function Callback; diff --git a/simgear/package/CatalogTest.cxx b/simgear/package/CatalogTest.cxx index fd90ab86..4e3e8e1b 100644 --- a/simgear/package/CatalogTest.cxx +++ b/simgear/package/CatalogTest.cxx @@ -61,6 +61,7 @@ std::string readFileIntoString(const SGPath& path) SGPath global_serverFilesRoot; unsigned int global_catalogVersion = 0; +bool global_failRequests = false; class TestPackageChannel : public TestServerChannel { @@ -71,6 +72,10 @@ public: state = STATE_IDLE; SGPath localPath(global_serverFilesRoot); + if (global_failRequests) { + closeWhenDone(); + return; + } if (path == "/catalogTest1/catalog.xml") { if (global_catalogVersion > 0) { @@ -540,6 +545,186 @@ void testInstallTarPackage(HTTP::Client* cl) SG_VERIFY(p.exists()); } +void testDisableDueToVersion(HTTP::Client* cl) +{ + global_catalogVersion = 0; + SGPath rootPath(simgear::Dir::current().path()); + rootPath.append("cat_disable_at_version"); + simgear::Dir pd(rootPath); + pd.removeChildren(); + + { + pkg::RootRef root(new pkg::Root(rootPath, "8.1.2")); + root->setHTTPClient(cl); + + pkg::CatalogRef c = pkg::Catalog::createFromUrl(root.ptr(), "http://localhost:2000/catalogTest1/catalog.xml"); + waitForUpdateComplete(cl, root); + SG_VERIFY(c->isEnabled()); + + // install a package + pkg::PackageRef p1 = root->getPackageById("org.flightgear.test.catalog1.b737-NG"); + SG_CHECK_EQUAL(p1->id(), "b737-NG"); + pkg::InstallRef ins = p1->install(); + SG_VERIFY(ins->isQueued()); + waitForUpdateComplete(cl, root); + SG_VERIFY(p1->isInstalled()); + } + + // bump version and refresh + { + pkg::RootRef root(new pkg::Root(rootPath, "9.1.2")); + pkg::CatalogRef cat = root->getCatalogById("org.flightgear.test.catalog1"); + SG_CHECK_EQUAL(root->allCatalogs().size(), 1); + SG_VERIFY(!cat->isEnabled()); + SG_CHECK_EQUAL(root->catalogs().size(), 0); + + root->setHTTPClient(cl); + root->refresh(); + waitForUpdateComplete(cl, root); + SG_CHECK_EQUAL(root->allCatalogs().size(), 1); + + + SG_CHECK_EQUAL(cat->status(), pkg::Delegate::FAIL_VERSION); + SG_VERIFY(!cat->isEnabled()); + SG_CHECK_EQUAL(cat->id(), "org.flightgear.test.catalog1"); + + auto enabledCats = root->catalogs(); + auto it = std::find(enabledCats.begin(), enabledCats.end(), cat); + SG_VERIFY(it == enabledCats.end()); + SG_CHECK_EQUAL(enabledCats.size(), 0); + + auto allCats = root->allCatalogs(); + auto j = std::find(allCats.begin(), allCats.end(), cat); + SG_VERIFY(j != allCats.end()); + + SG_CHECK_EQUAL(allCats.size(), 1); + + // ensure existing package is still installed but not directly list + + pkg::PackageRef p1 = root->getPackageById("org.flightgear.test.catalog1.b737-NG"); + SG_VERIFY(p1 != pkg::PackageRef()); + SG_CHECK_EQUAL(p1->id(), "b737-NG"); + + auto packs = root->allPackages(); + auto k = std::find(packs.begin(), packs.end(), p1); + SG_VERIFY(k == packs.end()); + } +} + +void testVersionMigrate(HTTP::Client* cl) +{ + global_catalogVersion = 2; // version which has migration info + SGPath rootPath(simgear::Dir::current().path()); + rootPath.append("cat_migrate_version"); + simgear::Dir pd(rootPath); + pd.removeChildren(); + + { + pkg::RootRef root(new pkg::Root(rootPath, "8.1.2")); + root->setHTTPClient(cl); + + pkg::CatalogRef c = pkg::Catalog::createFromUrl(root.ptr(), "http://localhost:2000/catalogTest1/catalog.xml"); + waitForUpdateComplete(cl, root); + SG_VERIFY(c->isEnabled()); + + // install a package + pkg::PackageRef p1 = root->getPackageById("org.flightgear.test.catalog1.b737-NG"); + SG_CHECK_EQUAL(p1->id(), "b737-NG"); + pkg::InstallRef ins = p1->install(); + SG_VERIFY(ins->isQueued()); + waitForUpdateComplete(cl, root); + SG_VERIFY(p1->isInstalled()); + } + + // bump version and refresh + { + pkg::RootRef root(new pkg::Root(rootPath, "10.1.2")); + root->setHTTPClient(cl); + + // this should cause auto-migration + root->refresh(true); + waitForUpdateComplete(cl, root); + + pkg::CatalogRef cat = root->getCatalogById("org.flightgear.test.catalog1"); + SG_VERIFY(cat->isEnabled()); + SG_CHECK_EQUAL(cat->status(), pkg::Delegate::STATUS_REFRESHED); + SG_CHECK_EQUAL(cat->id(), "org.flightgear.test.catalog1"); + SG_CHECK_EQUAL(cat->url(), "http://localhost:2000/catalogTest1/catalog-v10.xml"); + + auto enabledCats = root->catalogs(); + auto it = std::find(enabledCats.begin(), enabledCats.end(), cat); + SG_VERIFY(it != enabledCats.end()); + + // ensure existing package is still installed + + pkg::PackageRef p1 = root->getPackageById("org.flightgear.test.catalog1.b737-NG"); + SG_VERIFY(p1 != pkg::PackageRef()); + SG_CHECK_EQUAL(p1->id(), "b737-NG"); + + auto packs = root->allPackages(); + auto k = std::find(packs.begin(), packs.end(), p1); + SG_VERIFY(k != packs.end()); + } +} + +void testOfflineMode(HTTP::Client* cl) +{ + global_catalogVersion = 0; + SGPath rootPath(simgear::Dir::current().path()); + rootPath.append("cat_offline_mode"); + simgear::Dir pd(rootPath); + pd.removeChildren(); + + { + pkg::RootRef root(new pkg::Root(rootPath, "8.1.2")); + root->setHTTPClient(cl); + + pkg::CatalogRef c = pkg::Catalog::createFromUrl(root.ptr(), "http://localhost:2000/catalogTest1/catalog.xml"); + waitForUpdateComplete(cl, root); + SG_VERIFY(c->isEnabled()); + + // install a package + pkg::PackageRef p1 = root->getPackageById("org.flightgear.test.catalog1.b737-NG"); + SG_CHECK_EQUAL(p1->id(), "b737-NG"); + pkg::InstallRef ins = p1->install(); + SG_VERIFY(ins->isQueued()); + waitForUpdateComplete(cl, root); + SG_VERIFY(p1->isInstalled()); + } + + global_failRequests = true; + + { + pkg::RootRef root(new pkg::Root(rootPath, "8.1.2")); + SG_CHECK_EQUAL(root->catalogs().size(), 1); + + root->setHTTPClient(cl); + root->refresh(true); + waitForUpdateComplete(cl, root); + SG_CHECK_EQUAL(root->catalogs().size(), 1); + + pkg::CatalogRef cat = root->getCatalogById("org.flightgear.test.catalog1"); + SG_VERIFY(cat->isEnabled()); + SG_CHECK_EQUAL(cat->status(), pkg::Delegate::FAIL_DOWNLOAD); + SG_CHECK_EQUAL(cat->id(), "org.flightgear.test.catalog1"); + + auto enabledCats = root->catalogs(); + auto it = std::find(enabledCats.begin(), enabledCats.end(), cat); + SG_VERIFY(it != enabledCats.end()); + + // ensure existing package is still installed + pkg::PackageRef p1 = root->getPackageById("org.flightgear.test.catalog1.b737-NG"); + SG_VERIFY(p1 != pkg::PackageRef()); + SG_CHECK_EQUAL(p1->id(), "b737-NG"); + + auto packs = root->allPackages(); + auto k = std::find(packs.begin(), packs.end(), p1); + SG_VERIFY(k != packs.end()); + } + + global_failRequests = false; +} + int main(int argc, char* argv[]) { @@ -564,6 +749,12 @@ int main(int argc, char* argv[]) testInstallTarPackage(&cl); + testDisableDueToVersion(&cl); + + testOfflineMode(&cl); + + testVersionMigrate(&cl); + std::cout << "Successfully passed all tests!" << std::endl; return EXIT_SUCCESS; } diff --git a/simgear/package/Root.cxx b/simgear/package/Root.cxx index c8eac52a..e4c895a8 100644 --- a/simgear/package/Root.cxx +++ b/simgear/package/Root.cxx @@ -408,15 +408,11 @@ Root::Root(const SGPath& aPath, const std::string& aVersion) : } for (SGPath c : dir.children(Dir::TYPE_DIR | Dir::NO_DOT_OR_DOTDOT)) { - CatalogRef cat = Catalog::createFromPath(this, c); - if (cat) { - if (cat->status() == Delegate::STATUS_SUCCESS) { - d->catalogs[cat->id()] = cat; - } else { - // catalog has problems, such as needing an update - // keep it out of the main collection for now - d->disabledCatalogs.push_back(cat); - } + // note this will set the catalog status, which will insert into + // disabled catalogs automatically if necesary + auto cat = Catalog::createFromPath(this, c); + if (cat && cat->isEnabled()) { + d->catalogs.insert({cat->id(), cat}); } } // of child directories iteration } @@ -438,9 +434,15 @@ std::string Root::applicationVersion() const CatalogRef Root::getCatalogById(const std::string& aId) const { - CatalogDict::const_iterator it = d->catalogs.find(aId); + auto it = d->catalogs.find(aId); if (it == d->catalogs.end()) { - return NULL; + // check disabled catalog list + auto j = std::find_if(d->disabledCatalogs.begin(), d->disabledCatalogs.end(), + [aId](const CatalogRef& cat) { return cat->id() == aId; }); + if (j != d->disabledCatalogs.end()) { + return *j; + } + return nullptr; } return it->second; @@ -484,6 +486,13 @@ CatalogList Root::catalogs() const return r; } + +CatalogList Root::allCatalogs() const +{ + CatalogList r = catalogs(); + r.insert(r.end(), d->disabledCatalogs.begin(), d->disabledCatalogs.end()); + return r; +} PackageList Root::allPackages() const @@ -544,12 +553,9 @@ void Root::refresh(bool aForce) toRefresh.insert(toRefresh.end(), d->disabledCatalogs.begin(), d->disabledCatalogs.end()); - - - CatalogList::iterator j = toRefresh.begin(); - for (; j != toRefresh.end(); ++j) { - (*j)->refresh(); - didStartAny = true; + for (auto cat : toRefresh) { + cat->refresh(); + didStartAny = true; } if (!didStartAny) { @@ -677,23 +683,20 @@ void Root::catalogRefreshStatus(CatalogRef aCat, Delegate::StatusCode aReason) d->catalogs.insert(catIt, CatalogDict::value_type(aCat->id(), aCat)); // catalog might have been previously disabled, let's remove in that case - CatalogList::iterator j = std::find(d->disabledCatalogs.begin(), - d->disabledCatalogs.end(), - aCat); + auto j = std::find(d->disabledCatalogs.begin(), + d->disabledCatalogs.end(), + aCat); if (j != d->disabledCatalogs.end()) { SG_LOG(SG_GENERAL, SG_INFO, "re-enabling disabled catalog:" << aCat->id()); d->disabledCatalogs.erase(j); } } - if ((aReason != Delegate::STATUS_REFRESHED) && - (aReason != Delegate::STATUS_IN_PROGRESS) && - (aReason != Delegate::STATUS_SUCCESS)) - { + if (!aCat->isEnabled()) { // catalog has errors, disable it - CatalogList::iterator j = std::find(d->disabledCatalogs.begin(), - d->disabledCatalogs.end(), - aCat); + auto j = std::find(d->disabledCatalogs.begin(), + d->disabledCatalogs.end(), + aCat); if (j == d->disabledCatalogs.end()) { SG_LOG(SG_GENERAL, SG_INFO, "disabling catalog:" << aCat->id()); d->disabledCatalogs.push_back(aCat); @@ -703,7 +706,7 @@ void Root::catalogRefreshStatus(CatalogRef aCat, Delegate::StatusCode aReason) if (catIt != d->catalogs.end()) { d->catalogs.erase(catIt); } - } // of catalog has errors case + } // of catalog is disabled if (d->refreshing.empty()) { d->fireRefreshStatus(CatalogRef(), Delegate::STATUS_REFRESHED); @@ -718,13 +721,8 @@ bool Root::removeCatalogById(const std::string& aId) CatalogDict::iterator catIt = d->catalogs.find(aId); if (catIt == d->catalogs.end()) { // check the disabled list - CatalogList::iterator j = d->disabledCatalogs.begin(); - for (; j != d->disabledCatalogs.end(); ++j) { - if ((*j)->id() == aId) { - break; - } - } - + auto j = std::find_if(d->disabledCatalogs.begin(), d->disabledCatalogs.end(), + [aId](const CatalogRef& cat) { return cat->id() == aId; }); if (j == d->disabledCatalogs.end()) { SG_LOG(SG_GENERAL, SG_WARN, "removeCatalogById: no catalog with id:" << aId); return false; diff --git a/simgear/package/Root.hxx b/simgear/package/Root.hxx index 0a16e982..3f461740 100644 --- a/simgear/package/Root.hxx +++ b/simgear/package/Root.hxx @@ -69,7 +69,13 @@ public: std::string getLocale() const; CatalogList catalogs() const; - + + /** + * retrive all catalogs, including currently disabled ones + */ + CatalogList allCatalogs() const; + + void setMaxAgeSeconds(unsigned int seconds); unsigned int maxAgeSeconds() const; diff --git a/simgear/package/catalogTest1/catalog-v10.xml b/simgear/package/catalogTest1/catalog-v10.xml new file mode 100644 index 00000000..c86e8f79 --- /dev/null +++ b/simgear/package/catalogTest1/catalog-v10.xml @@ -0,0 +1,178 @@ + + + + org.flightgear.test.catalog1 + First test catalog + http://localhost:2000/catalogTest1/catalog-v10.xml + 4 + + 10.0.* + 10.1.* + + alpha + Alpha package + 8 + 593 + + a469c4b837f0521db48616cfe65ac1ea + http://localhost:2000/catalogTest1/alpha.zip + + alpha + + + + + c172p + Cessna 172-P + c172p + A plane made by Cessna on Jupiter + 42 + 860 + Standard author + + cessna + ga + piston + ifr + + + 3 + 4 + 5 + 4 + + + + + org.flightgear.test.catalog1.common-sounds + 10 + + + + exterior + thumb-exterior.png + http://foo.bar.com/thumb-exterior.png + + + + panel + thumb-panel.png + http://foo.bar.com/thumb-panel.png + + + + thumb-something.png + http://foo.bar.com/thumb-something.png + + + + c172p-2d-panel + C172 with 2d panel only + + + + c172p-floats + C172 with floats + A plane with floats + Floats variant author + + + exterior + thumb-exterior-floats.png + http://foo.bar.com/thumb-exterior-floats.png + + + + panel + thumb-panel.png + http://foo.bar.com/thumb-panel.png + + + http://foo.bar.com/thumb-floats.png + thumb-floats.png + + + + c172p-skis + C172 with skis + A plane with skis + + c172p + + + exterior + thumb-exterior-skis.png + http://foo.bar.com/thumb-exterior-skis.png + + + + panel + thumb-panel.png + http://foo.bar.com/thumb-panel.png + + + + + c172r + C172R + Equally good version of the C172 + _package_ + + panel + thumb-panel.png + http://foo.bar.com/thumb-panel.png + + + + + c172r-floats + C172R-floats + Equally good version of the C172 with floats + c172r + + panel + thumb-panel.png + http://foo.bar.com/thumb-panel.png + + + + ec0e2ffdf98d6a5c05c77445e5447ff5 + http://localhost:2000/catalogTest1/c172p.zip + + http://foo.bar.com/thumb-exterior.png + exterior.png + + + + b737-NG + Boeing 737 NG + b737NG + A popular twin-engined narrow body jet + 111 + 860 + + boeing + jet + ifr + + + 5 + 5 + 4 + 4 + + + a94ca5704f305b90767f40617d194ed6 + http://localhost:2000/catalogTest1/b737.tar.gz + + + + common-sounds + Common sound files for test catalog aircraft + 10 + sounds + http://localhost:2000/catalogTest1/common-sounds.zip + 360 + acf9eb89cf396eb42f8823d9cdf17584 + + diff --git a/simgear/package/catalogTest1/catalog-v2.xml b/simgear/package/catalogTest1/catalog-v2.xml index 2c8961ac..75a19c57 100644 --- a/simgear/package/catalogTest1/catalog-v2.xml +++ b/simgear/package/catalogTest1/catalog-v2.xml @@ -10,6 +10,11 @@ 8.0.0 8.2.0 + + 10.*.* + http://localhost:2000/catalogTest1/catalog-v10.xml + + alpha Alpha package diff --git a/simgear/package/catalogTest1/catalog-v9.xml b/simgear/package/catalogTest1/catalog-v9.xml new file mode 100644 index 00000000..2dcd789e --- /dev/null +++ b/simgear/package/catalogTest1/catalog-v9.xml @@ -0,0 +1,178 @@ + + + + org.flightgear.test.catalog1 + First test catalog + http://localhost:2000/catalogTest1/catalog.xml + 4 + + 9.1.* + + + alpha + Alpha package + 8 + 593 + + a469c4b837f0521db48616cfe65ac1ea + http://localhost:2000/catalogTest1/alpha.zip + + alpha + + + + + c172p + Cessna 172-P + c172p + A plane made by Cessna on Jupiter + 42 + 860 + Standard author + + cessna + ga + piston + ifr + + + 3 + 4 + 5 + 4 + + + + + org.flightgear.test.catalog1.common-sounds + 10 + + + + exterior + thumb-exterior.png + http://foo.bar.com/thumb-exterior.png + + + + panel + thumb-panel.png + http://foo.bar.com/thumb-panel.png + + + + thumb-something.png + http://foo.bar.com/thumb-something.png + + + + c172p-2d-panel + C172 with 2d panel only + + + + c172p-floats + C172 with floats + A plane with floats + Floats variant author + + + exterior + thumb-exterior-floats.png + http://foo.bar.com/thumb-exterior-floats.png + + + + panel + thumb-panel.png + http://foo.bar.com/thumb-panel.png + + + http://foo.bar.com/thumb-floats.png + thumb-floats.png + + + + c172p-skis + C172 with skis + A plane with skis + + c172p + + + exterior + thumb-exterior-skis.png + http://foo.bar.com/thumb-exterior-skis.png + + + + panel + thumb-panel.png + http://foo.bar.com/thumb-panel.png + + + + + c172r + C172R + Equally good version of the C172 + _package_ + + panel + thumb-panel.png + http://foo.bar.com/thumb-panel.png + + + + + c172r-floats + C172R-floats + Equally good version of the C172 with floats + c172r + + panel + thumb-panel.png + http://foo.bar.com/thumb-panel.png + + + + ec0e2ffdf98d6a5c05c77445e5447ff5 + http://localhost:2000/catalogTest1/c172p.zip + + http://foo.bar.com/thumb-exterior.png + exterior.png + + + + b737-NG + Boeing 737 NG + b737NG + A popular twin-engined narrow body jet + 111 + 860 + + boeing + jet + ifr + + + 5 + 5 + 4 + 4 + + + a94ca5704f305b90767f40617d194ed6 + http://localhost:2000/catalogTest1/b737.tar.gz + + + + common-sounds + Common sound files for test catalog aircraft + 10 + sounds + http://localhost:2000/catalogTest1/common-sounds.zip + 360 + acf9eb89cf396eb42f8823d9cdf17584 + +