mongodb / laravel-mongodb

A MongoDB based Eloquent model and Query builder for Laravel (Moloquent)
https://www.mongodb.com/compatibility/mongodb-laravel-integration
MIT License
6.96k stars 1.42k forks source link

[Bug fix] Fix BelongToMany when using ObjectId in relation #3014

Open florianJacques opened 1 week ago

florianJacques commented 1 week ago

When we use mongodb ObjectIDs for relationships In the case of a BelongToMany relation, the foreignKeys was incorrectly cast

{
    "_id" : ObjectId("667e831289f74871500d80a7"),
    "name" : "Jean-Luc Picard",
    "updated_at" : ISODate("2024-06-28T09:32:02.401+0000"),
    "created_at" : ISODate("2024-06-28T09:32:02.401+0000"),
    "planetIds" : [
        {
            "oid" : "667e831289f74871500d80a2"
        },
        {
            "oid" : "667e831289f74871500d80a3"
        },
        {
            "oid" : "667e831289f74871500d80a4"
        }
    ]
}

With the type check, the cast is adapted to avoid converting an object into an associative array.

{
    "_id" : ObjectId("667e91e6bfeefc593308f745"),
    "name" : "Tanya Kirbuk",
    "updated_at" : ISODate("2024-06-28T10:35:18.459+0000"),
    "created_at" : ISODate("2024-06-28T10:35:18.459+0000"),
    "planetIds" : [
        ObjectId("667e91e6bfeefc593308f742"),
        ObjectId("667e91e6bfeefc593308f743"),
        ObjectId("667e91e6bfeefc593308f744")
    ]
}

3015

Checklist

alcaeus commented 1 week ago

Can you please add tests for this change? Thank you!

florianJacques commented 1 week ago

Can you please add tests for this change? Thank you!

Yes of course when I have a little time. I missed the sync/toggle functions. I still have work to do.

florianJacques commented 5 days ago

Add Test OK.

GromNaN commented 1 day ago

Sorry for the time it takes to review your PR. This part of the code is complicated and we need to ensure this is feature we can support and maintain for the long run. The more we copy and modify code from the parent class, the more difficult it is to maintain and keep compatibility with multiple versions of Laravel version (even minor version can break). Also, I guess belongsToMany is not the only type of relationship that would need such change.

florianJacques commented 1 day ago

Sorry for the time it takes to review your PR. This part of the code is complicated and we need to ensure this is feature we can support and maintain for the long run. The more we copy and modify code from the parent class, the more difficult it is to maintain and keep compatibility with multiple versions of Laravel version (even minor version can break). Also, I guess belongsToMany is not the only type of relationship that would need such change.

No problem, the big change is in the "formatRecordsList" method, which currently assumes the existence of a pivot table.