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

Add Postgres connection option to use the pgcrypto extension to generate UUIDs #3537

Merged
merged 12 commits into from Feb 27, 2019
Merged

Conversation

typokign
Copy link
Contributor

@typokign typokign commented Jan 29, 2019

The uuid-ossp module is effectively deprecated. It is unmaintained, and Postgres does not compile with the module included by default. Therefore, some installations of Postgres, as well as some popular docker images, do not include this module. Attempting to create an Entity with a PrimaryGeneratedColumn('uuid') will throw an error that the uuid_generate_v4() function is unavailable.

Visiting the Postgres documentation at https://www.postgresql.org/docs/9.4/uuid-ossp.html shows:

Note: If you only need randomly-generated (version 4) UUIDs, consider using the gen_random_uuid() function from the pgcrypto module instead.

The pgcrypto module is included by default in Postgres and offers the same functionality.

Solution: To maintain backwards compatibility, the extension to be used defaults to the old uuid-ossp extension. Users running an instance of Postgres that does not include this module can manually set the uuidExtension property on the Postgres connection options to pgcrypto. I strongly recommend changing the default to pgcrypto in the next breaking release.

@luozhihua
Copy link

I have encountered an error where the uuid_generate_v4() function is not available because the postgresql docker image I am using does not support the uuid-ossp module, so I hope to support pgcrypto as soon as possible. Thank you very much!

@typokign typokign closed this Jan 29, 2019
@typokign typokign reopened this Jan 29, 2019
@typokign
Copy link
Contributor Author

Travis build is acting up, forgive the open/close spam please :)

@typokign typokign closed this Jan 29, 2019
@typokign typokign reopened this Jan 29, 2019
@luozhihua
Copy link

@pleerock @AlexMesser @daniel-lang

I am sorry to bother, I am not sure who can promote this problem. I also don't know if this PR can be accepted and released. If it can be merged, it is very pleasant. If it can't be merged, please let us know so that I can seek other solutions as soon as possible.

I have used Typeorm + Postgresql to be perfect, but after I upgraded the Docker Postgresql image, Postgresql no longer supports the uuid-ossp module (meaning that the uuid_generate_v4() method called by Typeorm is no longer available), in fact this is not a problem with Typeorm. Instead, Postgresql abolished the interface, but we still hope that Typeorm can follow up on this issue, thanks to all those who contributed to Typeorm!

@typokign
Copy link
Contributor Author

@luozhihua You're welcome to import my forked version with the fix at dacruz21/typeorm, master branch. Of course I would recommend pinning the commit hash in case I were to slip some malware in without your knowledge (but I pinky promise I won't 😉). Not a permanent solution but hopefully one that will unblock you until this PR has undergone the scrutiny it requires.

@luozhihua
Copy link

@dacruz21 Thanks for your amazing work, I have Clone your branch yesterday and temporarily solved the problem that I can't operate the database's table containing the uuid field, but this is not a long-term solution, thank you again for your work.

@pleerock
Copy link
Member

pleerock commented Feb 9, 2019

doesn't postgres his own uuid generator in the latest versions? If no and we have to merge this PR, then we need to make sure it doesn't introduce a breaking change.

@pleerock
Copy link
Member

pleerock commented Feb 9, 2019

We already have same PR - #3176 . Please checkout exist conversation as well.

@typokign
Copy link
Contributor Author

We already have same PR - #3176 . Please checkout exist conversation as well.

This will not affect users who have existing UUID columns. New columns will be generated from a different function, but the underlying data type does not change, as postgres does not use a uuid column type. There will be no visible difference between old columns and new.

@typokign
Copy link
Contributor Author

doesn't postgres his own uuid generator in the latest versions? If no and we have to merge this PR, then we need to make sure it doesn't introduce a breaking change.

Define "own"? The pgcrypto module is included with all newer postgres installs, so although it needs to be imported, the import should never fail. I am not aware of any UUID generation function that does not need to be imported.

@typokign typokign closed this Feb 19, 2019
@typokign typokign reopened this Feb 19, 2019
@vlapo
Copy link
Contributor

vlapo commented Feb 19, 2019

We already have same PR - #3176 . Please checkout exist conversation as well.

This will not affect users who have existing UUID columns. New columns will be generated from a different function, but the underlying data type does not change, as postgres does not use a uuid column type. There will be no visible difference between old columns and new.

But if you use uuid-ossp extension in old version of typeorm and you migrate to new one and do not have pgcrypto installed you have to change something in your setup. --- this is main reason why to merge this into next major release as breaking change

@pleerock
Copy link
Member

I think best if we support both extension and add postgres connection option uuidExtension: "pgcrypto"|"uuid-ossp" defaulted to "uuid-ossp". This way everyone can used what they need without any breaking changes. In the future versions we'll switch default to "pgcrypto"

@typokign
Copy link
Contributor Author

I think best if we support both extension and add postgres connection option uuidExtension: "pgcrypto"|"uuid-ossp" defaulted to "uuid-ossp". This way everyone can used what they need without any breaking changes. In the future versions we'll switch default to "pgcrypto"

Sounds like a good plan - I'll throw that together this weekend and update the PR soon.

@@ -6,13 +6,16 @@ import {closeTestingConnections, createTestingConnections, reloadTestingDatabase
import {Post} from "./entity/Post";
import {Question} from "./entity/Question";

describe("uuid-postgres", () => {
describe("pgcrypto", () => {
Copy link
Contributor Author

@typokign typokign Feb 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff isn't exactly clear what I did here - I actually renamed the uuid-postgres tests to uuid-ossp, and configured the tests to use the uuid-ossp extension. Then I copied the uuid-ossp.ts file to pgcrypto.ts, left all the tests the same, and set the extension to pgcrypto. Looks like Git linked the old uuid-postgres file to the new pgcrypto file, but if you manually diff the two files you'll see that none of the tests have been changed.

@typokign typokign changed the title Use pgcrypto gen_random_uuid() instead of uuid-ossp uuid_generate_v4() Add Postgres connection option to use the pgcrypto extension to generate UUIDs Feb 24, 2019
@typokign
Copy link
Contributor Author

@pleerock @vlapo I believe this is ready for re-review. Thanks!

@vlapo
Copy link
Contributor

vlapo commented Feb 26, 2019

Looks fine for me. Just do not let this feature fall into forgotten and add some documentation https://github.com/typeorm/typeorm/blob/master/docs/connection-options.md

@pleerock
Copy link
Member

Looks great, thank you very much for this awesome contribution!

@pleerock pleerock merged commit 122601b into typeorm:master Feb 27, 2019
abingoal added a commit to abingoal/typeorm that referenced this pull request Feb 27, 2019
@abingoal abingoal mentioned this pull request Feb 27, 2019
@vlapo vlapo mentioned this pull request Mar 12, 2019
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

Successfully merging this pull request may close these issues.

None yet

4 participants