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

Allow Schema to work with the same JSON grammar as Query #36343

Closed
LarsGrevelink opened this issue Feb 22, 2021 · 2 comments
Closed

Allow Schema to work with the same JSON grammar as Query #36343

LarsGrevelink opened this issue Feb 22, 2021 · 2 comments

Comments

@LarsGrevelink
Copy link
Contributor

LarsGrevelink commented Feb 22, 2021

  • Laravel Version: 8.22.1
  • PHP Version: 7.4.15
  • Database Driver & Version: SQLite 3.34.0

Description:

I doubted whether this should be marked as a bug or a feature request. Feel free to close it when it is the latter, added it to the Laravel Ideas repo in case we want to treat it that way; laravel/ideas#2500

We are working with Laravel's migrations and are using these to set up both our local databases (MySQL) and our testing database (SQLite). While doing so we encountered a difference between the query and schema grammar with handling JSON columns, in this case with virtual columns.

In the current situation, the expression is left untouched. Using either -> or ->> makes it work for MySQL databases but breaks it for SQLite because of this. It would be great if we could use the grammar applied to queries on the schema side as well.

Steps To Reproduce:

The migration we ran was similar to this;

Schema::table('my_table', function (Blueprint $table) {
    $table->string('my_extracted_value')->virtualAs('json_field->extract_me');
});

Which resulted in;

alter table "my_table" add column "my_extracted_value" varchar as (json_field->extract_me)

-- Illuminate\Database\QueryException: SQLSTATE[HY000]: General error: 1 near ">": syntax error (SQL: alter table "my_table" add column "my_extracted_value" varchar as (json_field->extract_me))

Suggestion:

In case there was no intent for the virtual as value to be left untouched, would you be open to moving the following functions into Illuminate\Database\Grammar or Illuminate\Database\Schema\Grammar;

  • Illuminate\Database\Query\Grammars->isJsonSelector
  • Illuminate\Database\Query\Grammars->wrapJsonSelector
  • Illuminate\Database\Query\Grammars->wrapJsonFieldAndPath
  • Illuminate\Database\Query\Grammars->wrapJsonPath

That way the same logic can be applied in both the query and schema grammar cases. For SQLite we could do something similar to this;

// Illuminate\Database\Schema\Grammars\SQLiteGrammar
protected function modifyVirtualAs(Blueprint $blueprint, Fluent $column)
{
    if (! is_null($virtualAs = $column->virtualAs)) {
        if ($this->isJsonSelector($virtualAs)) {
            $virtualAs = $this->wrapJsonSelector($virtualAs);
        }

        return " as ({$virtualAs})";
    }
}

Would love to make a PR for this change but wanted to check whether there was an additional reason for the current functionality. If you have more locations besides the virtual columns for SQLite where we should change this I would love to take this along as well.

Cheers!

@driesvints
Copy link
Member

Heya, just feel free to attempt a PR if you want to change this 👍

@LarsGrevelink
Copy link
Contributor Author

Perfect - will do! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants