From c52adad1fec6ad5fcb4a99623fbc73d033688517 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Fri, 23 Sep 2016 11:05:33 +1200 Subject: [PATCH] FIX: Graceful degradation if obsolete classnames in ChangeSetItem (fixes #6065) NEW: Add SilverStripe\ORM\UnexpectedDataException class. This change provides more graceful handling of the case where a ChangeSetItem is referencing a no-longer-existing class. The new exception, SilverStripe\ORM\UnexpectedDataException, is intended to be available for throwing whenever we have unexpected data in the database. It can be trapped by the relevant UIs and more graceful errors than HTTP 500s can be provided. --- ORM/UnexpectedDataException.php | 10 ++++++++ ORM/Versioning/ChangeSet.php | 20 ++++++++++------ ORM/Versioning/ChangeSetItem.php | 17 +++++++++++++ admin/code/CampaignAdmin.php | 37 ++++++++++++++++++---------- admin/tests/CampaignAdminTest.php | 40 +++++++++++++++++++++++++++++++ 5 files changed, 104 insertions(+), 20 deletions(-) create mode 100644 ORM/UnexpectedDataException.php create mode 100644 admin/tests/CampaignAdminTest.php diff --git a/ORM/UnexpectedDataException.php b/ORM/UnexpectedDataException.php new file mode 100644 index 000000000..910bc6699 --- /dev/null +++ b/ORM/UnexpectedDataException.php @@ -0,0 +1,10 @@ +findReferenced() as $referee) { - /** @var DataObject $referee */ - $key = $this->implicitKey($referee); + try { + /** @var DataObject $referee */ + $key = $this->implicitKey($referee); - $referenced[$key] = [ - 'ObjectID' => $referee->ID, - 'ObjectClass' => $referee->baseClass(), - ]; + $referenced[$key] = [ + 'ObjectID' => $referee->ID, + 'ObjectClass' => $referee->baseClass(), + ]; - $references[$key][] = $item->ID; + $references[$key][] = $item->ID; + + // Skip any bad records + } catch(UnexpectedDataException $e) { + } } } diff --git a/ORM/Versioning/ChangeSetItem.php b/ORM/Versioning/ChangeSetItem.php index 67f3a3ab3..e4f005837 100644 --- a/ORM/Versioning/ChangeSetItem.php +++ b/ORM/Versioning/ChangeSetItem.php @@ -9,6 +9,7 @@ use SilverStripe\ORM\DataList; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\ManyManyList; use SilverStripe\ORM\SS_List; +use SilverStripe\ORM\UnexpectedDataException; use SilverStripe\Security\Member; use SilverStripe\Security\Permission; use BadMethodCallException; @@ -112,6 +113,10 @@ class ChangeSetItem extends DataObject implements Thumbnail { * @return string */ public function getChangeType() { + if(!class_exists($this->ObjectClass)) { + throw new UnexpectedDataException("Invalid Class '{$this->ObjectClass}' in ChangeSetItem #{$this->ID}"); + } + // Get change versions if($this->VersionBefore || $this->VersionAfter) { $draftVersion = $this->VersionAfter; // After publishing draft was written to stage @@ -144,6 +149,10 @@ class ChangeSetItem extends DataObject implements Thumbnail { * @return Versioned|DataObject */ protected function getObjectInStage($stage) { + if(!class_exists($this->ObjectClass)) { + throw new UnexpectedDataException("Invalid Class '{$this->ObjectClass}' in ChangeSetItem #{$this->ID}"); + } + return Versioned::get_by_stage($this->ObjectClass, $stage)->byID($this->ObjectID); } @@ -153,6 +162,10 @@ class ChangeSetItem extends DataObject implements Thumbnail { * @return Versioned|DataObject */ protected function getObjectLatestVersion() { + if(!class_exists($this->ObjectClass)) { + throw new UnexpectedDataException("Invalid Class '{$this->ObjectClass}' in ChangeSetItem #{$this->ID}"); + } + return Versioned::get_latest_version($this->ObjectClass, $this->ObjectID); } @@ -180,6 +193,10 @@ class ChangeSetItem extends DataObject implements Thumbnail { * Note: Unlike Versioned::doPublish() and Versioned::doUnpublish, this action is not recursive. */ public function publish() { + if(!class_exists($this->ObjectClass)) { + throw new UnexpectedDataException("Invalid Class '{$this->ObjectClass}' in ChangeSetItem #{$this->ID}"); + } + // Logical checks prior to publish if(!$this->canPublish()) { throw new Exception("The current member does not have permission to publish this ChangeSetItem."); diff --git a/admin/code/CampaignAdmin.php b/admin/code/CampaignAdmin.php index 95b987f3b..b3dd8f4d3 100644 --- a/admin/code/CampaignAdmin.php +++ b/admin/code/CampaignAdmin.php @@ -14,6 +14,7 @@ use SilverStripe\ORM\SS_List; use SilverStripe\ORM\Versioning\ChangeSet; use SilverStripe\ORM\Versioning\ChangeSetItem; use SilverStripe\ORM\DataObject; +use SilverStripe\ORM\UnexpectedDataException; use SilverStripe\Security\SecurityToken; use SilverStripe\Security\PermissionProvider; use LogicException; @@ -256,9 +257,6 @@ JSON; * @return array */ protected function getChangeSetResource(ChangeSet $changeSet) { - // Before presenting the changeset to the client, - // synchronise it with new changes. - $changeSet->sync(); $hal = [ '_links' => [ 'self' => [ @@ -267,24 +265,37 @@ JSON; ], 'ID' => $changeSet->ID, 'Name' => $changeSet->Name, - 'Description' => $changeSet->getDescription(), 'Created' => $changeSet->Created, 'LastEdited' => $changeSet->LastEdited, 'State' => $changeSet->State, 'canEdit' => $changeSet->canEdit(), - 'canPublish' => $changeSet->canPublish(), + 'canPublish' => false, '_embedded' => ['items' => []] ]; - foreach($changeSet->Changes() as $changeSetItem) { - if(!$changeSetItem) { - continue; - } - /** @var ChangesetItem $changeSetItem */ - $resource = $this->getChangeSetItemResource($changeSetItem); - $hal['_embedded']['items'][] = $resource; + // Before presenting the changeset to the client, + // synchronise it with new changes. + try { + $changeSet->sync(); + $hal['Description'] = $changeSet->getDescription(); + $hal['canPublish'] = $changeSet->canPublish(); + + foreach($changeSet->Changes() as $changeSetItem) { + if(!$changeSetItem) { + continue; + } + + /** @var ChangesetItem $changeSetItem */ + $resource = $this->getChangeSetItemResource($changeSetItem); + $hal['_embedded']['items'][] = $resource; + } + $hal['ChangesCount'] = count($hal['_embedded']['items']); + + // An unexpected data exception means that the database is corrupt + } catch(UnexpectedDataException $e) { + $hal['Description'] = 'Corrupt database! ' . $e->getMessage(); + $hal['ChangesCount'] = '-'; } - $hal['ChangesCount'] = count($hal['_embedded']['items']); return $hal; } diff --git a/admin/tests/CampaignAdminTest.php b/admin/tests/CampaignAdminTest.php new file mode 100644 index 000000000..12a494642 --- /dev/null +++ b/admin/tests/CampaignAdminTest.php @@ -0,0 +1,40 @@ +getMethod($method); + $methodObj->setAccessible(true); + return $methodObj->invokeArgs($object, $args); + } + + function testInvalidDataHandling() { + $changeset = new CampaignAdminTest_InvalidChangeSet(); + $admin = new CampaignAdmin(); + + $result = $this->callProtectedMethod($admin, 'getChangeSetResource', [$changeset] ); + $this->assertEquals('Corrupt database! bad data' , $result['Description']); + } +} + +class CampaignAdminTest_InvalidChangeSet extends ChangeSet +{ + function sync() + { + throw new UnexpectedDataException("bad data"); + } +}