Merge pull request #6066 from sminnee/campaignadmin-error-check

FIX: Graceful degradation if obsolete classnames in ChangeSetItem (fixes #6065)

ORM/UnexpectedDataException.php

@@ -0,0 +1,10 @@
+<?php
+
+namespace SilverStripe\ORM;
+
+/**
+ * Throw this exception whenever unexpected data is found.
+ */
+class UnexpectedDataException extends \Exception
+{
+}

ORM/Versioning/ChangeSet.php

@@ -11,6 +11,7 @@ use SilverStripe\ORM\HasManyList;
 use SilverStripe\ORM\ValidationException;
 use SilverStripe\ORM\DB;
 use SilverStripe\ORM\DataObject;
+use SilverStripe\ORM\UnexpectedDataException;
 use SilverStripe\Security\Member;
 use SilverStripe\Security\Permission;
 use BadMethodCallException;
@@ -202,15 +203,20 @@ class ChangeSet extends DataObject {
 			$explicit[$explicitKey] = true;
 
 			foreach ($item->findReferenced() as $referee) {
-				/** @var DataObject $referee */
+				try {
-				$key = $this->implicitKey($referee);
+					/** @var DataObject $referee */
+					$key = $this->implicitKey($referee);
 
-				$referenced[$key] = [
+					$referenced[$key] = [
-					'ObjectID' => $referee->ID,
+						'ObjectID' => $referee->ID,
-					'ObjectClass' => $referee->baseClass(),
+						'ObjectClass' => $referee->baseClass(),
-				];
+					];
 
-				$references[$key][] = $item->ID;
+					$references[$key][] = $item->ID;
+
+				// Skip any bad records
+				} catch(UnexpectedDataException $e) {
+				}
 			}
 		}
 

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.");

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 */
+		// Before presenting the changeset to the client,
-			$resource = $this->getChangeSetItemResource($changeSetItem);
+		// synchronise it with new changes.
-			$hal['_embedded']['items'][] = $resource;
+		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;
 	}
 

admin/tests/CampaignAdminTest.php

@@ -0,0 +1,40 @@
+<?php
+
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Admin\CampaignAdmin;
+use SilverStripe\ORM\Versioning\ChangeSet;
+use SilverStripe\ORM\UnexpectedDataException;
+
+class CampaignAdminTest extends SapphireTest
+{
+
+	/**
+	 * Call a protected method on an object via reflection
+	 *
+	 * @param object $object The object to call the method on
+	 * @param string $method The name of the method
+	 * @param array $args The arguments to pass to the method
+	 */
+	function callProtectedMethod($object, $method, $args = []) {
+	  $class = new ReflectionClass(get_class($object));
+	  $methodObj = $class->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");
+	}
+}