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

Repeater fails silently and pretty badly on BelongsToMany relationship #5933

Closed
LBreda opened this issue Mar 14, 2023 · 7 comments · Fixed by #6074
Closed

Repeater fails silently and pretty badly on BelongsToMany relationship #5933

LBreda opened this issue Mar 14, 2023 · 7 comments · Fixed by #6074
Labels
bug Something isn't working unconfirmed

Comments

@LBreda
Copy link
Contributor

LBreda commented Mar 14, 2023

Package

filament/forms

Package Version

v2.17.15

Laravel Version

v10.3.3

Livewire Version

v2.3

PHP Version

PHP 8.2

Problem description

When I try to update (or delete) a related item via a relationship-based repeater, it updates (or deletes) the related item with the same id of the pivot table record.

Expected behavior

I expect to update or delete the actual record I try to update or deleted, also when its id isn't the same of the pivot table record. At least I expect a failure. Editing another record pretty randomly is very bad.

Steps to reproduce

In general:

  • Create two tables and a pivot table to connect them in a many-to-many fashion
  • Create the two models, with the BelongsToMany relationship
  • Create a resource for one of the two models, with a repeater using the relationship
  • Create the related records, carefully avoiding them to have the same id of the pivot table record
  • Try to update them via the repeater.

In the reproduction repository:

  • Configure .env
  • Run php artisan app:install. Fill the credentials when asked
  • Open the project on /admin and login
  • Try to edit one of the related items in the first test record
  • Behold how it actually changes the related items of the second test record

Reproduction repository

https://github.com/LBreda/filament-repeater-bug

Relevant log output

No response

@LBreda LBreda added bug Something isn't working unconfirmed labels Mar 14, 2023
@danharrin
Copy link
Member

Unfortunately this is a misunderstanding about what the repeater does. The repeater writes to the "related" model of the relationship. In this case, if your repeater is related to test_items, it writes to the test_items table. Not the pivot table.

In your case, you want to set up a HasMany relationship to the pivot table and use that for the repeater. Then, creating and updating and deleting repeater items will change pivot table records, not test_items.

@danharrin danharrin closed this as not planned Won't fix, can't repro, duplicate, stale Mar 14, 2023
@LBreda
Copy link
Contributor Author

LBreda commented Mar 14, 2023

No, I didn't misunderstand.

In my example it MUST write to the related model, I'm perfecly OK with it.

The problem is that it picks the wrong record on the related table: it picks the one with the same id of the pivot table.

The example I built is pretty clear, you probably should try it.

  • Test(1) has two related items, TestItem(3) [linked by the record 1 of the pivot] and TestItem(4) [linked by the record 2 of the pivot]
  • Test(2) has two related items, TestItem(1) [linked by the record 3 of the pivot] and TestItem(2) [linked by the record 4 of the pivot]

When I try to edit TestItem(3) via the repeater, it picks the id from the pivot and edits TestItem(1), since the id of the pivot is 1.

This is a obvious bug. Please reopen the issue.

@danharrin danharrin reopened this Mar 14, 2023
@danharrin
Copy link
Member

I'm pretty sure there is an explanation as to why it works that way, when I come back to this issue I'll add more info.

@LBreda
Copy link
Contributor Author

LBreda commented Mar 14, 2023

I'm pretty sure too, and that's why I'm avoiding working it around risking to break other parts of my project.

A note: the repeater items in the sent data contain all the field values and all the pivot table values (id, test_id, test_item_id), and the items are named with the pivot table id. It probably picks the wrong relatedKeyName (maybe id instead of related_table_name.id?) somewhere under the hood.

@LBreda
Copy link
Contributor Author

LBreda commented Mar 27, 2023

Could you please take a look? It seems pretty huge to me on a security side (this bug lets/makes you change records you are not intended to change) and it is preventing me to use the system. I tried to inspect what's going on, but the inner workings are not documented enough to let me correctly follow the request's lifecycle.

@LBreda
Copy link
Contributor Author

LBreda commented Mar 29, 2023

Some details. On Filament\Forms\Components\Repeater, line 324, the $existingRecords variable, when you try to update related record via the repeater, contains a collection of objects of the related model (correct), filled with the properties of the relationship (wrong).

immagine

@LBreda
Copy link
Contributor Author

LBreda commented Mar 31, 2023

TY

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working unconfirmed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants