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.

Sign up for GitHub

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

PHPORM-255 Enable disabling the id to _id field rename in embedded documents #3332

Merged
merged 4 commits into from
Apr 9, 2025
+159 −19
Merged

PHPORM-255 Enable disabling the id to _id field rename in embedded documents #3332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3b4ad5a
PHPORM-255 Add a method to disable renaming the id to _id field in em…
GromNaN Apr 1, 2025
f780906
Use $connection property
GromNaN Apr 2, 2025
48c14cc
Add the option "rename_embedded_id_field" in database connection config
GromNaN Apr 7, 2025
68e57f4
Fix aliasing _id in embedded documents + add test
GromNaN Apr 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/Connection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ class Connection extends BaseConnection

private ?CommandSubscriber $commandSubscriber = null;

/** @var bool Whether to rename the rename "id" into "_id" for embedded documents. */
private bool $renameEmbeddedIdField;

/**
* Create a new database connection instance.
*/
Expand Down Expand Up @@ -80,6 +83,8 @@ public function __construct(array $config)
$this->useDefaultSchemaGrammar();

$this->useDefaultQueryGrammar();

$this->renameEmbeddedIdField = $config['rename_embedded_id_field'] ?? true;
}

/**
Expand Down Expand Up @@ -395,6 +400,18 @@ public function __call($method, $parameters)
return $this->db->$method(...$parameters);
}

/** Set whether to rename "id" field into "_id" for embedded documents. */
public function setRenameEmbeddedIdField(bool $rename): void
{
$this->renameEmbeddedIdField = $rename;
}

/** Get whether to rename "id" field into "_id" for embedded documents. */
public function getRenameEmbeddedIdField(): bool
{
return $this->renameEmbeddedIdField;
}

/**
* Return the server version of one of the MongoDB servers: primary for
* replica sets and standalone, and the selected server for sharded clusters.
Expand Down
3 changes: 2 additions & 1 deletion src/Eloquent/Builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use MongoDB\Model\BSONDocument;

use function array_key_exists;
use function array_map;
use function array_replace;
use function collect;
use function is_array;
Expand Down Expand Up @@ -237,7 +238,7 @@ public function raw($value = null)
// Convert MongoCursor results to a collection of models.
if ($results instanceof CursorInterface) {
$results->setTypeMap(['root' => 'array', 'document' => 'array', 'array' => 'array']);
$results = $this->query->aliasIdForResult(iterator_to_array($results));
$results = array_map(fn ($document) => $this->query->aliasIdForResult($document), iterator_to_array($results));

return $this->model->hydrate($results);
}
Expand Down
26 changes: 17 additions & 9 deletions src/Query/Builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
use MongoDB\Builder\Type\SearchOperatorInterface;
use MongoDB\Driver\Cursor;
use MongoDB\Driver\ReadPreference;
use MongoDB\Laravel\Connection;
use Override;
use RuntimeException;
use stdClass;
Expand Down Expand Up @@ -83,6 +84,7 @@
use function trait_exists;
use function var_export;

/** @property Connection $connection */
class Builder extends BaseBuilder
{
private const REGEX_DELIMITERS = ['/', '#', '~'];
Expand Down Expand Up @@ -1764,9 +1766,9 @@ public function orWhereIntegerNotInRaw($column, $values, $boolean = 'and')
throw new BadMethodCallException('This method is not supported by MongoDB');
}

private function aliasIdForQuery(array $values): array
private function aliasIdForQuery(array $values, bool $root = true): array
{
if (array_key_exists('id', $values)) {
if (array_key_exists('id', $values) && ($root || $this->connection->getRenameEmbeddedIdField())) {
if (array_key_exists('_id', $values) && $values['id'] !== $values['_id']) {
throw new InvalidArgumentException('Cannot have both "id" and "_id" fields.');
}
Expand All @@ -1793,7 +1795,7 @@ private function aliasIdForQuery(array $values): array
}

// ".id" subfield are alias for "._id"
if (str_ends_with($key, '.id')) {
if (str_ends_with($key, '.id') && ($root || $this->connection->getRenameEmbeddedIdField())) {
$newkey = substr($key, 0, -3) . '._id';
if (array_key_exists($newkey, $values) && $value !== $values[$newkey]) {
throw new InvalidArgumentException(sprintf('Cannot have both "%s" and "%s" fields.', $key, $newkey));
Expand All @@ -1806,7 +1808,7 @@ private function aliasIdForQuery(array $values): array

foreach ($values as &$value) {
if (is_array($value)) {
$value = $this->aliasIdForQuery($value);
$value = $this->aliasIdForQuery($value, false);
Copy link
Member

Choose a reason for hiding this comment

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

Note that this appears to be the only recursive call to aliasIdForQuery(). All other calls (e.g. preparing projections, wheres) are invoked at the root level.

} elseif ($value instanceof DateTimeInterface) {
$value = new UTCDateTime($value);
}
Expand All @@ -1824,10 +1826,13 @@ private function aliasIdForQuery(array $values): array
*
* @template T of array|object
*/
public function aliasIdForResult(array|object $values): array|object
public function aliasIdForResult(array|object $values, bool $root = true): array|object
Copy link
Member

Choose a reason for hiding this comment

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

There appear to be two calls to aliasIdForResult() from MongoDB\Laravel\Eloquent\Builder::raw() (see: search results).

The context suggests that you're only processing a single document, but in one case you call iterator_to_array() on a cursor (which suggests it'd be handling multiple results instead of just one).

Neither of the aliasIdForResult() calls there seem affected by this change, since $root: false is only used when recursing, but it'd be worth confirming.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, that's fixed by using array_map.

Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm, this fix was necessary to get the test case for multiple results passing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously, the function was called with $root = true on the list of document, and then each document root had $root = false because the function was called recursively. Using array_map, I'm calling aliasIdForResult on the documents directly, instead of the collection of documents.

{
if (is_array($values)) {
if (array_key_exists('_id', $values) && ! array_key_exists('id', $values)) {
if (
array_key_exists('_id', $values) && ! array_key_exists('id', $values)
&& ($root || $this->connection->getRenameEmbeddedIdField())
) {
$values['id'] = $values['_id'];
unset($values['_id']);
}
Expand All @@ -1837,13 +1842,16 @@ public function aliasIdForResult(array|object $values): array|object
$values[$key] = Date::instance($value->toDateTime())
->setTimezone(new DateTimeZone(date_default_timezone_get()));
} elseif (is_array($value) || is_object($value)) {
$values[$key] = $this->aliasIdForResult($value);
$values[$key] = $this->aliasIdForResult($value, false);
}
}
}

if ($values instanceof stdClass) {
if (property_exists($values, '_id') && ! property_exists($values, 'id')) {
if (
property_exists($values, '_id') && ! property_exists($values, 'id')
&& ($root || $this->connection->getRenameEmbeddedIdField())
) {
$values->id = $values->_id;
unset($values->_id);
}
Expand All @@ -1853,7 +1861,7 @@ public function aliasIdForResult(array|object $values): array|object
$values->{$key} = Date::instance($value->toDateTime())
->setTimezone(new DateTimeZone(date_default_timezone_get()));
} elseif (is_array($value) || is_object($value)) {
$values->{$key} = $this->aliasIdForResult($value);
$values->{$key} = $this->aliasIdForResult($value, false);
}
}
}
Expand Down
43 changes: 34 additions & 9 deletions tests/Query/BuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
use Illuminate\Tests\Database\DatabaseQueryBuilderTest;
use InvalidArgumentException;
use LogicException;
use Mockery as m;
use MongoDB\BSON\Regex;
use MongoDB\BSON\UTCDateTime;
use MongoDB\Driver\ReadPreference;
Expand All @@ -39,7 +38,7 @@ public function testMql(array $expected, Closure $build, ?string $requiredMethod
$this->markTestSkipped(sprintf('Method "%s::%s()" does not exist.', Builder::class, $requiredMethod));
}

$builder = $build(self::getBuilder());
$builder = $build($this->getBuilder());
$this->assertInstanceOf(Builder::class, $builder);
$mql = $builder->toMql();

Expand Down Expand Up @@ -1447,7 +1446,7 @@ function (Builder $elemMatchQuery): void {
#[DataProvider('provideExceptions')]
public function testException($class, $message, Closure $build): void
{
$builder = self::getBuilder();
$builder = $this->getBuilder();

$this->expectException($class);
$this->expectExceptionMessage($message);
Expand Down Expand Up @@ -1545,7 +1544,7 @@ public static function provideExceptions(): iterable
#[DataProvider('getEloquentMethodsNotSupported')]
public function testEloquentMethodsNotSupported(Closure $callback)
{
$builder = self::getBuilder();
$builder = $this->getBuilder();

$this->expectException(BadMethodCallException::class);
$this->expectExceptionMessage('This method is not supported by MongoDB');
Expand Down Expand Up @@ -1600,12 +1599,38 @@ public static function getEloquentMethodsNotSupported()
yield 'orWhereIntegerNotInRaw' => [fn (Builder $builder) => $builder->orWhereIntegerNotInRaw('id', ['1a', 2])];
}

private static function getBuilder(): Builder
public function testRenameEmbeddedIdFieldCanBeDisabled()
Copy link
Member

Choose a reason for hiding this comment

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

Does this test exercise both aliasIdForQuery and aliasIdForResult()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for this question, I added a full test to cover both.

{
$connection = m::mock(Connection::class);
$processor = m::mock(Processor::class);
$connection->shouldReceive('getSession')->andReturn(null);
$connection->shouldReceive('getQueryGrammar')->andReturn(new Grammar($connection));
$builder = $this->getBuilder(false);
$this->assertFalse($builder->getConnection()->getRenameEmbeddedIdField());

$mql = $builder
->where('id', '=', 10)
->where('nested.id', '=', 20)
->where('embed', '=', ['id' => 30])
->toMql();

$this->assertEquals([
'find' => [
[
'$and' => [
['_id' => 10],
['nested.id' => 20],
['embed' => ['id' => 30]],
],
],
['typeMap' => ['root' => 'object', 'document' => 'array']],
],
], $mql);
}

private function getBuilder(bool $renameEmbeddedIdField = true): Builder
{
$connection = $this->createStub(Connection::class);
$connection->method('getRenameEmbeddedIdField')->willReturn($renameEmbeddedIdField);
$processor = $this->createStub(Processor::class);
$connection->method('getSession')->willReturn(null);
$connection->method('getQueryGrammar')->willReturn(new Grammar($connection));

return new Builder($connection, null, $processor);
}
Expand Down
89 changes: 89 additions & 0 deletions tests/QueryBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@
use MongoDB\BSON\UTCDateTime;
use MongoDB\Collection;
use MongoDB\Driver\Cursor;
use MongoDB\Driver\CursorInterface;
use MongoDB\Driver\Monitoring\CommandFailedEvent;
use MongoDB\Driver\Monitoring\CommandStartedEvent;
use MongoDB\Driver\Monitoring\CommandSubscriber;
use MongoDB\Driver\Monitoring\CommandSucceededEvent;
use MongoDB\Laravel\Connection;
use MongoDB\Laravel\Query\Builder;
use MongoDB\Laravel\Tests\Models\Item;
use MongoDB\Laravel\Tests\Models\User;
Expand Down Expand Up @@ -336,6 +338,93 @@ public function testRaw()
$this->assertEquals('Jane Doe', $results[0]->name);
}

public function testRawResultRenameId()
{
$connection = DB::connection('mongodb');
self::assertInstanceOf(Connection::class, $connection);

$date = Carbon::createFromDate(1986, 12, 31)->setTime(12, 0, 0);
User::insert([
['id' => 1, 'name' => 'Jane Doe', 'address' => ['id' => 11, 'city' => 'Ghent'], 'birthday' => $date],
['id' => 2, 'name' => 'John Doe', 'address' => ['id' => 12, 'city' => 'Brussels'], 'birthday' => $date],
]);

// Using raw database query, result is not altered
$results = $connection->table('users')->raw(fn (Collection $collection) => $collection->find([]));
self::assertInstanceOf(CursorInterface::class, $results);
$results = $results->toArray();
self::assertCount(2, $results);

self::assertObjectHasProperty('_id', $results[0]);
self::assertObjectNotHasProperty('id', $results[0]);
self::assertSame(1, $results[0]->_id);

self::assertObjectHasProperty('_id', $results[0]->address);
self::assertObjectNotHasProperty('id', $results[0]->address);
self::assertSame(11, $results[0]->address->_id);

self::assertInstanceOf(UTCDateTime::class, $results[0]->birthday);

// Using Eloquent query, result is transformed
self::assertTrue($connection->getRenameEmbeddedIdField());
$results = User::raw(fn (Collection $collection) => $collection->find([]));
self::assertInstanceOf(LaravelCollection::class, $results);
self::assertCount(2, $results);

$attributes = $results->first()->getAttributes();
self::assertArrayHasKey('id', $attributes);
self::assertArrayNotHasKey('_id', $attributes);
self::assertSame(1, $attributes['id']);

self::assertArrayHasKey('id', $attributes['address']);
self::assertArrayNotHasKey('_id', $attributes['address']);
self::assertSame(11, $attributes['address']['id']);

self::assertEquals($date, $attributes['birthday']);

// Single result
$result = User::raw(fn (Collection $collection) => $collection->findOne([], ['typeMap' => ['root' => 'object', 'document' => 'array']]));
self::assertInstanceOf(User::class, $result);

$attributes = $result->getAttributes();
self::assertArrayHasKey('id', $attributes);
self::assertArrayNotHasKey('_id', $attributes);
self::assertSame(1, $attributes['id']);

self::assertArrayHasKey('id', $attributes['address']);
self::assertArrayNotHasKey('_id', $attributes['address']);
self::assertSame(11, $attributes['address']['id']);

// Change the renameEmbeddedIdField option
$connection->setRenameEmbeddedIdField(false);

$results = User::raw(fn (Collection $collection) => $collection->find([]));
self::assertInstanceOf(LaravelCollection::class, $results);
self::assertCount(2, $results);

$attributes = $results->first()->getAttributes();
self::assertArrayHasKey('id', $attributes);
self::assertArrayNotHasKey('_id', $attributes);
self::assertSame(1, $attributes['id']);

self::assertArrayHasKey('_id', $attributes['address']);
self::assertArrayNotHasKey('id', $attributes['address']);
self::assertSame(11, $attributes['address']['_id']);

// Single result
$result = User::raw(fn (Collection $collection) => $collection->findOne([]));
self::assertInstanceOf(User::class, $result);

$attributes = $result->getAttributes();
self::assertArrayHasKey('id', $attributes);
self::assertArrayNotHasKey('_id', $attributes);
self::assertSame(1, $attributes['id']);

self::assertArrayHasKey('_id', $attributes['address']);
self::assertArrayNotHasKey('id', $attributes['address']);
self::assertSame(11, $attributes['address']['_id']);
}

public function testPush()
{
$id = DB::table('users')->insertGetId([
Expand Down
Loading