[5.4][5.5] firstOrCreate(), updateOrCreate() excludes attributes on creation (when using $fillable)

[darron1217]

• Laravel Version: 5.4.36
• PHP Version: 7.1.6
• Database Driver & Version: MariaDB 10.1.25

Description:

updateOrCreate() excludes key attributes on insertion.

When I look into the framework code deeply, I found that updateOrCreate() uses firstOrNew() function internally.
The problem is firstOrNew() function uses newModelInstance() to get new Model instance. In this way, attribute which is not allowed by $fillable property would not be assigned to the model. (like st_id on my code below)

framework/src/Illuminate/Database/Eloquent/Builder.php

Lines 391 to 396 in b353719

	public function updateOrCreate(array $attributes, array $values = [])
	{
	return tap($this->firstOrNew($attributes), function ($instance) use ($values) {
	$instance->fill($values)->save();
	});
	}

framework/src/Illuminate/Database/Eloquent/Builder.php

Lines 357 to 364 in b353719

	public function firstOrNew(array $attributes, array $values = [])
	{
	if (! is_null($instance = $this->where($attributes)->first())) {
	return $instance;
	}
	return $this->newModelInstance($attributes + $values);
	}

Logically, search condition must be included in the record (especially on insertion).
So.. In my opinion, firstOrCreate(), firstOrNew(), updateOrCreate() should forceFill() the given attributes.

Steps To Reproduce:

When I run the code below

$record = new AttendanceRecord([
  'st_id' => 1,
  'rec_date' => '2017-09-06',
  'in_time' => '11:00',
  'out_time' => '14:00'
]);
// One Attendance hasMany records (attendance id = at_id)
$attendance->records()->updateOrCreate([
    'st_id' => $record->st_id,
    'rec_date' => $record->rec_date
], $record);

then I got SQL Error below

SQLSTATE[HY000]: General error: 1364 Field 'st_id' doesn't have a default value (SQL: insert into `attendance_records` (`at_id`, `in_time`, `out_time`, `updated_at`, `created_at`) values (1, 10:00, 17:00,  2017-09-06 15:58:40, 2017-09-06 15:58:40))

These are my Eloquent Model and Database

class AttendanceRecord extends Model
{
  protected $table = 'attendance_records';
  public $timestamps = true;

  protected $dates = [
    'rec_date',
  ];

  protected $fillable = [
    'in_time',
    'out_time',
  ];

  public function getInTimeAttribute($value)
  {
    $in_time = new Carbon($value);
    return $in_time->format('H:i');
  }

  public function getOutTimeAttribute($value)
  {
    $out_time = new Carbon($value);
    return $out_time->format('H:i');
  }
}

Schema::create('attendance_records', function(Blueprint $table) {
  $table->integer('at_id')->unsigned();
  $table->integer('st_id')->unsigned();
  $table->date('rec_date');
  $table->time('in_time');
  $table->time('out_time');
  $table->timestamps();
  $table->unique(['at_id', 'st_id', 'rec_date']);
});

[rodrigopedra]

@darron1217

I think this the intended behavior when using $fillable to protect your models of mass assignment.

If someone does something like this:

$attendance->records()->updateOrCreate([
    'st_id' => $record->st_id,
    'rec_date' => $record->rec_date
], request()->all());

It could lead to a security problem if you bypass the $fillable guard.

In your case, you could try to unguard the model before inserting if you are sure that input is safe:

AttendanceRecord::$unguarded = false;

$attendance->records()->updateOrCreate([
    'st_id' => $record->st_id,
    'rec_date' => $record->rec_date
], $record->getAttributes()); // also I think you want to pass the attributes as the values

AttendanceRecord::$unguarded = true;

[themsaid]

This would be a major breaking change if applied, and actually I don't think it should be applied at all, you should stop guarding all fields if you want this kind of flexibility.

Closing since it's not a bug but feel free to start a discussion on laravel/internals.

[darron1217]

@themsaid I think it is logical bug...
It is only designed for table which has auto-generating primary key...
'Search Condition' for my case, is 'multi column combined key' which is alternative for auto-generating key. This means it should not be included in $fillable, but should be included when inserting.
Please concern again for this issue.
Anyway, thanks for replying my complicated request :)

[avatarofhope2]

I think it would be good to be able to provide an argument to updateOrCreate to tell the underlying fill to use forceFill in cases where you are trying to get the system to create a record using data from a modified model instance.
for example, if a user wants to restore a soft-deleted record by creating a new record with the same unique key, there is user data coming from the request, and the system can set ->deleted_at = null, but using updateOrCreate would fail to update the deleted_at.