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

PostgreSQL dropPrimary does not work, if using prefix #42804

Closed
mihy opened this issue Jun 14, 2022 · 12 comments
Closed

PostgreSQL dropPrimary does not work, if using prefix #42804

mihy opened this issue Jun 14, 2022 · 12 comments

Comments

@mihy
Copy link

mihy commented Jun 14, 2022

  • Laravel Version: v9.17.0 (but also the same with any lower version
  • PHP Version: 8.1 (but PHP version does not take affect)
  • Database Driver & Version: PostgreSQL (14.3), but also does not take affect

Description:

I can not drop existing index in my database (PostgreSQL) with standard migration.
The problem persist, if using PostgreSQL and prefix is not empty string.
The problem is in file:

https://github.com/laravel/framework/blob/9.x/src/Illuminate/Database/Schema/Grammars/PostgresGrammar.php

Line: 355

It generates index name without prefix.

When primary key index name is generating, it leaves naming for PostgreSQL, even if passing optional parameter $index. Generated SQL is something like:

alter table "prefix_test" add primary key ("id")

and PostgreSQL creates index name: prefix_test_pkey

When executing $table->dropPrimary(), raw sql is:
alter table "prefix_test" drop constraint "test_pkey", it does not add prefix and so migration throws error that:

constraint "test_pkey" of relation "prefix_test" does not exist.

It should generate prefix_test_pkey

Optionally $table->primary() and $table->dropPrimary() allows to pass index name, but PostgresGrammar ignores it.

Steps To Reproduce:

  1. install Laravel
  2. change .env DB_CONNECTION to pgsql
  3. change config/database.php line 75 from 'prefix' => '', to 'prefix' => 'prefix_', (under pgsql)
  4. delete all files from database/migrations
  5. create migration file (php artisan make:migration create_test_table (see below)
  6. php artisan migrate (throws error)

file: XXXXXX_create_test_table.php

id(); $table->dropPrimary(); }); } /** * Reverse the migrations. * * @return void */ public function down() { } };
@mihy
Copy link
Author

mihy commented Jun 14, 2022

Actual code of migration file
`<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

return new class extends Migration {
/**
* Run the migrations.
*
* @return void
*/
public function up()
{
Schema::create('test', function (Blueprint $table) {
$table->id();
$table->dropPrimary();
});
}

/**
 * Reverse the migrations.
 *
 * @return void
 */
public function down()
{
}

};`

@driesvints
Copy link
Member

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!

@driesvints
Copy link
Member

I'll re-open this once you provided a repo.

@programster
Copy link

@driesvints I am experiencing a similar issue, whereby I am struggling to remove a primary key that has a custom constraint name. This happened because I am moving an existing database over to using Laravel, so its tables were not set up by Laravel itself.

I have never submitted a bug report for Laravel before, but hopefully I followed your instructions correctly on setting up a demonstraton repo, which can be found here:

https://github.com/programster/Laravel-bug-report-dropPrimary

They key commit demonstrating the bug is here:
programster/Laravel-bug-report-dropPrimary@7f1e774

I made sure to test it was still the case after setting up PostgreSQL running in a docker container and updating my .env file database settings to:

DB_CONNECTION=pgsql
DB_HOST=192.168.0.x
DB_PORT=5432
DB_DATABASE=bug_report
DB_USERNAME=laravel
DB_PASSWORD=laravel

@driesvints
Copy link
Member

@programster I think:

  1. You need to use the Schema builder, not a raw query
  2. You can't do those two operations in the same migration

@programster
Copy link

programster commented Jul 28, 2022

@driesvints the first query where I'm creating the table....

ksnip_tmp_JqFszZ

...is not supposed to show how I would have built it in Laravel, but to simulate the problematic table structure I have inherited by trying to move a legacy database over to Laravel. I am showing an example of how I am trying to fix it by using dropPrimary() in the second statement.

I don't think I need to split the operations into two migrations to demonstrate the bug, as the db statement query executes immediately, and it is the one that I need to execute first. If it makes you happier to demonstrate the point though, I would be happy to do so....

When I provide the name of the constraint to dropPrimary...

ksnip_tmp_tQZtRA

...it should be using that name in the query it uses to try and drop the constraint, instead of using its own derived name from looking at the table name.

@yasirmturk
Copy link

👍 I have the same issue

@programster
Copy link

programster commented Aug 5, 2022

I'll re-open this once you provided a repo.

@driesvints can yoiu re-open this issue now that you have the example info you asked for. Let me know if something is still outstanding.

@driesvints
Copy link
Member

I'm sorry, but I don't think you should be using raw queries in that situation. Right now I don't have intention of reopening this.

@programster
Copy link

programster commented Aug 8, 2022

I'm sorry, but I don't think you should be using raw queries in that situation. Right now I don't have intention of reopening this.

Your missing the point. The raw query in that case was simply to set up the state that the database is already in! Not a demonstration of the user using raw queries.

What is the developer supposed to do if they are converting an existing codebase over to using Laravel, and the database (from the legacy system) already has primary keys that have an index name that Laravel is not expecting? This is what the sample codebase is trying to simulate to demonstrate the problem. At the moment, the developer is forced to use raw queries to drop the primary key to then "fix" the database so that they can use Schema in future, when they would rather just use Schema in their migrations without having to resort to using raw.

@warlof
Copy link

warlof commented Aug 19, 2023

Hello,

I discover the exact same issue on my database using Postgres 15 and Laravel 10.

According to me, issue seems to come from this place

public function compileDropPrimary(Blueprint $blueprint, Fluent $command)
{
$index = $this->wrap("{$blueprint->getTable()}_pkey");
return 'alter table '.$this->wrapTable($blueprint)." drop constraint {$index}";
}

public function compileDropPrimary(Blueprint $blueprint, Fluent $command)
{
$index = $this->wrap("{$blueprint->getTable()}_pkey");
return 'alter table '.$this->wrapTable($blueprint)." drop constraint {$index}";
}

In case a key name was provided, it's not used and hardcooded with current table name suffixed with _pkey

In my case, I had a table that I renamed.
The table was properly renamed - but the primary key was still using the previous table name as a constraint name prefix.

    public function up()
    {
        Schema::rename('corporation_mining_observer_data', 'corporation_industry_mining_observer_data');
    }

With this simple migration, I moved the table from corporation_mining_observer_data to corporation_industry_mining_observer_data

image

So now, if I try to drop the primary key using this code:

    public function up()
    {
        Schema::table('corporation_industry_mining_observer_data', function (Blueprint $table) {
            $table->dropIndex('corporation_mining_observer_data_pkey');
            $table->primary(['corporation_id', 'observer_id', 'recorded_corporation_id', 'character_id', 'type_id', 'last_updated'],
                'obeserver_data_primary');
        });
    }

Laravel will replaced the provided name corporation_mining_observer_data_pkey with corporation_industry_mining_observer_data_pkey

And of course, it crash because the constraint corporation_industry_mining_observer_data_pkey does not exist in the database.

@hafezdivandari
Copy link
Contributor

Fixed on Laravel 11.x via #50019

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

No branches or pull requests

6 participants