Change emails foreign key - 2 #3896
Conversation
|
I tried to modify all reads which use |
| SELECT p.username | ||
| FROM emails e INNER JOIN participants p | ||
| ON e.participant_id = p.id | ||
| WHERE e.address = %(email)s |
There was a problem hiding this comment.
ok so that's what the uneven indentation at the start of the line is about! I was scratching my head with that. Man, now you're just messing with a java guy. 😏
There was a problem hiding this comment.
Regardless, I'll admit that this does look cleaner. Fixing.
There was a problem hiding this comment.
There was a problem hiding this comment.
Nice! Should answer this 😄
I hope our tests are exhaustive enough to catch this... Try removing the If we do encounter errors in production, we'll patch them and add tests so that they never happen again. |
|
Running test. |
| AND address=%s | ||
| AND verified IS NULL | ||
| RETURNING nonce | ||
| """, (verification_start, self.username, email)) | ||
| """, (verification_start, self.id, email)) | ||
| if not nonce: | ||
| return self.add_email(email) | ||
|
|
There was a problem hiding this comment.
@rohitpaulk tests are failing. 😞 All the email tests get stuck in this recursive call. Do you have any idea why? I have dropped the participant column and modified email writes to only include participant_id
There was a problem hiding this comment.
Can you share a diff of the changes you made vs the code in this PR?
There was a problem hiding this comment.
diff --git a/gratipay/models/participant.py b/gratipay/models/participant.py
index 242ebff..e64c508 100644
--- a/gratipay/models/participant.py
+++ b/gratipay/models/participant.py
@@ -410,9 +410,9 @@ class Participant(Model):
add_event(c, 'participant', dict(id=self.id, action='add', values=dict(email=email)))
c.run("""
INSERT INTO emails
- (address, nonce, verification_start, participant, participant_id)
- VALUES (%s, %s, %s, %s, %s)
- """, (email, nonce, verification_start, self.username, self.id))
+ (address, nonce, verification_start, participant_id)
+ VALUES (%s, %s, %s, %s)
+ """, (email, nonce, verification_start, self.id))
except IntegrityError:
nonce = self.db.one("""
UPDATE emailsThere was a problem hiding this comment.
diff --git a/gratipay/models/participant.py b/gratipay/models/participant.py
index 242ebff..e64c508 100644
--- a/gratipay/models/participant.py
+++ b/gratipay/models/participant.py
@@ -410,9 +410,9 @@ class Participant(Model):
add_event(c, 'participant', dict(id=self.id, action='add', values=dict(email=email)))
c.run("""
INSERT INTO emails
- (address, nonce, verification_start, participant, participant_id)
- VALUES (%s, %s, %s, %s, %s)
- """, (email, nonce, verification_start, self.username, self.id))
+ (address, nonce, verification_start, participant_id)
+ VALUES (%s, %s, %s, %s)
+ """, (email, nonce, verification_start, self.id))
except IntegrityError:
nonce = self.db.one("""
UPDATE emails
diff --git a/sql/branch.sql b/sql/branch.sql
index 4a74cea..f316636 100644
--- a/sql/branch.sql
+++ b/sql/branch.sql
@@ -2,4 +2,6 @@ UPDATE emails AS e
SET participant_id = p.id
FROM participants AS p
WHERE p.username = e.participant
-AND e.participant_id IS NULL;
\ No newline at end of file
+AND e.participant_id IS NULL;
+
+ALTER TABLE emails DROP COLUMN participant;That's weird... I applied the above diff and all tests are passing for me.
There was a problem hiding this comment.
What's your test output? Which test is getting stuck in the loop?
There was a problem hiding this comment.
Hmm. The Integrity error is being hit, which means a record exists. If a record exists though, we shouldn't be hitting the recursive loop...
Given that the loop exited right there, we should be able to inspect the DB and see what's happening.. Open psql and try? Let's make sure we have the same setup though... here's mine
$ psql gratipay-test
gratipay-test=# \d emails
Table "public.emails"
Column | Type | Modifiers
--------------------+--------------------------+-----------------------------------------------------
id | integer | not null default nextval('emails_id_seq'::regclass)
address | text | not null
verified | boolean |
nonce | text |
verification_start | timestamp with time zone | not null default now()
verification_end | timestamp with time zone |
participant_id | bigint |
Indexes:
"emails_pkey" PRIMARY KEY, btree (id)
"emails_address_verified_key" UNIQUE CONSTRAINT, btree (address, verified)
"emails_participant_id_address_key" UNIQUE CONSTRAINT, btree (participant_id, address)
Check constraints:
"verified_cant_be_false" CHECK (verified IS NOT FALSE)
Foreign-key constraints:
"emails_participant_id_fkey" FOREIGN KEY (participant_id) REFERENCES participants(id) ON UPDATE RESTRICT ON DELETE RESTRICT
gratipay-test=#There was a problem hiding this comment.
ok I have no idea what the problem was but I couldn't even run the site locally at localhost:8537. make run kept exiting. So I restarted my PC and now everything works! All tests pass. 💃
There was a problem hiding this comment.
Aha. So this is ready to go, eh?
There was a problem hiding this comment.
Aha! I see. I applied your diff before running test. participant was still existing in gratipay-test. I had no idea branch.sql is run by the test suite. Figured it'd be like rails where the test database is created from the current schema.
There was a problem hiding this comment.
Aha. So this is ready to go, eh?
Yes! 💃
|
@rohitpaulk on a side note, why is that recursive call there anyway? If record failed to be created once because of a reason like this, it'll surely keep failing. |
|
Okay we need one last thing - we want to constrain this |
|
aandis
force-pushed
the
change-emails-foreign-key-2
branch
from
1c73db5 to
e94593c
Compare
The recursive call handles a specific race condition, I think. What if an email address existed - and was unverified before the
I do get that the code can be kinda extremely hard to read though... """
This is called when
1) Adding a new email address
2) Resending the verification email for an unverified email address
Returns the number of emails sent.
"""This might benefit from being split into two separate functions (and extracting common stuff out into different functions), seems to me like we're trying to stuff too much logic in here. |
|
Okay, this looks good to me. I'll try deploying this tomorrow. If anyone gets to this before me, keep in mind that we should run the migration script before deploying code. |
|
👍 |
Change emails foreign key - 2
|
@rohitpaulk Did you get into any issues while running the migration for copying values in production? |
Nope, smooth sailing ⛵ |
|
@rohitpaulk awesome graphical analysis. I guess that the bugs like this is the primary use case that is solved by Flux architecture - https://facebook.github.io/flux/docs/overview.html |
This is the next step after #3893. Pr includes a migration to fill the new
emails.participant_idcolumn with existing data and modifies email reads to then read from the new column instead ofemails.participant.