Skip to content

Wrapped fieldname to solve SQL Error when sorting #16044

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

Merged
merged 5 commits into from
Apr 15, 2025

Conversation

jeroendeleur
Copy link
Contributor

Wrapped fieldname with backticks, because when the primary key of a model is a reserved word, MySql will throw an error.

Description

When the model has a reserved word as the PK e.g. key sorting records will throw the following SQL error.

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'key = 'cZKBUpPyUR' then 1 when key = 'EQ5zh0Eqhf' then 2 when key = 'lwmhTQ8C...' at line 1 (Connection: mysql, SQL: update age_groups set sort = case when key = 'cZKBUpPyUR' then 1 when key = 'EQ5zh0Eqhf' then 2 when key = 'lwmhTQ8Cjd' then 3 when key = '0XaE8LzBZN' then 4 when key = 'tUkKkfH3P2' then 5 when key

I've added backticks around $modelKeyName on line 47 of the CanReorderRecords concern, this will solve the issue.

Visual changes

No visual changes

Functional changes

  • [ v] Code style has been fixed by running the composer cs command.
  • [ v] Changes have been tested to not break existing functionality.
  • [ v] Documentation is up-to-date.

Wrapped fieldname with backticks, because when the primary key of a model is a reserved word, MySql will throw an error.
@danharrin
Copy link
Member

I think this will break every other driver apart from MySQL? right?

@jeroendeleur
Copy link
Contributor Author

I think this will break every other driver apart from MySQL? right?

This is a fair point. Is it an idea to replace it with upsert?

Replacing

$model
    ->newModelQuery()
    ->whereIn($modelKeyName, array_values($order))
    ->update([
        $orderColumn => DB::raw(
            'case ' . collect($order)
                ->map(fn ($recordKey, int $recordIndex): string => 'when `' . $modelKeyName . '` = ' . DB::getPdo()->quote($recordKey) . ' then ' . ($recordIndex + 1))
                ->implode(' ') . ' end'
        ),
    ]);

with

$collection = collect($order)
                ->map(fn ($recordKey, int $recordIndex) => [$modelKeyName => $recordKey, $orderColumn => $recordIndex]);
$model->upsert($collection->toArray(), [$modelKeyName], [$orderColumn]);

@danharrin
Copy link
Member

Hmm I'm not sure I've seen upsert used in this way. Maybe get the query grammar object, and then use invade($grammar)->wrapValue($string) to wrap the string in the correct quotes for that database engine?

@danharrin danharrin added the bug Something isn't working label Apr 13, 2025
@danharrin danharrin modified the milestones: v2, v3 Apr 13, 2025
@jeroendeleur
Copy link
Contributor Author

jeroendeleur commented Apr 15, 2025

That was my first aproach, but google sent me through the doctrine/dbal rabbit hole. I have changed the code to:

$wrappedModelKeyName = DB::connection()->getSchemaGrammar()->wrap($modelKeyName);

And then used the new variable in the raw SQL.

I have tested it on, MySql, MariaDB, SQLite and Postgres drivers, not on SQL server as I don't have a test bench for that.

@jeroendeleur jeroendeleur changed the title Wrapped fieldname with backticks to solve SQL Error Wrapped fieldname to solve SQL Error when sorting Apr 15, 2025
@github-project-automation github-project-automation bot moved this from Todo to In Progress in Roadmap Apr 15, 2025
@danharrin
Copy link
Member

Thanks for fixing it!

@danharrin danharrin merged commit e76fdea into filamentphp:3.x Apr 15, 2025
14 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Roadmap Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants