fix(pins)_: remove pins when the og message is deleted #6231
Conversation
Jenkins BuildsClick to see older builds (16)
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6231 +/- ##
===========================================
+ Coverage 61.54% 61.67% +0.13%
===========================================
Files 842 842
Lines 110598 110612 +14
===========================================
+ Hits 68067 68224 +157
+ Misses 34582 34420 -162
- Partials 7949 7968 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -1,3 +1,13 @@ | |||
| -- Delete pinned messages that are not associated with any user message | |||
| -- This makes sure the migration below works even if a message is deleted | |||
| DELETE FROM pin_messages | |||
There was a problem hiding this comment.
Hmm. So at the moment of running the migration, if we didn't yet receive the message, but we already received the pin_message, we will drop the information about the pinning action 🤔
There was a problem hiding this comment.
But this also means that further, if we receive the pin_message before the message itself (which is possible), we won't be able to save it into the pin_message table, because message_id foreign key will not be found. Sounds not good, but maybe (hopefully) I'm missing smth?
There was a problem hiding this comment.
Oh that's a great point, I didn't think about that! I guess we don't have a test covering that because it would have failed then.
I now realize that the original migration was already merged 😬
I wonder then if maybe I should revert it since there is that edge case you just found
There was a problem hiding this comment.
I wrote a test real quick that receives a Pin before the actual message and it fails in the query with FOREIGN KEY constraint failed, because like you said @igor-sirotin , the message is not found.
I'm not sure what's the best course of action. I can revert the migration or we can try to find a way to still save the pin when the message doesn't exist yet? cc @osmaczko since he proposed the FOREIGN KEY migration. Before that, I just had a query that deleted the pins when we deleted the message #6173 (comment)
There was a problem hiding this comment.
I didn't think of it either, sorry for that. Removing FOREIGN KEY constrain seems like a way to go.
There was a problem hiding this comment.
@igor-sirotin @osmaczko I removed the migration completely.
This means that people that had the issue with the previous migration will now be able to login correctly.
People that did run the migration previously will still be able to login as usual. However, they will have the edge case where if a pin message is received before the actual message, they will never see the pin, but that should be rare and more importantly, it will only be applied to dev accounts, never release accounts.
There is a way to fix it completely by applying the DELETE FROM pin_messages migration in this one and then creating a new migration that removes the CONSTRAINT.
What do you guys prefer?
There was a problem hiding this comment.
Removing migration completely sounds fine to me, as far as it affects only non-release users, as you mentioned.
There was a problem hiding this comment.
@jrainville Yeah removing sounds good to me 👍
But why did we add FOREIGN KEY in the first place? To remove the rows in cascade? Then I guess we need a replacement for this behaviour?
There was a problem hiding this comment.
But why did we add FOREIGN KEY in the first place? To remove the rows in cascade? Then I guess we need a replacement for this behaviour?
Yes you are right. I will add a new issue after this one to do the behaviour manually without needing the constraint.
Similar to what I had here #6173 (comment)
There was a problem hiding this comment.
Issue created here: #6246
@igor-sirotin @osmaczko can you give an approval to this one, I'll start working on the fix in a few.
jrainville
force-pushed
the
fix/pin-migration
branch
from
754e2cd to
8c73295
Compare
jrainville
force-pushed
the
fix/pin-migration
branch
from
8c73295 to
72a72bc
Compare
jrainville
changed the title
|
@igor-sirotin @osmaczko I actually put the fix in the PR directly and changed the title |
@jrainville I thought something is wrong here and was looking in panic for a bug in |
protocol/message_persistence.go
Outdated
| if err == nil { | ||
| err = tx.Commit() | ||
| return | ||
| } | ||
| // don't shadow original error | ||
| _ = tx.Rollback() |
There was a problem hiding this comment.
Not a big deal, but would be nice to apply this pattern here: Skip deferred error
There was a problem hiding this comment.
Done. Let me know if I did it correctly
jrainville
force-pushed
the
fix/pin-migration
branch
from
72a72bc to
a0a0075
Compare
|
@osmaczko reminder to review please |
EDIT: I completely removed the migration in the end since we found that it actually breaks a behaviour of receiving a pin before a message. I also fixed the original issue without the migration but by manually deleting the pins when the message is deleted.
Turns out if you had old pinned messages that were still associated with deleted user messages, then the original migration didn't work. This fixes it.If someone already ran the migration successfully because they didn't have deleted pinned messages, then it just won't run again and everything is already fine. If someone was blocked because of the migration, then it will just run fine now.Thanks to @alexjba for find the cause