Trim verbose comments, keep only essential 'why' explanations

Author: premtsd-code

src/Migration/Destinations/Appwrite.php

@@ -422,21 +422,13 @@ protected function createDatabase(Database $resource): bool
         $createdAt = $this->normalizeDateTime($resource->getCreatedAt());
         $updatedAt = $this->normalizeDateTime($resource->getUpdatedAt(), $createdAt);
 
-        // Skip/Upsert: pre-check an existing database. Databases contain the
-        // entire tree of collections + rows, so they are never destructively
-        // reconciled — under Upsert-newer the container metadata is updated
-        // in place (name, enabled, etc.) but children are untouched.
-        //
-        // Fail mode short-circuits: no pre-check, let the create below throw
-        // DuplicateException as designed. Saves N metadata reads on the
-        // common first-time-migration path.
+        // Fail mode skips the pre-check so library's DuplicateException surfaces on re-migration.
         if ($this->onDuplicate !== OnDuplicate::Fail) {
             $existing = $this->dbForProject->getDocument('databases', $resource->getId());
             $action = $this->onDuplicate->resolveSchemaAction(
                 !$existing->isEmpty(),
                 $updatedAt,
                 $existing->getUpdatedAt(),
-                // canDrop: false by default — databases hold child tables + rows.
             );
 
             if ($action === SchemaAction::Tolerate) {
@@ -457,7 +449,6 @@ protected function createDatabase(Database $resource): bool
                 $resource->setSequence($existing->getSequence());
                 return true;
             }
-            // SchemaAction::Create → fall through to the normal create flow.
         }
 
         $database = $this->dbForProject->createDocument('databases', new UtopiaDocument([
@@ -541,12 +532,6 @@ protected function createEntity(Table $resource): bool
             $dbForDatabases->create();
         }
 
-        // Skip/Upsert: pre-check an existing table. Like databases, tables
-        // contain user data (rows) so they are never destructively
-        // reconciled — under Upsert-newer the container metadata is
-        // updated in place (name, enabled, permissions, etc.) but child
-        // rows are untouched. Attribute/index reconciliation happens
-        // per-resource at a lower layer. Fail short-circuits.
         if ($this->onDuplicate !== OnDuplicate::Fail) {
             $existing = $this->dbForProject->getDocument(
                 'database_' . $database->getSequence(),
@@ -556,7 +541,6 @@ protected function createEntity(Table $resource): bool
                 !$existing->isEmpty(),
                 $updatedAt,
                 $existing->getUpdatedAt(),
-                // canDrop: false by default — tables hold child rows.
             );
 
             if ($action === SchemaAction::Tolerate) {
@@ -580,7 +564,6 @@ protected function createEntity(Table $resource): bool
                 $resource->setSequence($existing->getSequence());
                 return true;
             }
-            // SchemaAction::Create → fall through to the normal create flow.
         }
 
         $table = $this->dbForProject->createDocument('database_' . $database->getSequence(), new UtopiaDocument([
@@ -723,17 +706,14 @@ protected function createField(Column|Attribute $resource): bool
         $updatedAt = $this->normalizeDateTime($resource->getUpdatedAt(), $createdAt);
         $dbForDatabases = ($this->getDatabasesDB)($database);
 
-        // Skip/Upsert: pre-check against `attributes` metadata via one
-        // resolveSchemaAction call. Fail mode short-circuits to preserve
-        // zero-overhead fresh-migration behavior.
         $attributeMetaId = $database->getSequence() . '_' . $table->getSequence() . '_' . $resource->getKey();
         if ($this->onDuplicate !== OnDuplicate::Fail) {
             $existingAttr = $this->dbForProject->getDocument('attributes', $attributeMetaId);
             $action = $this->onDuplicate->resolveSchemaAction(
                 !$existingAttr->isEmpty(),
                 $updatedAt,
                 $existingAttr->getUpdatedAt(),
-                canDrop: true,  // attributes are leaves; column data repopulates via row Upsert
+                canDrop: true,
             );
 
             if ($action === SchemaAction::Tolerate) {
@@ -752,7 +732,6 @@ protected function createField(Column|Attribute $resource): bool
                     $dbForDatabases,
                 );
             }
-            // SchemaAction::Create → fall through to the normal create flow.
         }
 
         try {
@@ -1033,18 +1012,14 @@ protected function createIndex(Index $resource): bool
             );
         }
 
-        // Skip/Upsert: pre-check against `indexes` metadata via one
-        // resolveSchemaAction call. Same rule as attributes, except that
-        // index drops are non-destructive (no row data lives in indexes) so
-        // rebuild cost is just the index build time. Fail short-circuits.
         $indexMetaId = $database->getSequence() . '_' . $table->getSequence() . '_' . $resource->getKey();
         if ($this->onDuplicate !== OnDuplicate::Fail) {
             $existingIdx = $this->dbForProject->getDocument('indexes', $indexMetaId);
             $action = $this->onDuplicate->resolveSchemaAction(
                 !$existingIdx->isEmpty(),
                 $updatedAt,
                 $existingIdx->getUpdatedAt(),
-                canDrop: true,  // indexes are leaves; carry no user data
+                canDrop: true,
             );
 
             if ($action === SchemaAction::Tolerate) {
@@ -1066,7 +1041,6 @@ protected function createIndex(Index $resource): bool
                     $table->getId()
                 );
             }
-            // SchemaAction::Create → fall through to the normal create flow.
         }
 
         $index = $this->dbForProject->createDocument('indexes', $index);
@@ -1244,26 +1218,11 @@ protected function createRecord(Row $resource, bool $isLast): bool
     }
 
     /**
-     * Fully remove an attribute from destination — both metadata documents
-     * and the physical column(s) — so the caller can create it fresh without
-     * colliding with any remnants.
+     * Drop an attribute (metadata doc + physical column) so it can be recreated.
      *
-     * For plain attributes this is a parent-side cleanup. For two-way
-     * relationship attributes, createField writes a second `attributes`
-     * document on the related table (under {dbSeq}_{relatedTableSeq}_{twoWayKey})
-     * in addition to the parent-side document. Dropping only the parent
-     * side leaves the child document dangling and a subsequent recreate
-     * collides on it with DuplicateException. This method mirrors
-     * createField's two-doc write so the "reconcile" path is symmetric.
-     *
-     * Physical cleanup on the related table is best-effort: the library's
-     * deleteAttribute on the parent side can cascade to the child column
-     * depending on relationship handling, so the second deleteAttribute may
-     * no-op or NotFoundException — both swallowed.
-     *
-     * @param UtopiaDocument|null $relatedTable null when the attribute isn't
-     *                                          a two-way relationship;
-     *                                          required otherwise.
+     * Two-way relationships require mirroring: createField writes a second
+     * `attributes` document on the related table keyed by twoWayKey. Dropping
+     * only the parent leaves that child doc dangling and the recreate collides.
      */
     private function deleteAttributeCompletely(
         UtopiaDocument $database,
@@ -1276,15 +1235,11 @@ private function deleteAttributeCompletely(
         $collectionId = 'database_' . $database->getSequence() . '_collection_' . $table->getSequence();
         $attributeMetaId = $database->getSequence() . '_' . $table->getSequence() . '_' . $resource->getKey();
 
-        // Parent side.
         $dbForDatabases->deleteAttribute($collectionId, $resource->getKey());
         $this->dbForProject->deleteDocument('attributes', $attributeMetaId);
         $this->dbForProject->purgeCachedDocument('database_' . $database->getSequence(), $table->getId());
         $dbForDatabases->purgeCachedCollection($collectionId);
 
-        // Child side, only for two-way relationships. createField writes a
-        // second `attributes` document keyed on the related table; mirror
-        // that write here.
         if ($type !== UtopiaDatabase::VAR_RELATIONSHIP || $relatedTable === null) {
             return;
         }
@@ -1303,13 +1258,12 @@ private function deleteAttributeCompletely(
         try {
             $this->dbForProject->deleteDocument('attributes', $childMetaId);
         } catch (\Throwable) {
-            // Child metadata already gone — interrupted prior run.
+            // already gone
         }
         try {
             $dbForDatabases->deleteAttribute($childCollectionId, $twoWayKey);
         } catch (\Throwable) {
-            // Physical column already gone — parent-side deleteAttribute may
-            // have cascaded via utopia-php/database relationship handling.
+            // parent-side delete may have cascaded
         }
         $this->dbForProject->purgeCachedDocument('database_' . $database->getSequence(), $relatedTable->getId());
         $dbForDatabases->purgeCachedCollection($childCollectionId);

src/Migration/Destinations/OnDuplicate.php

@@ -3,39 +3,16 @@
 namespace Utopia\Migration\Destinations;
 
 /**
- * Caller branches on one of these outcomes after asking
- * {@see OnDuplicate::resolveSchemaAction()} what to do with a possibly
- * pre-existing schema resource on the destination.
+ * Outcome of {@see OnDuplicate::resolveSchemaAction()}.
  */
 enum SchemaAction
 {
-    /** Resource doesn't exist — run the normal create flow. */
     case Create;
-
-    /** Resource exists; leave it alone (Skip, or Upsert with dest up-to-date). */
     case Tolerate;
-
-    /**
-     * Resource exists; Upsert mode + source strictly newer + resource is a
-     * leaf (attribute/index) — drop + recreate. Data-preserving containers
-     * get {@see self::UpdateInPlace} instead.
-     */
     case DropAndRecreate;
-
-    /**
-     * Resource exists; Upsert mode + source strictly newer + resource is a
-     * container (database/table) — update metadata in place without
-     * touching children. Callers should only overwrite fields that are
-     * safe to source-wins (name, enabled, search, permissions, etc.) and
-     * must never touch immutable fields ($id, $createdAt, internal
-     * sequences).
-     */
     case UpdateInPlace;
 }
 
-/**
- * Behavior when a destination row with an existing ID is encountered.
- */
 enum OnDuplicate: string
 {
     case Fail = 'fail';
@@ -51,22 +28,11 @@ public static function values(): array
     }
 
     /**
-     * Single decision point for schema-level reconciliation on re-migration.
-     *
-     * Callers typically short-circuit on Fail mode before invoking this (to
-     * avoid the destination metadata lookup entirely — the library's own
-     * DuplicateException surfaces from the create call as designed).
+     * Schema-level reconciliation decision.
      *
-     * $canDrop picks the Upsert-newer reconciliation strategy; default
-     * false is the safe option — destructive reconciliation requires
-     * explicit opt-in at the call site:
-     *   - true  → DropAndRecreate (attributes, indexes; column data is
-     *             repopulated by the follow-up row Upsert, or is pure
-     *             metadata for indexes).
-     *   - false → UpdateInPlace (databases, tables; their child rows and
-     *             sub-resources must be preserved, so destructive
-     *             reconciliation is replaced with an updateDocument on the
-     *             container's own metadata document).
+     * $canDrop = true  → leaves (attributes, indexes) get DropAndRecreate on Upsert-newer.
+     * $canDrop = false → containers (databases, tables) get UpdateInPlace on Upsert-newer.
+     * Default is false so destructive reconciliation requires explicit opt-in.
      */
     public function resolveSchemaAction(
         bool $exists,
@@ -78,7 +44,7 @@ public function resolveSchemaAction(
             return SchemaAction::Create;
         }
         return match ($this) {
-            self::Fail   => SchemaAction::Create,   // caller's create flow will throw
+            self::Fail   => SchemaAction::Create,
             self::Skip   => SchemaAction::Tolerate,
             self::Upsert => $this->sourceIsNewer($sourceUpdatedAt, $destUpdatedAt)
                 ? ($canDrop ? SchemaAction::DropAndRecreate : SchemaAction::UpdateInPlace)
@@ -87,13 +53,8 @@ public function resolveSchemaAction(
     }
 
     /**
-     * Unparseable or clearly-invalid timestamps → false (conservative:
-     * preserve the existing destination rather than risk a destructive drop
-     * on garbage input). PHP's strtotime() accepts some malformed dates
-     * leniently — '0000-00-00 00:00:00' for example parses to a large
-     * negative epoch rather than returning false — so we also reject
-     * non-positive epochs. Any legitimate Appwrite updatedAt is well past
-     * 1970-01-01.
+     * strtotime() accepts '0000-00-00' leniently (returns a large negative
+     * epoch, not false), so reject non-positive epochs too.
      */
     private function sourceIsNewer(string $source, string $dest): bool
     {