From c14a01eb8ba35a07caec89af875378d047a9ede3 Mon Sep 17 00:00:00 2001 From: snipe Date: Fri, 13 Oct 2023 12:18:19 +0100 Subject: [PATCH 1/6] Added comments to explain the potential race condition Signed-off-by: snipe --- app/Observers/AssetObserver.php | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/app/Observers/AssetObserver.php b/app/Observers/AssetObserver.php index 3cbaedded2..55542ff2eb 100644 --- a/app/Observers/AssetObserver.php +++ b/app/Observers/AssetObserver.php @@ -120,17 +120,29 @@ class AssetObserver $logAction->user_id = Auth::id(); $logAction->logaction('delete'); } - + + /** + * Executes every time an asset is saved. + * + * This matters specifically because any database fields affected here MUST already exist on + * the assets table (and/or any related models), or related migrations WILL fail. + * + * For example, if there is a database migration that's a bit older and modifies an asset, if the save + * fires before a field gets created in a later migration and that field in the later migration + * is used in this observer, it doesn't actually exist yet and the migration will break. + * + * @see https://github.com/snipe/snipe-it/issues/13723#issuecomment-1761315938 + */ public function saving(Asset $asset) { - //determine if calculated eol and then calculate it - this should only happen on a new asset - if(is_null($asset->asset_eol_date) && !is_null($asset->purchase_date) && !is_null($asset->model->eol)){ + // determine if calculated eol and then calculate it - this should only happen on a new asset + if (is_null($asset->asset_eol_date) && !is_null($asset->purchase_date) && !is_null($asset->model->eol)){ $asset->asset_eol_date = $asset->purchase_date->addMonths($asset->model->eol)->format('Y-m-d'); $asset->eol_explicit = false; } - //determine if explicit and set eol_explit to true - if(!is_null($asset->asset_eol_date) && !is_null($asset->purchase_date)) { + // determine if explicit and set eol_explicit to true + if (!is_null($asset->asset_eol_date) && !is_null($asset->purchase_date)) { if($asset->model->eol) { $months = Carbon::parse($asset->asset_eol_date)->diffInMonths($asset->purchase_date); if($months != $asset->model->eol) { From 0b39591d882a263a106ffd8f80133f4959699f6d Mon Sep 17 00:00:00 2001 From: snipe Date: Fri, 13 Oct 2023 12:18:58 +0100 Subject: [PATCH 2/6] Add the eol_explicit column earlier to accomodate the observer Signed-off-by: snipe --- .../2023_01_21_225350_add_eol_date_on_assets_table.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/database/migrations/2023_01_21_225350_add_eol_date_on_assets_table.php b/database/migrations/2023_01_21_225350_add_eol_date_on_assets_table.php index 9f5c5aa1e5..fbd4688da2 100644 --- a/database/migrations/2023_01_21_225350_add_eol_date_on_assets_table.php +++ b/database/migrations/2023_01_21_225350_add_eol_date_on_assets_table.php @@ -20,6 +20,14 @@ class AddEolDateOnAssetsTable extends Migration if (!Schema::hasColumn('assets', 'asset_eol_date')) { $table->date('asset_eol_date')->after('purchase_date')->nullable()->default(null); } + + // This is a temporary shim so we don't have to modify the asset observer for migrations where + // there is a large version difference. (See the AssetObserver notes). This column gets created + // later in 2023_07_13_052204_denormalized_eol_and_add_column_for_explicit_date_to_assets.php + // but we have to temporarily create it now so the save method below doesn't break + if (!Schema::hasColumn('assets', 'eol_explicit')) { + $table->boolean('eol_explicit')->default(false)->after('asset_eol_date'); + } }); // Chunk the model query to get the models that do have an EOL date From ea960c39bb5caa066495c71e071853cea161d94f Mon Sep 17 00:00:00 2001 From: snipe Date: Fri, 13 Oct 2023 12:19:32 +0100 Subject: [PATCH 3/6] Check if the eol_explicit column exists yet, add it if not Signed-off-by: snipe --- ...malized_eol_and_add_column_for_explicit_date_to_assets.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/database/migrations/2023_07_13_052204_denormalized_eol_and_add_column_for_explicit_date_to_assets.php b/database/migrations/2023_07_13_052204_denormalized_eol_and_add_column_for_explicit_date_to_assets.php index 8d3ca96829..982bd8ac0d 100644 --- a/database/migrations/2023_07_13_052204_denormalized_eol_and_add_column_for_explicit_date_to_assets.php +++ b/database/migrations/2023_07_13_052204_denormalized_eol_and_add_column_for_explicit_date_to_assets.php @@ -18,7 +18,9 @@ class DenormalizedEolAndAddColumnForExplicitDateToAssets extends Migration public function up() { Schema::table('assets', function (Blueprint $table) { - $table->boolean('eol_explicit')->default(false)->after('asset_eol_date'); + if (!Schema::hasColumn('assets', 'eol_explicit')) { + $table->boolean('eol_explicit')->default(false)->after('asset_eol_date'); + } }); From b2aed7feeac598100134db8b7edceea4a5c58b94 Mon Sep 17 00:00:00 2001 From: snipe Date: Fri, 13 Oct 2023 12:30:26 +0100 Subject: [PATCH 4/6] Removed temp column Signed-off-by: snipe --- .../2023_01_21_225350_add_eol_date_on_assets_table.php | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/database/migrations/2023_01_21_225350_add_eol_date_on_assets_table.php b/database/migrations/2023_01_21_225350_add_eol_date_on_assets_table.php index fbd4688da2..550f6f335b 100644 --- a/database/migrations/2023_01_21_225350_add_eol_date_on_assets_table.php +++ b/database/migrations/2023_01_21_225350_add_eol_date_on_assets_table.php @@ -20,14 +20,6 @@ class AddEolDateOnAssetsTable extends Migration if (!Schema::hasColumn('assets', 'asset_eol_date')) { $table->date('asset_eol_date')->after('purchase_date')->nullable()->default(null); } - - // This is a temporary shim so we don't have to modify the asset observer for migrations where - // there is a large version difference. (See the AssetObserver notes). This column gets created - // later in 2023_07_13_052204_denormalized_eol_and_add_column_for_explicit_date_to_assets.php - // but we have to temporarily create it now so the save method below doesn't break - if (!Schema::hasColumn('assets', 'eol_explicit')) { - $table->boolean('eol_explicit')->default(false)->after('asset_eol_date'); - } }); // Chunk the model query to get the models that do have an EOL date @@ -37,7 +29,7 @@ class AddEolDateOnAssetsTable extends Migration if ($asset->purchase_date!='') { $asset->asset_eol_date = $asset->present()->eol_date(); - $asset->save(); + $asset->saveQuietly(); } } From 2537d0fdaf3c6a8bbd848c16b028e53200d211ba Mon Sep 17 00:00:00 2001 From: snipe Date: Fri, 13 Oct 2023 12:34:46 +0100 Subject: [PATCH 5/6] Added comments Signed-off-by: snipe --- app/Observers/AssetObserver.php | 3 ++- .../2023_01_21_225350_add_eol_date_on_assets_table.php | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/Observers/AssetObserver.php b/app/Observers/AssetObserver.php index 55542ff2eb..c15c54a568 100644 --- a/app/Observers/AssetObserver.php +++ b/app/Observers/AssetObserver.php @@ -129,7 +129,8 @@ class AssetObserver * * For example, if there is a database migration that's a bit older and modifies an asset, if the save * fires before a field gets created in a later migration and that field in the later migration - * is used in this observer, it doesn't actually exist yet and the migration will break. + * is used in this observer, it doesn't actually exist yet and the migration will break unless we + * use saveQuietly() in the migration which skips this observer. * * @see https://github.com/snipe/snipe-it/issues/13723#issuecomment-1761315938 */ diff --git a/database/migrations/2023_01_21_225350_add_eol_date_on_assets_table.php b/database/migrations/2023_01_21_225350_add_eol_date_on_assets_table.php index 550f6f335b..d20d47d440 100644 --- a/database/migrations/2023_01_21_225350_add_eol_date_on_assets_table.php +++ b/database/migrations/2023_01_21_225350_add_eol_date_on_assets_table.php @@ -23,6 +23,8 @@ class AddEolDateOnAssetsTable extends Migration }); // Chunk the model query to get the models that do have an EOL date + // We use saveQuietly() here to skip the AssetObserver, since it modifies fields + // that do not yet exist on the assets table. AssetModel::whereNotNull('eol')->chunk(10, function ($models) { foreach ($models as $model) { foreach ($model->assets as $asset) { From ca1420c9bd326cbcaa8909417f8fda08fed467af Mon Sep 17 00:00:00 2001 From: snipe Date: Fri, 13 Oct 2023 12:39:28 +0100 Subject: [PATCH 6/6] Added the temp column back in Signed-off-by: snipe --- .../2023_01_21_225350_add_eol_date_on_assets_table.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/database/migrations/2023_01_21_225350_add_eol_date_on_assets_table.php b/database/migrations/2023_01_21_225350_add_eol_date_on_assets_table.php index d20d47d440..aa9086a7c7 100644 --- a/database/migrations/2023_01_21_225350_add_eol_date_on_assets_table.php +++ b/database/migrations/2023_01_21_225350_add_eol_date_on_assets_table.php @@ -17,9 +17,18 @@ class AddEolDateOnAssetsTable extends Migration { Schema::table('assets', function (Blueprint $table) { + if (!Schema::hasColumn('assets', 'asset_eol_date')) { $table->date('asset_eol_date')->after('purchase_date')->nullable()->default(null); } + + // This is a temporary shim so we don't have to modify the asset observer for migrations where + // there is a large version difference. (See the AssetObserver notes). This column gets created + // later in 2023_07_13_052204_denormalized_eol_and_add_column_for_explicit_date_to_assets.php + // but we have to temporarily create it now so the save method below doesn't break + if (!Schema::hasColumn('assets', 'eol_explicit')) { + $table->boolean('eol_explicit')->default(false)->after('asset_eol_date'); + } }); // Chunk the model query to get the models that do have an EOL date