Merge pull request #717 from nextras/fix-one-has-one

Fix OneHasOne relationship when loading from non-main side

src/Entity/IProperty.php

@@ -25,9 +25,10 @@ public function convertToRawValue($value);
 
 
 	/**
-	 * Sets raw value from storage.
-	 * Calling this method directly is not recommended.
+	 * Sets a fetched raw value from a storage.
+	 * Calling this method directly may BREAK things.
 	 * Implementation must not require entity instance.
+	 * This method is not symmetric to {@see getRawValue()}.
 	 * @param mixed $value
 	 * @internal
 	 */
@@ -37,6 +38,7 @@ public function setRawValue($value): void;
 	/**
 	 * Returns raw value.
 	 * Raw value is a normalized value which is suitable for storing.
+	 * This method is not symmetric to {@see setRawValue()}.
 	 * @return mixed
 	 */
 	public function getRawValue();

src/Mapper/Memory/RelationshipMapperManyHasOne.php

@@ -15,7 +15,8 @@
 class RelationshipMapperManyHasOne implements IRelationshipMapper
 {
 	public function __construct(
-		protected readonly PropertyMetadata $metadata,)
+		protected readonly PropertyMetadata $metadata,
+	)
 	{
 	}
 

src/Relationships/HasMany.php

@@ -170,7 +170,7 @@ public function remove($entity): ?IEntity
 			return null;
 		}
 
-		$entity = $this->createEntity($entity);
+		$entity = $this->createEntity($entity, attach: false);
 		if ($entity === null) {
 			return null;
 		}
@@ -193,7 +193,7 @@ public function remove($entity): ?IEntity
 
 	public function has($entity): bool
 	{
-		$entity = $this->createEntity($entity, false);
+		$entity = $this->createEntity($entity, need: false, attach: false);
 		if ($entity === null) {
 			return false;
 		}
@@ -332,16 +332,18 @@ function (): array {
 	 * @param E|string|int $entity
 	 * @return E|null
 	 */
-	protected function createEntity($entity, bool $need = true): ?IEntity
+	protected function createEntity($entity, bool $need = true, bool $attach = true): ?IEntity
 	{
 		if ($entity instanceof IEntity) {
-			if ($entity->isAttached()) {
-				$repository = $entity->getRepository()->getModel()->getRepositoryForEntity($this->parent);
-				$repository->attach($this->parent);
-
-			} elseif ($this->parent->isAttached()) {
-				$repository = $this->parent->getRepository()->getModel()->getRepositoryForEntity($entity);
-				$repository->attach($entity);
+			if ($attach) {
+				if ($entity->isAttached()) {
+					$repository = $entity->getRepository()->getModel()->getRepositoryForEntity($this->parent);
+					$repository->attach($this->parent);
+
+				} elseif ($this->parent->isAttached()) {
+					$repository = $this->parent->getRepository()->getModel()->getRepositoryForEntity($entity);
+					$repository->attach($entity);
+				}
 			}
 
 			return $entity;

src/Relationships/HasOne.php

@@ -30,14 +30,26 @@ abstract class HasOne implements IRelationshipContainer
 	/** @var ICollection<E>|null */
 	protected ?ICollection $collection = null;
 
-	/** Is value validated against storage? */
+	/**
+	 * Denotes if the value is validated, i.e. if the value is pk of valid entity or if the value is not null when it is
+	 * disallowed.
+	 *
+	 * By default, relationship is validated because no initial value has been set yet.
+	 * The first setRawValue will change that to false (with exception on null, which won't be validated later).
+	 */
 	protected bool $isValueValidated = true;
 
-	/** Is raw value loaded from storage and not converted yet? */
-	protected bool $isValueFromStorage = false;
+	/**
+	 * Denotes if the value is present. Value is not present when this relationship side
+	 * is not the main one and the reverse side was not yet asked to get the initial value.
+	 * After setting this value in runtime, the value is always present.
+	 *
+	 * If value is not present and is worked with, it is fetched via {@see fetchValue()}.
+	 */
+	protected bool $isValuePresent = true;
 
 	/** @var E|string|int|null */
-	protected $value = null;
+	protected mixed $value = null;
 
 	/** @var list<E> */
 	protected array $tracked = [];
@@ -88,10 +100,9 @@ public function convertToRawValue($value)
 
 	public function setRawValue($value): void
 	{
-		$isChanged = $this->getPrimaryValue() !== $value;
 		$this->value = $value;
-		$this->isValueValidated = !$isChanged && $value === null;
-		$this->isValueFromStorage = true;
+		$this->isValueValidated = $value === null;
+		$this->isValuePresent = true;
 	}
 
 
@@ -122,7 +133,7 @@ public function hasInjectedValue(): bool
 
 	public function isLoaded(): bool
 	{
-		return !$this->isValueFromStorage || $this->isValueValidated;
+		return $this->value instanceof IEntity;
 	}
 
 
@@ -141,19 +152,17 @@ public function set($value, bool $allowNull = false): bool
 			return false;
 		}
 
-		if (($this->parent !== null && $this->parent->isAttached()) || $value === null) {
-			$entity = $this->createEntity($value, $allowNull);
-			$isValueValidated = true;
+		if ($this->parent?->isAttached() === true || $value === null) {
+			$entity = $this->createEntity($value, allowNull: $allowNull);
 		} else {
 			$entity = $value;
-			$isValueValidated = false;
 		}
 
 		if ($entity instanceof IEntity || $entity === null) {
 			$isChanged = $this->isChanged($entity);
 			if ($isChanged) {
 				$this->modify();
-				$oldEntity = $this->getValue(false);
+				$oldEntity = $this->getValue(allowPreloadContainer: false);
 				if ($oldEntity !== null) {
 					$this->tracked[] = $oldEntity;
 				}
@@ -167,8 +176,8 @@ public function set($value, bool $allowNull = false): bool
 		}
 
 		$this->value = $entity;
-		$this->isValueValidated = $isValueValidated;
-		$this->isValueFromStorage = false;
+		$this->isValueValidated = $entity === null || $entity instanceof IEntity;
+		$this->isValuePresent = true;
 		return $isChanged;
 	}
 
@@ -214,7 +223,7 @@ protected function getPrimaryValue(): mixed
 	 */
 	protected function getValue(bool $allowPreloadContainer = true): ?IEntity
 	{
-		if (!$this->isValueValidated && ($this->value !== null || $this->metadata->isNullable)) {
+		if ((!$this->isValueValidated && ($this->value !== null || $this->metadata->isNullable)) || !$this->isValuePresent) {
 			$this->initValue($allowPreloadContainer);
 		}
 
@@ -229,9 +238,9 @@ protected function initValue(bool $allowPreloadContainer = true): void
 			throw new InvalidStateException('Relationship is not attached to a parent entity.');
 		}
 
-		if ($this->isValueFromStorage && $allowPreloadContainer) {
-			// load the value using relationship mapper to utilize preload container and not to validate if
-			// relationship's entity is really present in the database;
+		if (!$this->isValuePresent || $allowPreloadContainer) {
+			// load the value using relationship mapper to utilize preload container to avoid validation if the
+			// relationship's entity is actually present in the database;
 			$this->set($this->fetchValue());
 
 		} else {
@@ -273,11 +282,8 @@ protected function getTargetRepository(): IRepository
 	 */
 	protected function getCollection(): ICollection
 	{
-		if ($this->collection !== null) {
-			return $this->collection;
-		}
-
-		return $this->collection = $this->createCollection();
+		$this->collection ??= $this->createCollection();
+		return $this->collection;
 	}
 
 
@@ -342,6 +348,10 @@ protected function isChanged(?IEntity $newValue): bool
 			// newValue is null
 			return true;
 
+		} else if (!$this->isValuePresent) {
+			// before initial load, we cannot detect changes
+			return false;
+
 		} elseif ($newValue instanceof IEntity && $newValue->isPersisted()) {
 			// value is persisted entity or null
 			// newValue is persisted entity

src/Relationships/IRelationshipContainer.php

@@ -21,7 +21,8 @@ public function getEntity(): ?IEntity;
 
 
 	/**
-	 * Returns true if container was loaded.
+	 * Returns true if container was loaded, i.e. the relationship contains an entity in contrast to its primary
+	 * key only.
 	 */
 	public function isLoaded(): bool;
 

src/Relationships/ManyHasOne.php

@@ -52,15 +52,15 @@ protected function updateRelationship(?IEntity $oldEntity, ?IEntity $newEntity,
 	}
 
 
-	protected function initReverseRelationship(?IEntity $entity): void
+	protected function initReverseRelationship(?IEntity $currentEntity): void
 	{
 		$key = $this->metadataRelationship->property;
-		if ($key === null || $entity === null) {
+		if ($key === null || $currentEntity === null) {
 			return;
 		}
 
 		$this->updatingReverseRelationship = true;
-		$property = $entity->getProperty($key);
+		$property = $currentEntity->getProperty($key);
 		assert($property instanceof OneHasMany);
 		$property->trackEntity($this->getParentEntity());
 		$this->updatingReverseRelationship = false;

src/Relationships/OneHasOne.php

@@ -18,7 +18,7 @@ class OneHasOne extends HasOne
 	public function __construct(PropertyMetadata $metadata)
 	{
 		parent::__construct($metadata);
-		$this->isValueFromStorage = !$this->metadataRelationship->isMain;
+		$this->isValuePresent = $this->metadataRelationship->isMain;
 	}
 
 
@@ -34,25 +34,21 @@ public function setRawValue($value): void
 	{
 		parent::setRawValue($value);
 		if (!$this->metadataRelationship->isMain) {
-			$this->isValueValidated = false;
+			$this->isValuePresent = $value !== null;
 		}
 	}
 
 
 	public function getRawValue()
 	{
-		if ($this->isValueFromStorage && !$this->metadataRelationship->isMain) {
-			$this->initValue();
-		}
+		if (!$this->isValuePresent) $this->initValue();
 		return parent::getRawValue();
 	}
 
 
 	public function hasInjectedValue(): bool
 	{
-		if ($this->isValueFromStorage && !$this->metadataRelationship->isMain) {
-			$this->initValue();
-		}
+		if (!$this->isValuePresent) $this->initValue();
 		return parent::hasInjectedValue();
 	}
 
@@ -88,15 +84,15 @@ protected function updateRelationship(?IEntity $oldEntity, ?IEntity $newEntity,
 	}
 
 
-	protected function initReverseRelationship(?IEntity $entity): void
+	protected function initReverseRelationship(?IEntity $currentEntity): void
 	{
 		$key = $this->metadataRelationship->property;
-		if ($key === null || $entity === null) {
+		if ($key === null || $currentEntity === null) {
 			return;
 		}
 
 		$this->updatingReverseRelationship = true;
-		$property = $entity->getProperty($key);
+		$property = $currentEntity->getProperty($key);
 		assert($property instanceof OneHasOne);
 		$property->set($this->parent);
 		$this->updatingReverseRelationship = false;

tests/cases/integration/Relationships/relationships.oneHasMany.has().phpt

@@ -16,7 +16,6 @@ require_once __DIR__ . '/../../../bootstrap.php';
 
 class RelationshipsOneHasManyHasTest extends DataTestCase
 {
-
 	public function testHasValue(): void
 	{
 		$author = $this->orm->authors->getByIdChecked(1);
@@ -27,8 +26,8 @@ class RelationshipsOneHasManyHasTest extends DataTestCase
 		Assert::true($author->books->has($book));
 		$this->orm->books->remove($book);
 		Assert::false($author->books->has($book));
+		Assert::false($book->isAttached());
 	}
-
 }
 
 

tests/cases/integration/Relationships/relationships.oneHasOne.phpt

@@ -46,6 +46,27 @@ class RelationshipOneHasOneTest extends DataTestCase
 	}
 
 
+	public function testAccessFromNonMainSide(): void
+	{
+		$book = new Book();
+		$book->author = $this->orm->authors->getByIdChecked(1);
+		$book->title = 'GoT';
+		$book->publisher = 1;
+
+		$ean = new Ean();
+		$ean->code = 'GoTEAN';
+		$ean->book = $book;
+
+		$this->orm->books->persistAndFlush($book);
+		$eanId = $ean->id;
+
+		$this->orm->clear();
+
+		$ean = $this->orm->eans->getByIdChecked($eanId);
+		Assert::same('GoT', $ean->book->title);
+	}
+
+
 	public function testReadOnNullable(): void
 	{
 		$album = new PhotoAlbum();
@@ -265,6 +286,7 @@ class RelationshipOneHasOneTest extends DataTestCase
 		$ean = new Ean();
 		$ean->code = '1234';
 		$ean->book = $this->orm->books->getByIdChecked(1);
+		Assert::true($ean->isAttached());
 		$this->orm->eans->persistAndFlush($ean);
 		$this->orm->clear();
 

tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionTest_testConditionsInDifferentJoinsAndSameTable.sql

@@ -1,6 +1,6 @@
-SELECT "authors".* FROM "public"."authors" AS "authors" WHERE "authors"."id" = 1;
-SELECT "authors".* FROM "public"."authors" AS "authors" WHERE "authors"."id" = 2;
-SELECT "publishers".* FROM "publishers" AS "publishers" WHERE "publishers"."publisher_id" = 1;
+SELECT "authors".* FROM "public"."authors" AS "authors" WHERE "authors"."id" IN (1);
+SELECT "authors".* FROM "public"."authors" AS "authors" WHERE "authors"."id" IN (2);
+SELECT "publishers".* FROM "publishers" AS "publishers" WHERE "publishers"."publisher_id" IN (1);
 START TRANSACTION;
 INSERT INTO "books" ("title", "author_id", "translator_id", "next_part", "ean_id", "publisher_id", "genre", "published_at", "printed_at", "thread_id", "price", "price_currency", "orig_price_cents", "orig_price_currency") VALUES ('Books 5', 1, 2, NULL, NULL, 1, 'fantasy', '2021-12-31 23:59:59.000000'::timestamp, NULL, NULL, NULL, NULL, NULL, NULL);
 SELECT CURRVAL('public.books_id_seq');

tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionTest_testJoinDifferentPath.sql

@@ -4,9 +4,9 @@ INSERT INTO "eans" ("code", "type") VALUES ('123', 2);
 SELECT CURRVAL('public.eans_id_seq');
 UPDATE "books" SET "ean_id" = 1 WHERE "id" = 3;
 COMMIT;
-SELECT "authors".* FROM "public"."authors" AS "authors" WHERE "authors"."id" = 1;
-SELECT "publishers".* FROM "publishers" AS "publishers" WHERE "publishers"."publisher_id" = 1;
-SELECT "books".* FROM "books" AS "books" WHERE "books"."id" = 4;
+SELECT "authors".* FROM "public"."authors" AS "authors" WHERE "authors"."id" IN (1);
+SELECT "publishers".* FROM "publishers" AS "publishers" WHERE "publishers"."publisher_id" IN (1);
+SELECT "books".* FROM "books" AS "books" WHERE "books"."id" IN (4);
 START TRANSACTION;
 INSERT INTO "eans" ("code", "type") VALUES ('456', 2);
 SELECT CURRVAL('public.eans_id_seq');

tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipManyHasOneTest_testPersistenceHasOne.sql

@@ -1,4 +1,4 @@
-SELECT "publishers".* FROM "publishers" AS "publishers" WHERE "publishers"."publisher_id" = 1;
+SELECT "publishers".* FROM "publishers" AS "publishers" WHERE "publishers"."publisher_id" IN (1);
 START TRANSACTION;
 INSERT INTO "public"."authors" ("name", "born", "web", "favorite_author_id") VALUES ('Jon Snow', '2021-03-21 08:23:00.000000'::timestamp, 'http://www.example.com', NULL);
 SELECT CURRVAL('public.authors_id_seq');

tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipOneHasManyCompositePkTest_testLimit.sql

@@ -1,5 +1,5 @@
-SELECT "tags".* FROM "tags" AS "tags" WHERE "tags"."id" = 2;
-SELECT "authors".* FROM "public"."authors" AS "authors" WHERE "authors"."id" = 1;
+SELECT "tags".* FROM "tags" AS "tags" WHERE "tags"."id" IN (2);
+SELECT "authors".* FROM "public"."authors" AS "authors" WHERE "authors"."id" IN (1);
 START TRANSACTION;
 INSERT INTO "tag_followers" ("created_at", "author_id", "tag_id") VALUES ('2021-12-02 19:21:00.000000'::timestamptz, 1, 2);
 COMMIT;

tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipOneHasManyRemoveTest_testRemoveCollection.sql

@@ -1,4 +1,4 @@
-SELECT "publishers".* FROM "publishers" AS "publishers" WHERE "publishers"."publisher_id" = 1;
+SELECT "publishers".* FROM "publishers" AS "publishers" WHERE "publishers"."publisher_id" IN (1);
 START TRANSACTION;
 INSERT INTO "public"."authors" ("name", "born", "web", "favorite_author_id") VALUES ('A', '2021-03-21 08:23:00.000000'::timestamp, 'http://www.example.com', NULL);
 SELECT CURRVAL('public.authors_id_seq');

tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipOneHasManyRemoveTest_testRemoveCollectionAndParent.sql

@@ -1,4 +1,4 @@
-SELECT "publishers".* FROM "publishers" AS "publishers" WHERE "publishers"."publisher_id" = 1;
+SELECT "publishers".* FROM "publishers" AS "publishers" WHERE "publishers"."publisher_id" IN (1);
 START TRANSACTION;
 INSERT INTO "public"."authors" ("name", "born", "web", "favorite_author_id") VALUES ('A', '2021-03-21 08:23:00.000000'::timestamp, 'http://www.example.com', NULL);
 SELECT CURRVAL('public.authors_id_seq');