Skip to content

Fix BelongsToMany relationship bugs when using custom keys #2695

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

Merged
merged 7 commits into from
Dec 13, 2023
Merged

Fix BelongsToMany relationship bugs when using custom keys #2695

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3626c36
Update pluralizing logic in HybridRelations;
hans-thomas Dec 12, 2023
9c9e6d8
Update BelongsToMany.php
hans-thomas Dec 12, 2023
826d945
Add tests for sync, attach and detach actions with custom keys;
hans-thomas Dec 12, 2023
e54b822
apply phpcbf formatting
hans-thomas Dec 12, 2023
3e6913b
Merge branch '4.1' into belongs-to-many-relation-with-custom-keys
hans-thomas Dec 12, 2023
cfe4056
WIP
hans-thomas Dec 12, 2023
4f1eecb
Revert "Update pluralizing logic in HybridRelations;"
hans-thomas Dec 12, 2023
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
4 changes: 2 additions & 2 deletions src/Eloquent/HybridRelations.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,11 +303,11 @@ public function belongsToMany(
// First, we'll need to determine the foreign key and "other key" for the
// relationship. Once we have determined the keys we'll make the query
// instances as well as the relationship instances we need for this.
$foreignPivotKey = $foreignPivotKey ?: $this->getForeignKey() . 's';
$foreignPivotKey = $foreignPivotKey ?: Str::plural($this->getForeignKey());

$instance = new $related();

$relatedPivotKey = $relatedPivotKey ?: $instance->getForeignKey() . 's';
$relatedPivotKey = $relatedPivotKey ?: Str::plural($instance->getForeignKey());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pluralizing foreignPivotKey and relatedPivotKey keys in the same way in MorphToMany

laravel-mongodb/src/Eloquent/HybridRelations.php

Line 387 in f0eed1a

$relatedPivotKey = $relatedPivotKey ?: Str::plural($instance->getForeignKey());

Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid it's a breaking change. This (incorrect) naming convention is here since the beginning and it will break silently for applications that don't use schema validation (what I think is the majority). By changing this rule, they will start having 2 lists (childs and children) and that will be hard to fix.

Let's add a deprecation notice instead, if the pluralization is different.

Please revert in this PR and open an new one with this change.

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 agree. this can be done in 5.x version. devs need to override their foreignPivotKey and relatedPivotKey keys for just existing relationships to upgrade to 5.x version.


// If no table name was provided, we can guess it by concatenating the two
// models using underscores in alphabetical order. The two model names
Expand Down
12 changes: 6 additions & 6 deletions src/Relations/BelongsToMany.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,13 +183,13 @@ public function attach($id, array $attributes = [], $touch = true)
if ($id instanceof Model) {
$model = $id;

$id = $model->getKey();
$id = $this->parseId($model);

// Attach the new parent id to the related model.
$model->push($this->foreignPivotKey, $this->parent->getKey(), true);
$model->push($this->foreignPivotKey, $this->parent->{$this->parentKey}, true);
} else {
if ($id instanceof Collection) {
$id = $id->modelKeys();
$id = $this->parseIds($id);
}

$query = $this->newRelatedQuery();
Expand Down Expand Up @@ -221,7 +221,7 @@ public function attach($id, array $attributes = [], $touch = true)
public function detach($ids = [], $touch = true)
{
if ($ids instanceof Model) {
$ids = (array) $ids->getKey();
$ids = $this->parseIds($ids);
}

$query = $this->newRelatedQuery();
Expand All @@ -242,13 +242,13 @@ public function detach($ids = [], $touch = true)

// Prepare the query to select all related objects.
if (count($ids) > 0) {
$query->whereIn($this->related->getKeyName(), $ids);
$query->whereIn($this->relatedKey, $ids);
}

// Remove the relation to the parent.
assert($this->parent instanceof Model);
assert($query instanceof \MongoDB\Laravel\Eloquent\Builder);
$query->pull($this->foreignPivotKey, $this->parent->getKey());
$query->pull($this->foreignPivotKey, $this->parent->{$this->parentKey});

if ($touch) {
$this->touchIfTouching();
Expand Down
11 changes: 11 additions & 0 deletions tests/Models/Client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,17 @@ public function users(): BelongsToMany
return $this->belongsToMany(User::class);
}

public function skillsWithCustomKeys()
{
return $this->belongsToMany(
Skill::class,
foreignPivotKey: 'cclient_ids',
relatedPivotKey: 'cskill_ids',
parentKey: 'cclient_id',
relatedKey: 'cskill_id',
);
}

public function photo(): MorphOne
{
return $this->morphOne(Photo::class, 'has_image');
Expand Down
10 changes: 0 additions & 10 deletions tests/Models/Experience.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,6 @@ class Experience extends Eloquent

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');
}

public function sqlUsers(): MorphToMany
{
return $this->morphToMany(SqlUser::class, 'experienced');
Expand Down
228 changes: 202 additions & 26 deletions tests/RelationsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
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\Label;
Expand All @@ -36,7 +35,6 @@ public function tearDown(): void
Photo::truncate();
Label::truncate();
Skill::truncate();
Experience::truncate();
}

public function testHasMany(): void
Expand Down Expand Up @@ -350,48 +348,226 @@ public function testBelongsToManyAttachEloquentCollection(): void
$this->assertCount(2, $user->clients);
}

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

$client = Client::query()->find($client->_id);
$client->skillsWithCustomKeys()->sync([$skill1->cskill_id, $skill2->cskill_id]);
$this->assertCount(2, $client->skillsWithCustomKeys);

self::assertIsString($skill1->cskill_id);
self::assertContains($skill1->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($skill1->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

self::assertIsString($skill2->cskill_id);
self::assertContains($skill2->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($skill2->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

$check = Skill::query()->find($skill1->_id);
self::assertIsString($check->cskill_id);
self::assertContains($check->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($check->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

$check = Skill::query()->find($skill2->_id);
self::assertIsString($check->cskill_id);
self::assertContains($check->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($check->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
}

public function testBelongsToManySyncModelWithCustomKeys(): void
{
$client = Client::create(['cclient_id' => (string) (new ObjectId()), 'years' => '5']);
$skill1 = Skill::create(['cskill_id' => (string) (new ObjectId()), 'name' => 'PHP']);

$client = Client::query()->find($client->_id);
$client->skillsWithCustomKeys()->sync($skill1);
$this->assertCount(1, $client->skillsWithCustomKeys);

self::assertIsString($skill1->cskill_id);
self::assertContains($skill1->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($skill1->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

$check = Skill::query()->find($skill1->_id);
self::assertIsString($check->_id);
self::assertContains($check->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($check->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
}

public function testBelongsToManySyncEloquentCollectionWithCustomKeys(): void
{
$client = Client::create(['cclient_id' => (string) (new ObjectId()), '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);
$client = Client::query()->find($client->_id);
$client->skillsWithCustomKeys()->sync($collection);
$this->assertCount(2, $client->skillsWithCustomKeys);

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

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

$check = Skill::query()->find($skill1->_id);
self::assertIsString($check->cskill_id);
self::assertContains($check->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($check->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

$check = Skill::query()->find($skill2->_id);
self::assertIsString($check->cskill_id);
self::assertContains($check->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($check->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
}

public function testBelongsToManyAttachWithCustomKeys(): void
{
$client = Client::create(['cclient_id' => (string) (new ObjectId()), 'years' => '5']);
$skill1 = Skill::create(['cskill_id' => (string) (new ObjectId()), 'name' => 'PHP']);
$skill2 = Skill::create(['cskill_id' => (string) (new ObjectId()), 'name' => 'Laravel']);

$client = Client::query()->find($client->_id);
$client->skillsWithCustomKeys()->attach([$skill1->cskill_id, $skill2->cskill_id]);
$this->assertCount(2, $client->skillsWithCustomKeys);

self::assertIsString($skill1->cskill_id);
self::assertContains($skill1->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($skill1->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

self::assertIsString($skill2->cskill_id);
self::assertContains($skill2->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($skill2->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

$check = Skill::query()->find($skill1->_id);
self::assertIsString($check->cskill_id);
self::assertContains($check->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($check->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

$check = Skill::query()->find($skill2->_id);
self::assertIsString($check->cskill_id);
self::assertContains($check->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($check->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
}

public function testBelongsToManyAttachModelWithCustomKeys(): void
{
$client = Client::create(['cclient_id' => (string) (new ObjectId()), 'years' => '5']);
$skill1 = Skill::create(['cskill_id' => (string) (new ObjectId()), 'name' => 'PHP']);

$skill1->refresh();
self::assertIsString($skill1->_id);
self::assertNotContains($skill1->_id, $experience->skillsWithCustomRelatedKey->pluck('cskill_id'));
$client = Client::query()->find($client->_id);
$client->skillsWithCustomKeys()->attach($skill1);
$this->assertCount(1, $client->skillsWithCustomKeys);

self::assertIsString($skill1->cskill_id);
self::assertContains($skill1->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($skill1->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

$skill2->refresh();
self::assertIsString($skill2->_id);
self::assertNotContains($skill2->_id, $experience->skillsWithCustomRelatedKey->pluck('cskill_id'));
$check = Skill::query()->find($skill1->_id);
self::assertIsString($check->_id);
self::assertContains($check->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($check->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
}

public function testBelongsToManySyncEloquentCollectionWithCustomParentKey(): void
public function testBelongsToManyAttachEloquentCollectionWithCustomKeys(): void
{
$experience = Experience::create(['cexperience_id' => (string) (new ObjectId()), 'years' => '5']);
$skill1 = Skill::create(['name' => 'PHP']);
$skill2 = Skill::create(['name' => 'Laravel']);
$client = Client::create(['cclient_id' => (string) (new ObjectId()), '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->skillsWithCustomParentKey()->sync($collection);
$this->assertCount(2, $experience->skillsWithCustomParentKey);
$client = Client::query()->find($client->_id);
$client->skillsWithCustomKeys()->attach($collection);
$this->assertCount(2, $client->skillsWithCustomKeys);

self::assertIsString($skill1->cskill_id);
self::assertContains($skill1->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($skill1->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

self::assertIsString($skill2->cskill_id);
self::assertContains($skill2->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($skill2->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

$check = Skill::query()->find($skill1->_id);
self::assertIsString($check->cskill_id);
self::assertContains($check->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($check->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

$check = Skill::query()->find($skill2->_id);
self::assertIsString($check->cskill_id);
self::assertContains($check->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($check->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
}

public function testBelongsToManyDetachWithCustomKeys(): void
{
$client = Client::create(['cclient_id' => (string) (new ObjectId()), 'years' => '5']);
$skill1 = Skill::create(['cskill_id' => (string) (new ObjectId()), 'name' => 'PHP']);
$skill2 = Skill::create(['cskill_id' => (string) (new ObjectId()), 'name' => 'Laravel']);

$client = Client::query()->find($client->_id);
$client->skillsWithCustomKeys()->sync([$skill1->cskill_id, $skill2->cskill_id]);
$this->assertCount(2, $client->skillsWithCustomKeys);

$client->skillsWithCustomKeys()->detach($skill1->cskill_id);
$client->load('skillsWithCustomKeys'); // Reload the relationship based on the latest pivot column's data
$this->assertCount(1, $client->skillsWithCustomKeys);

self::assertIsString($skill1->cskill_id);
self::assertNotContains($skill1->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($skill1->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

self::assertIsString($skill1->_id);
self::assertContains($skill1->_id, $experience->skillsWithCustomParentKey->pluck('_id'));
self::assertIsString($skill2->cskill_id);
self::assertContains($skill2->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($skill2->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

$check = Skill::query()->find($skill1->_id);
self::assertIsString($check->cskill_id);
self::assertNotContains($check->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($check->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

$check = Skill::query()->find($skill2->_id);
self::assertIsString($check->cskill_id);
self::assertContains($check->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($check->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
}

self::assertIsString($skill2->_id);
self::assertContains($skill2->_id, $experience->skillsWithCustomParentKey->pluck('_id'));
public function testBelongsToManyDetachModelWithCustomKeys(): void
{
$client = Client::create(['cclient_id' => (string) (new ObjectId()), 'years' => '5']);
$skill1 = Skill::create(['cskill_id' => (string) (new ObjectId()), 'name' => 'PHP']);
$skill2 = Skill::create(['cskill_id' => (string) (new ObjectId()), 'name' => 'Laravel']);

$client = Client::query()->find($client->_id);
$client->skillsWithCustomKeys()->sync([$skill1->cskill_id, $skill2->cskill_id]);
$this->assertCount(2, $client->skillsWithCustomKeys);

$client->skillsWithCustomKeys()->detach($skill1);
$client->load('skillsWithCustomKeys'); // Reload the relationship based on the latest pivot column's data
$this->assertCount(1, $client->skillsWithCustomKeys);

self::assertIsString($skill1->cskill_id);
self::assertNotContains($skill1->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($skill1->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

self::assertIsString($skill2->cskill_id);
self::assertContains($skill2->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($skill2->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

$check = Skill::query()->find($skill1->_id);
self::assertIsString($check->cskill_id);
self::assertNotContains($check->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($check->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));

$check = Skill::query()->find($skill2->_id);
self::assertIsString($check->cskill_id);
self::assertContains($check->cskill_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
self::assertNotContains($check->_id, $client->skillsWithCustomKeys->pluck('cskill_id'));
}

public function testBelongsToManySyncAlreadyPresent(): void
Expand Down