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

Bug: PostgreSQL driver issues #4142

Closed
psuplat opened this issue Jan 21, 2021 · 7 comments · Fixed by #4147
Closed

Bug: PostgreSQL driver issues #4142

psuplat opened this issue Jan 21, 2021 · 7 comments · Fixed by #4147
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@psuplat
Copy link

psuplat commented Jan 21, 2021

I have had this bug (well, I think it is a bug anyways - I was wrong before, plenty of times) for a while now, but was to lazy to even report it. I was just kinda working around it.

I am facing a constant issue with migrations when using postgresql.

The setup is:
Apache 2.4.46
PHP 7.4.14
PostgreSQL 13.1

My config files setup:

public $prod = [
	'DSN'      => '',
	'hostname' => '',
	'username' => '',
	'password' => '',
	'database' => '',
	'DBDriver' => '',
	'DBPrefix' => '',
	'pConnect' => false,
	'DBDebug'  => (ENVIRONMENT !== 'production'),
	'cacheOn'  => false,
	'cacheDir' => '',
	'charset'  => 'utf8',
	'DBCollat' => 'utf8_general_ci',
	'swapPre'  => '',
	'encrypt'  => false,
	'compress' => false,
	'strictOn' => false,
	'failover' => [],
];

My .env setup:

database.prod.hostname = localhost
database.prod.database = playground
database.prod.username = postgres
database.prod.password = MyPasswordGoesHere
database.prod.DBDriver = Postgre
database.prod.DBDebug = TRUE
database.prod.schema = public
database.prod.port = 5432
database.prod.DSN = 'pgsql:dbname=playground;port=5432;host=localhost'

The above setup works perfectly fine when running the app. Fetching/inserting/updating data using models or query builder is smooth and fast - no issues.

However when I try to do migration here is where the problems start:
php spark migrate results in the below:
image

The workaround for this problem is to comment out the DSN part of the .env file, like so:
#database.prod.DSN = 'pgsql:dbname=playground;port=5432;host=localhost'

When I do the above, the migrations run almost perfectly. Obviously, this kills the app itself, so once I am done with migration I need to un-comment the DSN line in order to have the app working.

Now, the reason I have written almost over there, is because this work fine the first time I run migration. However when I add new migration file I get the below:
image

Here's how my migration files look like:
image

and here is the migration table in DB:
image

As you can see, from the above screenshots, all of my previous migration files are in the database, all that is missing is the last file in the list 2021-01-20 170310_approval_workflow.php

So, why on earth is the CI trying to run the migration again for those that have clearly already been run?

@psuplat psuplat added the bug Verified issues on the current code behavior or pull requests that will fix them label Jan 21, 2021
@michalsn
Copy link
Member

Not sure about the first part (I would probably have to run some tests locally) but about the second - you are using a different connection group. default is in your config file and prod is in your table with migrations. That's why CodeIgniter is trying to run all the migrations one more time - migrations history is unique per connection group.

@psuplat
Copy link
Author

psuplat commented Jan 21, 2021

Ah, crap - sorry I've paste the wrong part of the code, probably because they are the same, this is still a playground, prod does not really means production in this case. I have amended the above, however I do not see this being the issue, since in each migration file I specify which connection group to use = 'prod'

image

@michalsn
Copy link
Member

Okay, thanks for the explanation and more details.

I made some tests on the latest develop and I couldn't reproduce these connection errors.

I have some questions though. Why commenting DSN variable is killing your app? It has the same values as other config variables. If you don't provide a DSN string it gonna be build automatically from other config variables. The interesting part is why are you not providing user and password to the database in the DSN string. I guess there is a way to set up these via environment variables somehow? I'm not familiar with it, but I guess this might be a fault (so it's not a CodeIgniter issue).

As for a second problem with running migrations, I was able to reproduce a problem and will send a PR with a fix. It would be great if you could test it later.

@psuplat
Copy link
Author

psuplat commented Jan 24, 2021

@michalsn - thanks. During the week I will run some tests and let you know the outcome

@psuplat
Copy link
Author

psuplat commented Jan 24, 2021

Ok.

The fix works perfectly - all migrations run without trying to recreate the tables!

Now, on to the subject of the connection string. As per user manual:

Some database drivers (such as PDO, PostgreSQL, Oracle, ODBC) might require a full DSN string to be provided.

I have amended the string to include username & password, just in case, even though manual says:

If you provide a DSN string and it is missing some valid settings (e.g. the database character set), which are present in the rest of the configuration fields, CodeIgniter will append them

'pgsql:dbname=playground;port=5432;host=localhost;user=postgres;password=MyPasswordGoesHere'

but the results are the same. If I try to run migration with the dsn value it crashes:

Running all new migrations...
An uncaught Exception was encountered

Type:        CodeIgniter\Database\Exceptions\DatabaseException
Message:     Unable to connect to the database.
Filename:    /home/psuplat/Projects/Private/wuk-corporate/backend-api/vendor/codeigniter4/framework/system/Database/BaseConnection.php
Line Number: 425

When I comment out the dsn value, then the migration runs ok, but the app crashed when trying to reach any route:

"title": "PDOException",
 "type": "PDOException",
 "code": 500,
 "message": "invalid data source name",
 "file": "/home/psuplat/Projects/Private/wuk-corporate/backend-api/app/Libraries/CustomOauthStorage.php",
 "line": 65

@michalsn
Copy link
Member

The fix works perfectly - all migrations run without trying to recreate the tables!

Ok, thanks.

Now, on to the subject of the connection string. As per user manual:

Some database drivers (such as PDO, PostgreSQL, Oracle, ODBC) might require a full DSN string to be provided.

I have amended the string to include username & password, just in case, even though manual says:

If you provide a DSN string and it is missing some valid settings (e.g. the database character set), which are present in the rest of the configuration fields, CodeIgniter will append them

We should fix the description for this note because it's valid only for the universal version of DSN (URL-like form) - which is mentioned right before the box with a note.

'pgsql:dbname=playground;port=5432;host=localhost;user=postgres;password=MyPasswordGoesHere'

but the results are the same. If I try to run migration with the dsn value it crashes:

Well... this is a mystery to me. Sadly I can't recreate it locally. Would you be able to add dd($this->DSN) here and check the results for each case? With DSN value and without.

When I comment out the dsn value, then the migration runs ok, but the app crashed when trying to reach any route:

This error is strange because on a framework level we don't support PDO at all right now. Can you share what is happening on line 65 in Libraries/CustomOauthStorage.php file? My guess is that you're making a separate connection to the database via PDO in this file using DSN value from the config file. If that's the case, you should reuse the connection that already exists or use DSN from the database class since this one from the config file will be empty.

str_repace(' ', ';', db_connect()->DSN);

@psuplat
Copy link
Author

psuplat commented Jan 24, 2021

🤬🤬🤬🤬🤬

@michalsn what you said about not supporting PDO and making separate connection set of a light bulb in my head.
I am using a 3rd party library for oauth, which requires a PDO connection. I checked it again, and in the bit of code where I am building the pdo connection string I forgot to adjust for changes I made in the .env along the way, so mea culpa, mea culap, mea máxima culpa.

Since the migration issue is fixed by your commit, and connection was purely my fault, I will close this issue.

@psuplat psuplat closed this as completed Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants