laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.32k stars 10.95k forks source link

Unexpected behavior in firstOrNew method in belongs to many relationships #42967

Closed ashkan-jafarzadeh closed 2 years ago

ashkan-jafarzadeh commented 2 years ago

Description:

There is an unexpected behavior in firstOrNew method in belongsToMany relationships. When I try to firstOrNew a related model which it doesn't exist, I get the first record of database which I shouldn't.

Steps To Reproduce:

I have a ContentPost model which has a belongsToMany relationship with UploaderFile and the pivot table is called uploader_attachments. This is the relation method in ContentPost:

public funciton files()
{
   return $this->belongsToMany(UploaderFile::class, "uploader_attachments", "model_id", "file_id")
                    ->withPivot("group", "model_name")
                    ->withTimestamps()
                    ->where("model_name", "ContentPost")
            ;
}

When I try to find a UploaderFile with a original_name which doesn't exist, It returns a new empty instance as below:

# executed query on uploader-file model:
(new UploaderFile)->where("original_name","something not exists")->firstOrNew();

# results:
App\Models\UploaderFile {
     id: 0,
 }

But when I execute this query on the ContentPost's files relation method. It gives me the first record.

# executed query on content-post files relation method:
(new ContentPost)->files()->where("original_name","something not exists")->firstOrNew();

# results:
App\Models\UploaderFile {
     id: 1,
     original_name: "24432660_0.jpg",
     extension: "jpg",
     mime_type: "image/jpeg",
     given_name: "1578837071_SW4zL94LHGn1ZYb5VrHHkFljJ8PCHjH6BCYfMGAx",
     private: 0,
     created_at: "2020-01-12 17:21:11",
     updated_at: "2020-01-12 17:21:11",
     deleted_at: null,
     created_by: 2,
     updated_by: 0,
     deleted_by: 0,
     meta: "{"versions":["panel_thumb"]}",
   }
driesvints commented 2 years ago

Heya, thanks for reporting.

We'll need more info and/or code to debug this further. Can you please create a repository with the command below, commit the code that reproduces the issue as separate commits on the main/master branch and share the repository here? Please make sure that you have the latest version of the Laravel installer in order to run this command. Please also make sure you have both Git & the GitHub CLI tool properly set up.

laravel new bug-report --github="--public"

Please do not amend and create a separate commit with your custom changes. After you've posted the repository, we'll try to reproduce the issue.

Thanks!

vaites commented 2 years ago

Same problem here, I found it with something like this, where there's no addresses associated to the person:

$person->addresses()->where('postal_code', 12345)->first();      // null
$person->addresses()->where('postal_code', 12345)->firstOrNew(); // address with ID 1 (not related with person)

The firstOrNew() method must respect all previous conditions.

Please let me know if I can help by providing an example.

driesvints commented 2 years ago

I'll re-open once a repo is provided.

vaites commented 2 years ago

Hi @driesvints, here you have the repository: https://github.com/vaites/laravel-first-or-new

I created the migrations, the models and an Artisan command to test it:

git clone https://github.com/vaites/laravel-first-or-new
cd laravel-first-or-new
cp .env.example .env
composer install
php artisan test:first-or-new

The output will display 3 lines:

[OK] first() returned address 1 from person 1
[OK] first() returned null for person 2
[ERROR] firstOrNew() returned 1 for person 2

As you can see here, $address3 must be a new and empty model but the first row in the table is returned. The first() method works well returnning null.

I used SQLite's in memory database for simplicity but the problem is the same using MySQL.

driesvints commented 2 years ago

Sorry. Didn't get to it this week.

driesvints commented 2 years ago

Hi all, I now realize the usage of firstOrNew in combination with where statements won't work. The way firstOrNew works is by adding the filtering attributes in the array of the first argument:

(new ContentPost)->files()->firstOrNew(['original_name' => 'something not exists']);

And then you'll get the behavior you seek.

vaites commented 2 years ago

Hi @driesvints, I understand what you say but the behaviour is still unexpected. The framework must not allow to do thinks like that where even a core developer like you need to dig into it to know what's happening. If firstOrNew does not work with where statements, an exception must be thrown (at least if no attributes are specified).

Using the previous example:

$person->addresses()->where('postal_code', 12345)->first();      // null is OK
$person->addresses()->where('postal_code', 12345)->firstOrNew(); // throw an exception
$person->addresses()->firstOrNew(['postal_code' => 12345]);      // expected behaviour

At least, add a warning here so programmers are aware of this behavior...

driesvints commented 2 years ago

Feel free to attempt a pr 👍

vaites commented 2 years ago

No problem. Must I fix the problem, do you want me to add the warning on docs or both?

ashkan-jafarzadeh commented 2 years ago

Hi @driesvints, thanks for your answering. I think you misunderstood the problem. You know, it is not even about a where clause. Normal belongsToMany relationship can cause this problem.

Please check this image which is taken from the tinker:

(new ContentPost())->files()->first(); 
    // returns null
(new ContentPost())->files()->firstOrNew();
   // returns first record of table:
App\Models\UploaderFile {#5443
     id: 1,
     original_name: "24432660_0.jpg",
     extension: "jpg",
     mime_type: "image/jpeg",
     given_name: "1578837071_SW4zL94LHGn1ZYb5VrHHkFljJ8PCHjH6BCYfMGAx",
     private: 0,
     created_at: "2020-01-12 17:21:11",
     updated_at: "2020-01-12 17:21:11",
     deleted_at: null,
     created_by: 2,
     updated_by: 0,
     deleted_by: 0,
     meta: "{"versions":["panel_thumb"]}",
     converted: 0,
   }

This happens because no ContentPosts are loaded, so it will grab the first record. it is the same as when you fetch it directly from UploaderFile: image

(new UploaderFile())->firstOrNew();
   // returns first record:
App\Models\UploaderFile {#5424
     id: 1,
     original_name: "24432660_0.jpg",
     extension: "jpg",
     mime_type: "image/jpeg",
     given_name: "1578837071_SW4zL94LHGn1ZYb5VrHHkFljJ8PCHjH6BCYfMGAx",
     private: 0,
     created_at: "2020-01-12 17:21:11",
     updated_at: "2020-01-12 17:21:11",
     deleted_at: null,
     created_by: 2,
     updated_by: 0,
     deleted_by: 0,
     meta: "{"versions":["panel_thumb"]}",
     converted: 0,
   }

The problem is when you already loaded a content-post model and you know it doesn't have any files. But in firstOrNew method, it is loading the first record still: image

// I am loading the first ContentPost
ContentPost::first()->files()->first(); 
    // returns null
ContentPost::first()->files()->firstOrNew();
   // returns first record of table:
App\Models\UploaderFile {#5443
     id: 1,
     original_name: "24432660_0.jpg",
     extension: "jpg",
     mime_type: "image/jpeg",
     given_name: "1578837071_SW4zL94LHGn1ZYb5VrHHkFljJ8PCHjH6BCYfMGAx",
     private: 0,
     created_at: "2020-01-12 17:21:11",
     updated_at: "2020-01-12 17:21:11",
     deleted_at: null,
     created_by: 2,
     updated_by: 0,
     deleted_by: 0,
     meta: "{"versions":["panel_thumb"]}",
     converted: 0,
   }

In Laravel8, the same code was returning an empty instance of the UploaderFile, which it was correct.

vaites commented 2 years ago

@driesvints indicates that whenever firstOrNew() is used, it must receive the conditions in the first parameter, regardless of whether it is done from a relation or directly. But I agree with you @ashkanRimer because my example is just like yours, where there is no condition (implicitly there is the relation) and a wrong result is returned. This should never happen, and even less if the problem is known.

I am evaluating how to perform a PR either apply the where or throw an exception if some where has been applied and firstOrNew() receives no parameters.

ashkan-jafarzadeh commented 2 years ago

There is a real unexpected behavior when even a model has been attached to the specified model. As you see in this image, my model (ContentPost) already has a file. It has been attached to an UploaderFIle with ID of 207, but in firstOrNew method it still gets the first record of UploaderFiles!!

ContentPost::first()->files()->first();
  // returns the first correct attached file 
App\Models\UploaderFile {#5407
     id: 207,
     original_name: "35699887_0.jpeg",
     extension: "jpeg",
     mime_type: "image/jpeg",
     given_name: "1657699399_3BhUGrLSe8anxQrxBKhnJ8S3ZVYBFwvPneeqjOCQ",
     private: 0,
     created_at: "2022-07-13 12:33:19",
     updated_at: "2022-07-13 12:33:19",
     deleted_at: null,
     created_by: 0,
     updated_by: 0,
     deleted_by: 0,
     meta: "{"versions":["panel_thumb"]}",
     converted: 0,
     pivot: Illuminate\Database\Eloquent\Relations\Pivot {#5408
       model_id: 70,
       file_id: 207,
       group: "",
       model_name: "ContentPost",
       created_at: "2022-07-23 17:46:55",
       updated_at: "2022-07-23 17:46:55",
     },
   }

ContentPost::first()->files()->firstOrNew();
   // returns the not-attached, not correct file. first record of database:
App\Models\UploaderFile {#5429
     id: 1,
     original_name: "24432660_0.jpg",
     extension: "jpg",
     mime_type: "image/png",
     given_name: "1578837071_SW4zL94LHGn1ZYb5VrHHkFljJ8PCHjH6BCYfMGAx",
     private: 0,
     created_at: "2020-01-12 17:21:11",
     updated_at: "2020-01-12 17:21:11",
     deleted_at: null,
     created_by: 2,
     updated_by: 0,
     deleted_by: 0,
     meta: "{"versions":["panel_thumb"]}",
     converted: 0,
   }

We always expect the first and firstOrNew methods to behave the same when a model exists, but they have different behaviors.

driesvints commented 2 years ago

I see a PR was submitted. Let's see how it goes.

mtx-z commented 1 year ago

After upgrading my Laravel 5.x application to the latest 9.x version, I came across a similar issue. Specifically, when invoking the firstOrNew() method on a belongsToMany relationship. On a "new" case, the method returns the first record of the related table, rather than a new instance of the related model as expected. It is worth noting that this returned row is not related to the parent model through the belongsToMany relationship. Moreover, it is important to mention that the where() clauses linked to the relationship() are respected, whereas the "relationship clauses" are not.

As highlighted by others, this behavior can be misleading and should at least be documented.

3rgo commented 1 year ago

FWIW this "bug" is still present in 10.20. My workaround was :

$related = $user->relation->first() ?: new Model;
// ...
// Do whatever with $related model
// ...
if(!$related->exists){
    $related->save();
    $related->users()->attach($user);
} else {
    $related->save();
}
themrcatsu commented 11 months ago

Hi there, is there any update? It still returns first record in table, even if related record exists, and im using it without any where conditions, just:

auth()->user()->books()->firstOrNew()