Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip non-mapped typed properties with default values when using Proxies #2399

Open
wants to merge 2 commits into
base: 2.5.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 39 additions & 0 deletions lib/Doctrine/ODM/MongoDB/Proxy/Factory/StaticProxyFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@
use Doctrine\Persistence\NotifyPropertyChanged;
use ProxyManager\Factory\LazyLoadingGhostFactory;
use ProxyManager\Proxy\GhostObjectInterface;
use ReflectionClass;
use ReflectionProperty;

use function array_filter;
use function array_merge;
use function count;

/**
Expand Down Expand Up @@ -130,6 +132,19 @@ private function createInitializer(
* @return array<int, string>
*/
private function skippedFieldsFqns(ClassMetadata $metadata): array
{
$skippedIdFieldsFqns = $this->getIdFieldsFqns($metadata);
$skippedNonMappedFieldsFqns = $this->getUnmappedPropertyFqns($metadata);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks more expensive than the original identifier exclusion and is called each and every time we create a proxy object, pretty much a hot path. We should memoize the result of skippedFieldsFqns per $metadata

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or put into metadata itself?


return array_merge($skippedIdFieldsFqns, $skippedNonMappedFieldsFqns);
}

/**
* @param ClassMetadata<object> $metadata
*
* @return array<int, string>
*/
private function getIdFieldsFqns(ClassMetadata $metadata): array
{
$idFieldFqcns = [];

Expand All @@ -140,6 +155,30 @@ private function skippedFieldsFqns(ClassMetadata $metadata): array
return $idFieldFqcns;
}

/**
* @param ClassMetadata<object> $metadata
*
* @return array<int, string>
*/
private function getUnmappedPropertyFqns(ClassMetadata $metadata): array
{
$nonMappedFieldFqcns = [];

$reflectionClass = new ReflectionClass($metadata->getName());

foreach ($reflectionClass->getProperties() as $property) {
$propertyName = $property->getName();

if ($metadata->hasField($propertyName)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note as field with different PHP and DB field always confused me: metadata is operating on PHP's object properties, and DB field name is used only in hydrators/persisters, right? If yes then we're A-OK here

continue;
}

$nonMappedFieldFqcns[] = $this->propertyFqcn($property);
}

return $nonMappedFieldFqcns;
}

private function propertyFqcn(ReflectionProperty $property): string
{
if ($property->isPrivate()) {
Expand Down
36 changes: 36 additions & 0 deletions tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2349Test.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?php

declare(strict_types=1);

namespace Doctrine\ODM\MongoDB\Tests\Functional\Ticket;

use Doctrine\ODM\MongoDB\Tests\BaseTestCase;
use Documents74\GH2349Customer;
use Documents74\GH2349Order;

/** @requires PHP 7.4 */
Copy link
Member

@malarzm malarzm Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm seeing correctly, 7.4 was already minimal version for ODM 2.5.x :) No harm done given we haven't cleaned this up yet, worth doing before 2.6.x :)

class GH2349Test extends BaseTestCase
{
public function testAccessingUnitializedProperties(): void
{
$customer = new GH2349Customer('Some Place');
$order = new GH2349Order($customer);

$this->dm->persist($customer);
$this->dm->persist($order);
$this->dm->flush();
$this->dm->clear();

$orderId = GH2349Order::ID;

$order = $this->dm->getRepository(GH2349Order::class)->find($orderId);

// Fetch a list of Customers from the DB. Any customer object that was referenced in the above order is still
// a proxy object, however it will not have the defaults set for the un-managed $domainEvents property.
$customers = $this->dm->getRepository(GH2349Customer::class)->findAll();

foreach ($customers as $customer) {
self::assertSame([], $customer->getEvents()); // This would trigger an error if unmapped properties are unset
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test seems fragile:

  • we don't check whether $customer is indeed a proxy
  • and is it still uninitialized (the test would still pass)

Maybe a db query count would be also good to assert so we never get into situation like the one found in original PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also i believe this would be better placed in a generic proxy test, where we don't even need database operations. Just creating a proxy and asserting whether unmapped stuff can be used without initialization - this should be easier to maintain in future

}
}
}
37 changes: 37 additions & 0 deletions tests/Documents74/GH2349Customer.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<?php

declare(strict_types=1);

namespace Documents74;

use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM;

/** @ODM\Document() */
class GH2349Customer
{
public const ID = '610419a277d50f7609139542';

/** @ODM\Id() */
private string $id;

/** @ODM\Field() */
private string $name;

private array $domainEvents = [];

public function __construct(string $name)
{
$this->id = self::ID;
$this->name = $name;
}

public function doSomeUpdate(): void
{
$this->domainEvents[] = 'a new event!';
}

public function getEvents(): array
{
return $this->domainEvents;
}
}
26 changes: 26 additions & 0 deletions tests/Documents74/GH2349Order.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php

declare(strict_types=1);

namespace Documents74;

use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM;
use MongoDB\BSON\ObjectId;

/** @ODM\Document() */
class GH2349Order
{
public const ID = '610419a277d50f7609139543';

/** @ODM\Id() */
private string $id;

/** @ODM\ReferenceOne(targetDocument=GH2349Customer::class, storeAs="ref") */
private GH2349Customer $customer;

public function __construct(GH2349Customer $customer)
{
$this->id = (string) new ObjectId(self::ID);
$this->customer = $customer;
}
}