N°8764 halt setup wizard at data issue - review

- 2 types of SetupAudit constructors
- setup wizard new step management enhancement
- change SetupAudit GetIssue API behaviour

setup/applicationinstaller.class.inc.php

@@ -17,13 +17,13 @@
 //   You should have received a copy of the GNU Affero General Public License
 //   along with iTop. If not, see <http://www.gnu.org/licenses/>
 
+use Combodo\iTop\Setup\FeatureRemoval\InplaceSetupAudit;
 use Combodo\iTop\Setup\FeatureRemoval\ModelReflectionSerializer;
-use Combodo\iTop\Setup\FeatureRemoval\SetupAudit;
 
 require_once(APPROOT.'setup/parameters.class.inc.php');
 require_once(APPROOT.'setup/xmldataloader.class.inc.php');
 require_once(APPROOT.'setup/backup.class.inc.php');
-require_once APPROOT.'setup/feature_removal/SetupAudit.php';
+require_once APPROOT.'setup/feature_removal/InplaceSetupAudit.php';
 
 /**
  * The base class for the installation process.
@@ -262,7 +262,7 @@ class ApplicationInstaller
 					$sExtensionDir = $this->oParams->Get('extensions_dir', 'extensions');
 					$aMiscOptions = $this->oParams->Get('options', []);
 					$aRemovedExtensionCodes = $this->oParams->Get('removed_extensions', []);
-					$sDisableDataAudit = $this->oParams->Get('disable-data-audit', '');
+					$sSkipDataAudit = $this->oParams->Get('skip-data-audit', '');
 
 					$bUseSymbolicLinks = null;
 					if ((isset($aMiscOptions['symlinks']) && $aMiscOptions['symlinks'])) {
@@ -274,27 +274,36 @@ class ApplicationInstaller
 						}
 					}
 
+					$aParamValues = $this->oParams->GetParamForConfigArray();
+					$bIsSetupDataAuditEnabled = $this->IsSetupDataAuditEnabled($sSkipDataAudit, $aParamValues);
 					$this->DoCompile(
 						$aRemovedExtensionCodes,
 						$aSelectedModules,
 						$sSourceDir,
 						$sExtensionDir,
-						$sDisableDataAudit,
+						$bIsSetupDataAuditEnabled,
 						$bUseSymbolicLinks
 					);
 
+					if ($bIsSetupDataAuditEnabled) {
+						$sNextStep = 'setup-audit';
+						$sNextStepLabel = 'Checking data consistency with the new data model';
+					} else {
+						$sNextStep = 'db-schema';
+						$sNextStepLabel = 'Updating database schema';
+					}
+
 					$aResult = [
 						'status' => self::OK,
 						'message' => '',
-						'next-step' => 'setup-audit',
-						'next-step-label' => 'Checking data consistency with the new data model',
+						'next-step' => $sNextStep,
+						'next-step-label' => $sNextStepLabel,
 						'percentage-completed' => 40,
 					];
 					break;
 
 				case 'setup-audit':
-					$sDisableDataAudit = $this->oParams->Get('disable-data-audit', '');
-					$this->DoSetupAudit($sDisableDataAudit);
+					$this->DoSetupAudit();
 					$aResult = [
 						'status' => self::OK,
 						'message' => '',
@@ -499,7 +508,7 @@ class ApplicationInstaller
 	 * @param array $aSelectedModules
 	 * @param string $sSourceDir
 	 * @param string $sExtensionDir
-	 * @param string $sDisableDataAudit
+	 * @param bool $bIsSetupDataAuditEnabled
 	 * @param boolean $bUseSymbolicLinks
 	 *
 	 * @return void
@@ -508,7 +517,7 @@ class ApplicationInstaller
 	 *
 	 * @since 3.1.0 N°2013 added the aParamValues param
 	 */
-	protected function DoCompile($aRemovedExtensionCodes, $aSelectedModules, $sSourceDir, $sExtensionDir, $sDisableDataAudit, $bUseSymbolicLinks = null)
+	protected function DoCompile($aRemovedExtensionCodes, $aSelectedModules, $sSourceDir, $sExtensionDir, $bIsSetupDataAuditEnabled, $bUseSymbolicLinks = null)
 	{
 		/**
 	 * @since 3.2.0 move the ContextTag init at the very beginning of the method
@@ -549,7 +558,7 @@ class ApplicationInstaller
 		}
 
 		$bIsAlreadyInMaintenanceMode = SetupUtils::IsInMaintenanceMode();
-		if ($this->IsSetupDataAuditEnabled($sDisableDataAudit, $aParamValues)) {
+		if ($bIsSetupDataAuditEnabled) {
 			if ($bIsAlreadyInMaintenanceMode) {
 				//required to read DM before calling SaveModelInfo
 				SetupUtils::ExitMaintenanceMode();
@@ -650,21 +659,21 @@ class ApplicationInstaller
 		}
 	}
 
-	private function GetModelInfoPath(): string
+	private function GetModelInfoPath(string $sEnv): string
 	{
-		return APPROOT.'data/beforecompilation_modelinfo.json';
+		return APPROOT."data/beforecompilation_".$sEnv."_modelinfo.json";
 	}
 
 	private function SaveModelInfo(string $sEnvironment): void
 	{
 		$aModelInfo = ModelReflectionSerializer::GetInstance()->GetModelFromEnvironment($sEnvironment);
-		$sModelInfoPath = $this->GetModelInfoPath();
+		$sModelInfoPath = $this->GetModelInfoPath($sEnvironment);
 		file_put_contents($sModelInfoPath, json_encode($aModelInfo));
 	}
 
 	private function GetPreviousModelInfo(string $sEnvironment): array
 	{
-		$sContent = file_get_contents($this->GetModelInfoPath());
+		$sContent = file_get_contents($this->GetModelInfoPath($sEnvironment));
 		$aModelInfo = json_decode($sContent, true);
 
 		if (false === $aModelInfo) {
@@ -674,7 +683,7 @@ class ApplicationInstaller
 		return $aModelInfo;
 	}
 
-	protected function DoSetupAudit(string $sDisableDataAudit)
+	protected function DoSetupAudit()
 	{
 		/**
 		 * @since 3.2.0 move the ContextTag init at the very beginning of the method
@@ -682,35 +691,29 @@ class ApplicationInstaller
 		 */
 		$oContextTag = new ContextTag(ContextTag::TAG_SETUP);
 
-		$aParamValues = $this->oParams->GetParamForConfigArray();
-		if (! $this->IsSetupDataAuditEnabled($sDisableDataAudit, $aParamValues)) {
-			return;
-		}
-
 		$sTargetEnvironment = $this->GetTargetEnv();
 		$aPreviousCompilationModelInfo = $this->GetPreviousModelInfo($sTargetEnvironment);
 
-		$oSetupAudit = new SetupAudit($sTargetEnvironment, $sTargetEnvironment);
-		$oSetupAudit->ComputeClasses($aPreviousCompilationModelInfo);
+		$oSetupAudit = new InplaceSetupAudit($aPreviousCompilationModelInfo, $sTargetEnvironment);
 
-		try {
-			$oSetupAudit->GetIssues(true);
-		} catch (Exception $e) {
-			$iCount = $oSetupAudit->GetLastComputedFinalClassesRemovedCount();
+		$oSetupAudit->GetIssues(true);
+		$iCount = $oSetupAudit->GetDataToCleanupCount();
+
+		if ($iCount > 0) {
 			throw new Exception("$iCount elements require data adjustments or cleanup in the backoffice prior to upgrading iTop");
 		}
 	}
 
-	private function IsSetupDataAuditEnabled($sDisableDataAudit, array $aParamValues): bool
+	private function IsSetupDataAuditEnabled($sSkipDataAudit, array $aParamValues): bool
 	{
-		$sMode = $aParamValues['mode'];
-		if ($sMode !== "upgrade") {
-			//first install
+		if ($sSkipDataAudit === "checked") {
+			SetupLog::Info("Setup data audit disabled", null, ['skip-data-audit' => $sSkipDataAudit]);
 			return false;
 		}
 
-		if ($sDisableDataAudit === "checked") {
-			SetupLog::Info("Setup data audit disabled", null, ['disable-data-audit' => $sDisableDataAudit]);
+		$sMode = $aParamValues['mode'];
+		if ($sMode !== "upgrade") {
+			//first install
 			return false;
 		}
 

setup/feature_removal/AbstractSetupAudit.php

@@ -0,0 +1,123 @@
+<?php
+
+namespace Combodo\iTop\Setup\FeatureRemoval;
+
+use ContextTag;
+use DBObjectSearch;
+use DBObjectSet;
+use IssueLog;
+use MetaModel;
+use SetupLog;
+
+require_once APPROOT.'setup/feature_removal/ModelReflectionSerializer.php';
+
+abstract class AbstractSetupAudit
+{
+	//file used when present to trigger audit exception when testing specific setups
+	public const GETISSUE_ERROR_MSG_FILE_FORTESTONLY = '.setup_audit_error_msg.txt';
+
+	protected bool $bClassesInitialized = false;
+	protected array $aClassesBeforeRemoval = [];
+	protected array $aClassesAfterRemoval = [];
+	protected array $aRemovedClasses = [];
+	protected array $aFinalClassesToCleanup = [];
+
+	public function __construct()
+	{
+	}
+
+	abstract public function ComputeClasses(): void;
+
+	public function GetRemovedClasses(): array
+	{
+		$this->ComputeClasses();
+
+		if (count($this->aRemovedClasses) == 0) {
+			if (count($this->aClassesBeforeRemoval) == 0) {
+				return $this->aRemovedClasses;
+			}
+
+			if (count($this->aClassesAfterRemoval) == 0) {
+				return $this->aRemovedClasses;
+			}
+
+			$aExtensionsNames = array_diff($this->aClassesBeforeRemoval, $this->aClassesAfterRemoval);
+			$this->aRemovedClasses = [];
+			$aClasses = array_values($aExtensionsNames);
+			sort($aClasses);
+
+			foreach ($aClasses as $i => $sClass) {
+				$this->aRemovedClasses[] = $sClass;
+			}
+		}
+
+		return $this->aRemovedClasses;
+	}
+
+	/** test only: return file path that force audit error being raised
+	 *
+	 * @return string
+	 */
+	public static function GetErrorMessageFilePathForTestOnly(): string
+	{
+		return APPROOT."/data/".self::GETISSUE_ERROR_MSG_FILE_FORTESTONLY;
+	}
+
+	public function GetIssues(bool $bStopDataCheckAtFirstIssue = false): array
+	{
+		$sErrorMessageFilePath = self::GetErrorMessageFilePathForTestOnly();
+		if ($bStopDataCheckAtFirstIssue && is_file($sErrorMessageFilePath)) {
+			$sMsg = file_get_contents($sErrorMessageFilePath);
+			throw new \Exception($sMsg);
+		}
+
+		$this->aFinalClassesToCleanup = [];
+
+		foreach ($this->GetRemovedClasses() as $sClass) {
+			if (MetaModel::IsAbstract($sClass)) {
+				continue;
+			}
+
+			if (!MetaModel::IsStandaloneClass($sClass)) {
+				$iCount = $this->Count($sClass);
+				$this->aFinalClassesToCleanup[$sClass] = $iCount;
+				if ($bStopDataCheckAtFirstIssue && $iCount > 0) {
+					//setup envt: should raise issue ASAP
+					$this->LogInfoWithProperLogger("Setup audit found data to cleanup", null, $this->aFinalClassesToCleanup);
+					return $this->aFinalClassesToCleanup;
+				}
+			}
+		}
+
+		$this->LogInfoWithProperLogger("Setup audit found data to cleanup", null, ['data_to_cleanup' => $this->aFinalClassesToCleanup]);
+		return $this->aFinalClassesToCleanup;
+	}
+
+	public function GetDataToCleanupCount(): int
+	{
+		$res = 0;
+		foreach ($this->aFinalClassesToCleanup as $sClass => $iCount) {
+			$res += $iCount;
+		}
+		return $res;
+	}
+
+	private function Count($sClass): int
+	{
+		$oSearch = DBObjectSearch::FromOQL("SELECT $sClass", []);
+		$oSearch->AllowAllData();
+		$oSet = new DBObjectSet($oSearch);
+
+		return $oSet->Count();
+	}
+
+	//could be shared with others in log APIs ?
+	private function LogInfoWithProperLogger($sMessage, $sChannel = null, $aContext = []): void
+	{
+		if (ContextTag::Check(ContextTag::TAG_SETUP)) {
+			SetupLog::Info($sMessage, $sChannel, $aContext);
+		} else {
+			IssueLog::Info($sMessage, $sChannel, $aContext);
+		}
+	}
+}

setup/feature_removal/InplaceSetupAudit.php

@@ -0,0 +1,40 @@
+<?php
+
+namespace Combodo\iTop\Setup\FeatureRemoval;
+
+use MetaModel;
+
+require_once __DIR__.'/AbstractSetupAudit.php';
+require_once APPROOT.'setup/feature_removal/ModelReflectionSerializer.php';
+
+class InplaceSetupAudit extends AbstractSetupAudit
+{
+	//file used when present to trigger audit exception when testing specific setups
+	public const GETISSUE_ERROR_MSG_FILE_FORTESTONLY = '.setup_audit_error_msg.txt';
+
+	private string $sEnvAfterExtensionRemoval;
+
+	public function __construct(array $aClassesBeforeRemoval, string $sEnvAfterExtensionRemoval)
+	{
+		parent::__construct();
+		$this->aClassesBeforeRemoval = $aClassesBeforeRemoval;
+		$this->sEnvAfterExtensionRemoval = $sEnvAfterExtensionRemoval;
+	}
+
+	public function ComputeClasses(): void
+	{
+		if ($this->bClassesInitialized) {
+			return;
+		}
+
+		$sCurrentEnvt = MetaModel::GetEnvironment();
+
+		if ($sCurrentEnvt === $this->sEnvAfterExtensionRemoval) {
+			$this->aClassesAfterRemoval = MetaModel::GetClasses();
+		} else {
+			$this->aClassesAfterRemoval = ModelReflectionSerializer::GetInstance()->GetModelFromEnvironment($this->sEnvAfterExtensionRemoval);
+		}
+
+		$this->bClassesInitialized = true;
+	}
+}

setup/feature_removal/SetupAudit.php

@@ -2,16 +2,12 @@
 
 namespace Combodo\iTop\Setup\FeatureRemoval;
 
-use ContextTag;
-use DBObjectSearch;
-use DBObjectSet;
-use IssueLog;
 use MetaModel;
-use SetupLog;
 
+require_once __DIR__.'/AbstractSetupAudit.php';
 require_once APPROOT.'setup/feature_removal/ModelReflectionSerializer.php';
 
-class SetupAudit
+class SetupAudit extends AbstractSetupAudit
 {
 	//file used when present to trigger audit exception when testing specific setups
 	public const GETISSUE_ERROR_MSG_FILE_FORTESTONLY = '.setup_audit_error_msg.txt';
@@ -19,34 +15,24 @@ class SetupAudit
 	private string $sEnvBeforeExtensionRemoval;
 	private string $sEnvAfterExtensionRemoval;
 
-	private bool $bClassesInitialized = false;
-	private array $aClassesBeforeRemoval = [];
-	private array $aClassesAfterRemoval = [];
-	private array $aRemovedClasses = [];
-	private array $aFinalClassesRemoved = [];
-
-	public function __construct(string $sEnvBeforeExtensionRemoval, string $sEnvAfterExtensionRemoval = DryRemovalRuntimeEnvironment::DRY_REMOVAL_AUDIT_ENV)
+	public function __construct(string $sEnvBeforeExtensionRemoval, string $sEnvAfterExtensionRemoval)
 	{
+		parent::__construct();
 		$this->sEnvBeforeExtensionRemoval = $sEnvBeforeExtensionRemoval;
 		$this->sEnvAfterExtensionRemoval = $sEnvAfterExtensionRemoval;
 	}
 
-	public function ComputeClasses(array $aClassesBeforeRemoval = null)
+	public function ComputeClasses(): void
 	{
 		if ($this->bClassesInitialized) {
 			return;
 		}
 
 		$sCurrentEnvt = MetaModel::GetEnvironment();
-
-		if (is_null($aClassesBeforeRemoval)) {
-			if ($sCurrentEnvt === $this->sEnvBeforeExtensionRemoval) {
-				$this->aClassesBeforeRemoval = MetaModel::GetClasses();
-			} else {
-				$this->aClassesBeforeRemoval = ModelReflectionSerializer::GetInstance()->GetModelFromEnvironment($this->sEnvBeforeExtensionRemoval);
-			}
+		if ($sCurrentEnvt === $this->sEnvBeforeExtensionRemoval) {
+			$this->aClassesBeforeRemoval = MetaModel::GetClasses();
 		} else {
-			$this->aClassesBeforeRemoval = $aClassesBeforeRemoval;
+			$this->aClassesBeforeRemoval = ModelReflectionSerializer::GetInstance()->GetModelFromEnvironment($this->sEnvBeforeExtensionRemoval);
 		}
 
 		if ($sCurrentEnvt === $this->sEnvAfterExtensionRemoval) {
@@ -94,71 +80,4 @@ class SetupAudit
 
 		return $this->aRemovedClasses;
 	}
-
-	/** test only: return file path that force audit error being raised
-	 *
-	 * @return string
-	 */
-	public static function GetErrorMessageFilePathForTestOnly(): string
-	{
-		return APPROOT."/data/".self::GETISSUE_ERROR_MSG_FILE_FORTESTONLY;
-	}
-
-	public function GetIssues(bool $bThrowExceptionAtFirstIssue = false): array
-	{
-		$sErrorMessageFilePath = self::GetErrorMessageFilePathForTestOnly();
-		if ($bThrowExceptionAtFirstIssue && is_file($sErrorMessageFilePath)) {
-			$sMsg = file_get_contents($sErrorMessageFilePath);
-			throw new \Exception($sMsg);
-		}
-
-		$this->aFinalClassesRemoved = [];
-
-		foreach ($this->GetRemovedClasses() as $sClass) {
-			if (MetaModel::IsAbstract($sClass)) {
-				continue;
-			}
-
-			if (!MetaModel::IsStandaloneClass($sClass)) {
-				$iCount = $this->Count($sClass);
-				$this->aFinalClassesRemoved[$sClass] = $iCount;
-				if ($bThrowExceptionAtFirstIssue && $iCount > 0) {
-					//setup envt: should raise issue ASAP
-					$this->LogInfoWithProperLogger("Setup audit found data to cleanup", null, $this->aFinalClassesRemoved);
-					throw new \Exception($sClass);
-				}
-			}
-		}
-
-		$this->LogInfoWithProperLogger("Setup audit found data to cleanup", null, $this->aFinalClassesRemoved);
-		return $this->aFinalClassesRemoved;
-	}
-
-	public function GetLastComputedFinalClassesRemovedCount(): int
-	{
-		$res = 0;
-		foreach ($this->aFinalClassesRemoved as $sClass => $iCount) {
-			$res += $iCount;
-		}
-		return $res;
-	}
-
-	private function Count($sClass): int
-	{
-		$oSearch = DBObjectSearch::FromOQL("SELECT $sClass", []);
-		$oSearch->AllowAllData();
-		$oSet = new DBObjectSet($oSearch);
-
-		return $oSet->Count();
-	}
-
-	//could be shared with others in log APIs ?
-	private function LogInfoWithProperLogger($sMessage, $sChannel = null, $aContext = []): void
-	{
-		if (ContextTag::Check(ContextTag::TAG_SETUP)) {
-			SetupLog::Info($sMessage, $sChannel, $aContext);
-		} else {
-			IssueLog::Info($sMessage, $sChannel, $aContext);
-		}
-	}
 }

tests/php-unit-tests/unitary-tests/setup/feature_removal/SetupAuditTest.php

@@ -5,6 +5,7 @@ namespace Combodo\iTop\Test\UnitTest\Setup\FeatureRemoval;
 use Combodo\iTop\Setup\FeatureRemoval\DryRemovalRuntimeEnvironment;
 use Combodo\iTop\Setup\FeatureRemoval\ModelReflectionSerializer;
 use Combodo\iTop\Setup\FeatureRemoval\SetupAudit;
+use Combodo\iTop\Setup\FeatureRemoval\InplaceSetupAudit;
 use Combodo\iTop\Test\UnitTest\ItopCustomDatamodelTestCase;
 use Combodo\iTop\Test\UnitTest\Service\UnitTestRunTimeEnvironment;
 use Exception;
@@ -40,6 +41,7 @@ class SetupAuditTest extends ItopCustomDatamodelTestCase
 		parent::setUp();
 
 		$this->RequireOnceItopFile('/setup/feature_removal/SetupAudit.php');
+		$this->RequireOnceItopFile('/setup/feature_removal/InplaceSetupAudit.php');
 		$this->RequireOnceItopFile('/setup/feature_removal/DryRemovalRuntimeEnvironment.php');
 	}
 
@@ -54,7 +56,7 @@ class SetupAuditTest extends ItopCustomDatamodelTestCase
 		$oDryRemovalRuntimeEnvt->Prepare($this->GetTestEnvironment(), ['nominal_ext1', 'finalclass_ext2']);
 		$oDryRemovalRuntimeEnvt->CompileFrom($this->GetTestEnvironment());
 
-		$oSetupAudit = new SetupAudit(MetaModel::GetEnvironment());
+		$oSetupAudit = new SetupAudit(MetaModel::GetEnvironment(), DryRemovalRuntimeEnvironment::DRY_REMOVAL_AUDIT_ENV);
 
 		$expected = [
 			"Feature1Module1MyClass",
@@ -73,11 +75,11 @@ class SetupAuditTest extends ItopCustomDatamodelTestCase
 	{
 		$sEnv = MetaModel::GetEnvironment();
 
-		$aClassesAfterRemoval = ModelReflectionSerializer::GetInstance()->GetModelFromEnvironment($sEnv);
-		$aClassesAfterRemoval[] = "GabuZomeu";
+		$aClassesBeforeRemoval = ModelReflectionSerializer::GetInstance()->GetModelFromEnvironment($sEnv);
+		$aClassesBeforeRemoval[] = "GabuZomeu";
 
-		$oSetupAudit = new SetupAudit($sEnv, $sEnv);
-		$oSetupAudit->ComputeClasses($aClassesAfterRemoval);
+		$oSetupAudit = new InplaceSetupAudit($aClassesBeforeRemoval, $sEnv);
+		$oSetupAudit->ComputeClasses($aClassesBeforeRemoval);
 		$this->assertEquals(["GabuZomeu"], $oSetupAudit->GetRemovedClasses());
 	}
 
@@ -87,7 +89,7 @@ class SetupAuditTest extends ItopCustomDatamodelTestCase
 		$oOrg = $this->CreateOrganization($sUID);
 		$this->createObject('FinalClassFeature1Module1MyFinalClassFromLocation', ['org_id' => $oOrg->GetKey(), 'name' => $sUID, 'name2' => uniqid()]);
 
-		$oSetupAudit = new SetupAudit(MetaModel::GetEnvironment());
+		$oSetupAudit = new SetupAudit(MetaModel::GetEnvironment(), DryRemovalRuntimeEnvironment::DRY_REMOVAL_AUDIT_ENV);
 		$aRemovedClasses = [
 			"Feature1Module1MyClass",
 			"FinalClassFeature1Module1MyClass",
@@ -113,7 +115,7 @@ class SetupAuditTest extends ItopCustomDatamodelTestCase
 		$this->createObject('FinalClassFeature1Module1MyFinalClassFromLocation', ['org_id' => $oOrg->GetKey(), 'name' => $sUID, 'name2' => uniqid()]);
 		$this->createObject('FinalClassFeature2Module1MyFinalClassFromLocation', ['org_id' => $oOrg->GetKey(), 'name' => $sUID, 'name2' => uniqid()]);
 
-		$oSetupAudit = new SetupAudit(MetaModel::GetEnvironment());
+		$oSetupAudit = new SetupAudit(MetaModel::GetEnvironment(), DryRemovalRuntimeEnvironment::DRY_REMOVAL_AUDIT_ENV);
 		$aRemovedClasses = [
 			"Feature1Module1MyClass",
 			"FinalClassFeature1Module1MyClass",
@@ -125,8 +127,9 @@ class SetupAuditTest extends ItopCustomDatamodelTestCase
 		//avoid setup dry computation
 		$this->SetNonPublicProperty($oSetupAudit, 'aRemovedClasses', $aRemovedClasses);
 
-		$this->expectException(Exception::class);
-		$this->expectExceptionMessage('FinalClassFeature1Module1MyFinalClassFromLocation');
-		$oSetupAudit->GetIssues(true);
+		$expected = [
+			"FinalClassFeature1Module1MyFinalClassFromLocation" => 1,
+		];
+		$this->assertEqualsCanonicalizing($expected, $oSetupAudit->GetIssues(true));
 	}
 }