-
-
Notifications
You must be signed in to change notification settings - Fork 89
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
Use UUIDs as indentifiers for sessions in public #176
Conversation
@yshmarov Do you have an opinion on this? Also very okay if you don't want to bother, just let me know 😅 |
Heyyy @mikker, I'm loving this PR! 🤗 Having to add postgres plugins as dependencies is not cool very; it's much better if the gem is database-agnostic 👍 I am happy with adding an additional |
@@ -18,9 +18,11 @@ | |||
t.datetime "expires_at", precision: nil, null: false | |||
t.datetime "claimed_at", precision: nil | |||
t.string "token_digest", null: false | |||
t.string "identifier", null: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, with null: false
it might be worth considering column "identifier" of relation "passwordless_sessions" contains null values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should go with null: false
as the new default in new installations but null: true
in the upgrade instructions? That way it'll handle legacy tables and Rails can still validates :presence
f2d888e
to
967562a
Compare
As per discussion with @yshmarov in #169, a proposal to use UUIDs as Session identifiers in public.
It might be a bit more space and cpu optimized to use UUIDs as the primary keys directly but AFAIK that involves having to turn on a Postgres plugin
pgcrypto
guides ref.I feel like that puts unnecessary hassle on the user's end re: using DBs you don't control, etc. I'm also hesitant to add another required migration upgrade.
This way we can do the switch with only a vaguely breaking change (lol, if that's a thing)
@yshmarov what do you think?