laravel / ideas

Issues board used for Laravel internals discussions.
940 stars 28 forks source link

Custom Cast/convert in Eloquent #992

Open gogromat opened 6 years ago

gogromat commented 6 years ago

I am trying to figure out a way to save binary/varbinary MSSQL types with Eloquent (sqlsrv driver + doctrine/dbal v2.5.5). So far I figured out a way to prepare statement with conversion and a placeholder, but it involves manually calling DB::statement, instead of $model->save(), which is much more desirable.

DB::statement(
    "INSERT INTO files (file_name, content, ...) VALUES (?, CONVERT(VARBINARY(MAX) ?), ...));", [
        $file->file_name,
        base64_encode(file_get_contents($file->file_path))
]);

Notice this is a prepared statement, I call CONVERT(VARBINARY(MAX), ?) and then I feed my values afterwards. If I do it using something like setContentAttribute on a model and have it defined as

function setContentAttribute ($value) {
    $this->attributes['content'] = DB::raw("CONVERT(VARBINARY(MAX), '" . base64_encode($value) . "')");
}

it has two drawbacks:

Looking at the code I found this:

Query\Builder :: insert

        // Finally, we will run this query against the database connection and return
        // the results. We will need to also flatten these bindings before running
        // the query so they are all in one huge, flattened array for execution.
        return $this->connection->insert(
            $this->grammar->compileInsert($this, $values),
            $this->cleanBindings(Arr::flatten($values, 1))
        );

Grammars\Grammar :: compileInsert

        // We need to build a list of parameter place-holders of values that are bound
        // to the query. Each insert should have the exact same amount of parameter
        // bindings so we will loop through the record and parameterize them all.
        $parameters = collect($values)->map(function ($record) {
            return '('.$this->parameterize($record).')';
        })->implode(', ');

        return "insert into $table ($columns) values $parameters";

Grammars\Grammar :: parameterize and parameter

  /**
     * Create query parameter place-holders for an array.
     *
     * @param  array   $values
     * @return string
     */
    public function parameterize(array $values)
    {
        return implode(', ', array_map([$this, 'parameter'], $values));
    }

    /**
     * Get the appropriate query parameter place-holder for a value.
     *
     * @param  mixed   $value
     * @return string
     */
    public function parameter($value)
    {
        return $this->isExpression($value) ? $this->getValue($value) : '?';
    }

So parameters can be easily parametarised, I can actually have my setContentAttribute, which should set $this->attributes['content'] = DB::raw("CONVERT(VARBINARY(MAX), ?)");, but then I would need to get the value from the Expression as well. So far the

Query\Builder :: cleanBindings just ignores Expression instance.

    /**
     * Remove all of the expressions from a list of bindings.
     *
     * @param  array  $bindings
     * @return array
     */
    protected function cleanBindings(array $bindings)
    {
        return array_values(array_filter($bindings, function ($binding) {
            return ! $binding instanceof Expression;
        }));
    }

From what I sort of understand we can modify Expression to not only have getValue, but also have something like getParameter, switch the logic around and make Expression->getParamter return CONVERT(VARBINARY(MAX), ?) and a value as Expression->getValue return actual string value.

What do you think?

andrey-helldar commented 4 years ago

@gogromat, if I understand your problem correctly, the changes proposed in laravel/framework#30958 will make it easy to solve.

sisve commented 4 years ago

@andrey-helldar It seems like the problem is deeper than the actual interpretation of the values. Can your suggested PR modify the generated sql statements, which seems relevant for this issue?

andrey-helldar commented 4 years ago

@sisve, no, it cannot change the generated SQL, but it can, on the application side, convert the value to the view needed by the developer.

sisve commented 4 years ago

@andrey-helldar You say "convert the value to the view", but this issue is about inserting data to the database. It's not entirely clear what the original problem was, but I read this issue as a mssql-specific problem with data-types.

@gogromat Do you remember what the problem was, or how you ended up solving this?

andrey-helldar commented 4 years ago

@sisve, The reason for the misunderstanding was probably the language barrier, since English is not my native language and I could misunderstand the meaning of the question.

orim181 commented 4 years ago

I second this request. Here's another example plaguing me with SQL Server, Laravel 5.8, and the MSSQL PHP Driver:

I get this error when attempting to save a small decimal value like 0.00001 into a decimal(15,6) column. The MSSQL PHP driver will generate this kind of UPDATE statement: UPDATE [table] SET [amount] = '1.0E-5' WHERE id=1 not UPDATE [table] SET [amount] = '0.00001' WHERE id=1 which is what we would expect.

Now, the problem is that it's actually a bad statement because I will get this error: Error converting data type nvarchar to numeric. (SQL: update [table] set [amount] = '1.0E-5' where id=1)

The only workaround I found for this is to leverage SQL's format/convert functions so the correct statement can look like this: update [table] set [amount] = format(convert(float,'1.0E-5'),'0.0#########')

However, to do that I need to override the SQL casting inside Eloquent which I can't.

bcalik commented 4 years ago

Adding a DB::raw query to setContentAttribute is not a good solution because it has many drawbacks that you mentioned some of them.

A new casting feature for DB queries may solve the issue. We would add setContentQuery and getContentQuery kind of methods, and query builder would call this before running the query. set would be used for insert, update, where queries that containing a value, and get would be used for select queries.

So you could do things like DB::raw("CONVERT(VARBINARY(MAX), '$value'") in setContentQuery, then the actual $attributes variable and rest of the application would not be affected.

Anyone with a deep knowledge of Laravel's Query Builder may try this solution.