Persist: index new entity first so circular references don't loop

src/Model/IdentityMap.php

@@ -27,6 +27,13 @@ class IdentityMap
 	 */
 	public array $uninitializedEntityList = [];
 
+	/**
+	 * entities currently being persisted (write side) - used to break circular references
+	 *
+	 * @var array<class-string, array<string, bool>>
+	 */
+	public array $persistingList = [];
+
 
 	public function add(
 		\Spameri\Elastic\Entity\AbstractElasticEntity $entity,
@@ -90,6 +97,42 @@ public function markInserted(
 	}
 
 
+	public function markPersisting(
+		\Spameri\Elastic\Entity\AbstractElasticEntity $entity,
+	): void
+	{
+		if ($entity->id instanceof \Spameri\Elastic\Entity\Property\EmptyElasticId) {
+			return;
+		}
+
+		$this->persistingList[$entity::class][$entity->id()->value()] = true;
+	}
+
+
+	public function unmarkPersisting(
+		\Spameri\Elastic\Entity\AbstractElasticEntity $entity,
+	): void
+	{
+		if ($entity->id instanceof \Spameri\Elastic\Entity\Property\EmptyElasticId) {
+			return;
+		}
+
+		unset($this->persistingList[$entity::class][$entity->id()->value()]);
+	}
+
+
+	public function isPersisting(
+		\Spameri\Elastic\Entity\AbstractElasticEntity $entity,
+	): bool
+	{
+		if ($entity->id instanceof \Spameri\Elastic\Entity\Property\EmptyElasticId) {
+			return false;
+		}
+
+		return isset($this->persistingList[$entity::class][$entity->id()->value()]);
+	}
+
+
 	public function isChanged(
 		\Spameri\Elastic\Entity\AbstractElasticEntity $entity,
 	): bool
@@ -128,6 +171,7 @@ public function clear(): void
 		$this->persisted = [];
 		$this->creatingEntityList = [];
 		$this->uninitializedEntityList = [];
+		$this->persistingList = [];
 	}
 
 }

src/Model/Insert.php

@@ -15,6 +15,17 @@ public function __construct(
 
 
 	/**
+	 * New entities are indexed in two phases so that nested entities can reference
+	 * the parent (and circular references do not loop):
+	 *
+	 *  1. the parent is indexed first (empty body, no refresh) to obtain its id and
+	 *     is marked as "persisting" in the identity map,
+	 *  2. its nested entities are persisted - a back-reference to the parent now
+	 *     resolves to the parent's id instead of re-persisting it,
+	 *  3. the parent is re-indexed with the full body.
+	 *
+	 * Entities that already have an id are indexed once, as before.
+	 *
 	 * @throws \Spameri\Elastic\Exception\ElasticSearch
 	 * @throws \Spameri\Elastic\Exception\DocumentInsertFailed
 	 */
@@ -31,14 +42,42 @@ public function execute(
 			return $entity->id()->value();
 		}
 
-		// Only mark inserted for entities with real IDs (prevents collision on empty string key)
-		if ($hasRealId) {
+		// New entity: index it first (shallow body) to obtain its id before its
+		// nested entities are persisted, so circular references can resolve to it.
+		if ($hasRealId === false) {
+			$this->index($entity, $index, $this->shallowBody($entity), false);
+		}
+
+		$this->identityMap->markInserted($entity);
+		$this->identityMap->markPersisting($entity);
+
+		try {
+			$entityArray = $this->prepareEntityArray->prepare($entity, $hasSti);
+			unset($entityArray['id']);
+
+			$id = $this->index($entity, $index, $entityArray, true);
 			$this->identityMap->markInserted($entity);
+
+			return $id;
+
+		} finally {
+			$this->identityMap->unmarkPersisting($entity);
 		}
+	}
 
-		$entityArray = $this->prepareEntityArray->prepare($entity, $hasSti);
-		unset($entityArray['id']);
 
+	/**
+	 * @param array<mixed> $entityArray
+	 * @throws \Spameri\Elastic\Exception\ElasticSearch
+	 * @throws \Spameri\Elastic\Exception\DocumentInsertFailed
+	 */
+	private function index(
+		\Spameri\Elastic\Entity\AbstractElasticEntity $entity,
+		string $index,
+		array $entityArray,
+		bool $refresh,
+	): string
+	{
 		try {
 			$response = $this->clientProvider->client()->index(
 				(
@@ -55,27 +94,52 @@ public function execute(
 			throw new \Spameri\Elastic\Exception\ElasticSearch($exception->getMessage());
 		}
 
-		try {
-			$this->clientProvider->client()->indices()->refresh(
-				(
-					new \Spameri\ElasticQuery\Document($index)
+		if ($refresh === true) {
+			try {
+				$this->clientProvider->client()->indices()->refresh(
+					(
+						new \Spameri\ElasticQuery\Document($index)
+					)
+						->toArray(),
 				)
-					->toArray(),
-			)
-			;
+				;
 
-		} catch (\Elastic\Elasticsearch\Exception\ElasticsearchException $exception) {
-			throw new \Spameri\Elastic\Exception\ElasticSearch($exception->getMessage());
+			} catch (\Elastic\Elasticsearch\Exception\ElasticsearchException $exception) {
+				throw new \Spameri\Elastic\Exception\ElasticSearch($exception->getMessage());
+			}
 		}
 
 		if (isset($response['result']) && ($response['result'] === 'created' || $response['result'] === 'updated')) {
 			$entity->id = new \Spameri\Elastic\Entity\Property\ElasticId($response['_id']);
-			$this->identityMap->markInserted($entity);
 
 			return $response['_id'];
 		}
 
 		throw new \Spameri\Elastic\Exception\DocumentInsertFailed();
 	}
 
+
+	/**
+	 * Minimal non-empty body for the first index of a new entity: every field
+	 * (except the id) nulled. The real values are written by the second index;
+	 * this only reserves the document so it gets a server generated id.
+	 *
+	 * @return array<string, null>
+	 */
+	private function shallowBody(
+		\Spameri\Elastic\Entity\AbstractElasticEntity $entity,
+	): array
+	{
+		$body = [];
+		foreach (\array_keys($entity->entityVariables()) as $key) {
+			if ($key === 'id') {
+				continue;
+			}
+
+			$body[$key] = null;
+		}
+
+		return $body;
+	}
+
 }

src/Model/Insert/PrepareEntityArray.php

@@ -111,7 +111,10 @@ public function iterateVariables(
 
 					$preparedArray[$key][self::ENTITY_CLASS] = $property::class;
 
-					if ($this->identityMap->isChanged($property) === false) {
+					if (
+						$this->identityMap->isPersisting($property) === true
+						|| $this->identityMap->isChanged($property) === false
+					) {
 						$preparedArray[$key][self::ENTITY_ID] = $property->id()->value();
 
 					} else {
@@ -127,7 +130,10 @@ public function iterateVariables(
 			}
 
 			if ($property instanceof \Spameri\Elastic\Entity\AbstractElasticEntity) {
-				if ($this->identityMap->isChanged($property) === false) {
+				if (
+					$this->identityMap->isPersisting($property) === true
+					|| $this->identityMap->isChanged($property) === false
+				) {
 					$preparedArray[$key] = $property->id()->value();
 
 				} else {
@@ -178,7 +184,10 @@ public function iterateVariables(
 				} else {
 					/** @var \Spameri\Elastic\Entity\AbstractElasticEntity $item */
 					foreach ($property as $item) {
-						if ($this->identityMap->isChanged($item) === false) {
+						if (
+							$this->identityMap->isPersisting($item) === true
+							|| $this->identityMap->isChanged($item) === false
+						) {
 							$preparedArray[$key][] = $item->id()->value();
 
 						} else {

tests/SpameriTests/Elastic/EntityManager/PersistCircularTest.phpt

@@ -0,0 +1,68 @@
+<?php declare(strict_types = 1);
+
+namespace SpameriTests\Elastic\EntityManager;
+
+require_once __DIR__ . '/../../../bootstrap.php';
+
+/**
+ * Persisting an entity whose nested elastic entities reference it back must not
+ * loop: the parent is indexed first (so it has an id), then the children are
+ * persisted referencing the parent, then the parent is re-indexed.
+ *
+ * @testCase
+ */
+class PersistCircularTest extends \SpameriTests\Elastic\AbstractTestCase
+{
+
+	public function testPersistCircularReference(): void
+	{
+		/** @var \Spameri\Elastic\EntityManager $entityManager */
+		$entityManager = $this->container->getByType(\Spameri\Elastic\EntityManager::class);
+
+		$title = new \SpameriTests\Elastic\Data\Entity\Title(
+			new \Spameri\Elastic\Entity\Property\EmptyElasticId(),
+			null,
+		);
+		$image = new \SpameriTests\Elastic\Data\Entity\Image(
+			new \Spameri\Elastic\Entity\Property\EmptyElasticId(),
+			$title,
+			true,
+		);
+		// circular reference: title -> image -> title
+		$title->backdrop = $image;
+
+		// must finish (no infinite loop) and assign both ids
+		\Tester\Assert::noError(
+			static function () use ($entityManager, $title): void {
+				$entityManager->persist($title);
+			},
+		);
+
+		\Tester\Assert::false($title->id() instanceof \Spameri\Elastic\Entity\Property\EmptyElasticId);
+		\Tester\Assert::false($image->id() instanceof \Spameri\Elastic\Entity\Property\EmptyElasticId);
+		\Tester\Assert::notSame('', $title->id()->value());
+		\Tester\Assert::notSame('', $image->id()->value());
+
+		// each side recorded the other's resolved id on the in-memory graph
+		\Tester\Assert::same($image->id()->value(), $title->backdrop->id()->value());
+		\Tester\Assert::same($title->id()->value(), $image->title->id()->value());
+
+		// read the stored documents and verify the references were persisted
+		$client = $this->container->getByType(\Spameri\Elastic\ClientProvider::class)->client();
+
+		$titleSource = $client->get([
+			'index' => \SpameriTests\Elastic\Config::INDEX_TITLE,
+			'id' => $title->id()->value(),
+		])->asArray()['_source'];
+		$imageSource = $client->get([
+			'index' => \SpameriTests\Elastic\Config::INDEX_IMAGE,
+			'id' => $image->id()->value(),
+		])->asArray()['_source'];
+
+		\Tester\Assert::same($image->id()->value(), $titleSource['backdrop']);
+		\Tester\Assert::same($title->id()->value(), $imageSource['title']);
+	}
+
+}
+
+(new PersistCircularTest())->run();