Fixed #27996 - Added crypto extension and function to contrib.postgres #8265
Conversation
docs/releases/2.0.txt
Outdated
There was a problem hiding this comment.
I think this deserves 2 bullet points. One for RandomUUID and one for pgcrypto.
There was a problem hiding this comment.
Can you please leave this line at the end or make sure that the comment above still goes with the CreateExtension('uuid-ossp') line.
There was a problem hiding this comment.
You're right I alphabetical ordered the operation but I forgot to move the comment too. I'm fixing it.
There was a problem hiding this comment.
Why do you need this sleep function? If RandomUUID returns the same UUID for two consecutive calls at the same second that seems to be a bug in PostgreSQL.
There was a problem hiding this comment.
Actually it's only a dead code I forgot to delete, I'm going to remove it.
There was a problem hiding this comment.
I think you can omit (random) here.
There was a problem hiding this comment.
This needs to state that it needs the extension installing.
Also wouldn't hurt to include the canonical example code.
Even better, let's test whether UUIDField(default=RandomUUID) works - if it does let's document that both here and in the UUID docs as a note for PG users. (sounds like it doesn't but let's verify)
There was a problem hiding this comment.
This test would be better if we updated the whole model with the expression, like in the PR
docs/releases/2.0.txt
Outdated
There was a problem hiding this comment.
Thanks, I'm going to fix it.
django/contrib/postgres/functions.py
Outdated
There was a problem hiding this comment.
I think you could use _output_field = UUIDField() at the class level instead of this?
There was a problem hiding this comment.
That's slightly too much magic than I'd like, I think. Thoughts on that @jarshwah?
There was a problem hiding this comment.
I used the same convention that I found in the same file for TransactionNow function.
There was a problem hiding this comment.
Then it's settled, I think. Thanks :)
django/contrib/postgres/functions.py
Outdated
There was a problem hiding this comment.
That's slightly too much magic than I'd like, I think. Thoughts on that @jarshwah?
There was a problem hiding this comment.
I believe referencing INSTALLED_APPS here is not necessary.
There was a problem hiding this comment.
I tried to align this note to the same note I found into lookups.txt
There was a problem hiding this comment.
Hmm ... Could you stick to the style of the current file?
There was a problem hiding this comment.
"To use RandomUUID you have to have the pgcrypto extension installed."
There was a problem hiding this comment.
But I'm happy to defer to @timgraham for suggestions.
There was a problem hiding this comment.
I'm going to modify it as you suggested.
|
I'm waiting for the tests to be run on all databases. |
|
All checks have passed, I hope this will merged. |
timgraham
force-pushed
the
pgcrypto
branch
3 times, most recently
from
ddaf9bc to
8cd8f59
Compare
6 participants
Added CryptoExtension (pgcrypto) extension and RandomUUID
(GEN_RANDOM_UUID) function to contrib.postgres
Ticket #27996