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

[Fix] BelongsToMany sync does't use configured keys #2667

Merged
merged 11 commits into from
Nov 6, 2023
8 changes: 4 additions & 4 deletions src/Relations/BelongsToMany.php
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ protected function setWhere()
{
$foreign = $this->getForeignKey();

$this->query->where($foreign, '=', $this->parent->getKey());
$this->query->where($foreign, '=', $this->parent->{$this->parentKey});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a custom parentKey is defined, the custom parentKey's value must be searched in the foreign keys list instead of _id.


return $this;
}
Expand Down Expand Up @@ -116,7 +116,7 @@ public function sync($ids, $detaching = true)
];

if ($ids instanceof Collection) {
$ids = $ids->modelKeys();
$ids = $this->parseIds($ids);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in #2666, $ids->modelKeys() returns _id field or primary key. However, $this->parseIds($ids) returns relatedKey which is the primary key by default.

} elseif ($ids instanceof Model) {
$ids = $this->parseIds($ids);
}
Expand Down Expand Up @@ -190,10 +190,10 @@ public function attach($id, array $attributes = [], $touch = true)

$query = $this->newRelatedQuery();

$query->whereIn($this->related->getKeyName(), (array) $id);
$query->whereIn($this->relatedKey, (array) $id);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I said in the previous comment, relatedKey is $this->related->getKeyName() by default but respects our override.


// Attach the new parent id to the related model.
$query->push($this->foreignPivotKey, $this->parent->getKey(), true);
$query->push($this->foreignPivotKey, $this->parent->{$this->parentKey}, true);
Copy link
Contributor Author

@hans-thomas hans-thomas Nov 5, 2023

Choose a reason for hiding this comment

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

The $this->parentKey is replaced with $this->parent->getKey() to respect the custom parentKey field.

}

// Attach the new ids to the parent model.
Expand Down
26 changes: 26 additions & 0 deletions tests/Models/Experience.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php

declare(strict_types=1);

namespace MongoDB\Laravel\Tests\Models;

use MongoDB\Laravel\Eloquent\Model as Eloquent;

class Experience extends Eloquent
{
protected $connection = 'mongodb';
protected $collection = 'experiences';
protected static $unguarded = true;

protected $casts = ['years' => 'int'];

public function skillsWithCustomRelatedKey()
{
return $this->belongsToMany(Skill::class, relatedKey: 'cskill_id');
}

public function skillsWithCustomParentKey()
{
return $this->belongsToMany(Skill::class, parentKey: 'cexperience_id');
}
}
14 changes: 14 additions & 0 deletions tests/Models/Skill.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?php

declare(strict_types=1);

namespace MongoDB\Laravel\Tests\Models;

use MongoDB\Laravel\Eloquent\Model as Eloquent;

class Skill extends Eloquent
{
protected $connection = 'mongodb';
protected $collection = 'skills';
protected static $unguarded = true;
}
102 changes: 64 additions & 38 deletions tests/RelationsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,16 @@

use Illuminate\Database\Eloquent\Collection;
use Mockery;
use MongoDB\BSON\ObjectId;
use MongoDB\Laravel\Tests\Models\Address;
use MongoDB\Laravel\Tests\Models\Book;
use MongoDB\Laravel\Tests\Models\Client;
use MongoDB\Laravel\Tests\Models\Experience;
use MongoDB\Laravel\Tests\Models\Group;
use MongoDB\Laravel\Tests\Models\Item;
use MongoDB\Laravel\Tests\Models\Photo;
use MongoDB\Laravel\Tests\Models\Role;
use MongoDB\Laravel\Tests\Models\Skill;
use MongoDB\Laravel\Tests\Models\Soft;
use MongoDB\Laravel\Tests\Models\User;

Expand All @@ -30,6 +33,8 @@ public function tearDown(): void
Role::truncate();
Group::truncate();
Photo::truncate();
Skill::truncate();
Experience::truncate();
}

public function testHasMany(): void
Expand Down Expand Up @@ -255,36 +260,6 @@ public function testBelongsToMany(): void
$this->assertCount(1, $client->users);
}

public function testSyncBelongsToMany()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I merged this test with testBelongsToManySync test. because I see no reason to keep them separate and I think it's cleaner now.

{
$user = User::create(['name' => 'John Doe']);

$first = Client::query()->create(['name' => 'Hans']);
$second = Client::query()->create(['name' => 'Thomas']);

$user->load('clients');
self::assertEmpty($user->clients);

$user->clients()->sync($first);

$user->load('clients');
self::assertCount(1, $user->clients);
self::assertTrue($user->clients->first()->is($first));

$user->clients()->sync($second);

$user->load('clients');
self::assertCount(1, $user->clients);
self::assertTrue($user->clients->first()->is($second));

$user->clients()->syncWithoutDetaching($first);

$user->load('clients');
self::assertCount(2, $user->clients);
self::assertTrue($user->clients->first()->is($first));
self::assertTrue($user->clients->last()->is($second));
}

public function testBelongsToManyAttachesExistingModels(): void
{
$user = User::create(['name' => 'John Doe', 'client_ids' => ['1234523']]);
Expand Down Expand Up @@ -327,20 +302,27 @@ public function testBelongsToManyAttachesExistingModels(): void
public function testBelongsToManySync(): void
{
// create test instances
$user = User::create(['name' => 'John Doe']);
$client1 = Client::create(['name' => 'Pork Pies Ltd.'])->_id;
$client2 = Client::create(['name' => 'Buffet Bar Inc.'])->_id;
$user = User::create(['name' => 'Hans Thomas']);
$client1 = Client::create(['name' => 'Pork Pies Ltd.']);
$client2 = Client::create(['name' => 'Buffet Bar Inc.']);

// Sync multiple
$user->clients()->sync([$client1, $client2]);
$user->clients()->sync([$client1->_id, $client2->_id]);
$this->assertCount(2, $user->clients);

// Refresh user
$user = User::where('name', '=', 'John Doe')->first();
// Sync single wrapped by an array
$user->clients()->sync([$client1->_id]);
$user->load('clients');

$this->assertCount(1, $user->clients);
self::assertTrue($user->clients->first()->is($client1));

// Sync single model
$user->clients()->sync($client2);
$user->load('clients');

// Sync single
$user->clients()->sync([$client1]);
$this->assertCount(1, $user->clients);
self::assertTrue($user->clients->first()->is($client2));
}

public function testBelongsToManyAttachArray(): void
Expand All @@ -366,6 +348,50 @@ public function testBelongsToManyAttachEloquentCollection(): void
$this->assertCount(2, $user->clients);
}

public function testBelongsToManySyncEloquentCollectionWithCustomRelatedKey(): void
{
$experience = Experience::create(['years' => '5']);
$skill1 = Skill::create(['cskill_id' => (string) (new ObjectId()), 'name' => 'PHP']);
$skill2 = Skill::create(['cskill_id' => (string) (new ObjectId()), 'name' => 'Laravel']);
$collection = new Collection([$skill1, $skill2]);

$experience = Experience::query()->find($experience->id);
$experience->skillsWithCustomRelatedKey()->sync($collection);
$this->assertCount(2, $experience->skillsWithCustomRelatedKey);

self::assertIsString($skill1->cskill_id);
self::assertContains($skill1->cskill_id, $experience->skillsWithCustomRelatedKey->pluck('cskill_id'));

self::assertIsString($skill2->cskill_id);
self::assertContains($skill2->cskill_id, $experience->skillsWithCustomRelatedKey->pluck('cskill_id'));

$skill1->refresh();
self::assertIsString($skill1->_id);
self::assertNotContains($skill1->_id, $experience->skillsWithCustomRelatedKey->pluck('cskill_id'));

$skill2->refresh();
self::assertIsString($skill2->_id);
self::assertNotContains($skill2->_id, $experience->skillsWithCustomRelatedKey->pluck('cskill_id'));
}

public function testBelongsToManySyncEloquentCollectionWithCustomParentKey(): void
{
$experience = Experience::create(['cexperience_id' => (string) (new ObjectId()), 'years' => '5']);
$skill1 = Skill::create(['name' => 'PHP']);
$skill2 = Skill::create(['name' => 'Laravel']);
$collection = new Collection([$skill1, $skill2]);

$experience = Experience::query()->find($experience->id);
$experience->skillsWithCustomParentKey()->sync($collection);
$this->assertCount(2, $experience->skillsWithCustomParentKey);

self::assertIsString($skill1->_id);
self::assertContains($skill1->_id, $experience->skillsWithCustomParentKey->pluck('_id'));

self::assertIsString($skill2->_id);
self::assertContains($skill2->_id, $experience->skillsWithCustomParentKey->pluck('_id'));
}

public function testBelongsToManySyncAlreadyPresent(): void
{
$user = User::create(['name' => 'John Doe']);
Expand Down