-
-
Notifications
You must be signed in to change notification settings - Fork 931
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
chore: fix missing index on courier list count #3002
Changes from all commits
6069360
cf156c7
f7fe751
99ef939
e70720a
cb66eaf
aaf49e5
aa80826
c6f8da6
815a593
31cc37b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -126,12 +126,60 @@ func TestPersister(ctx context.Context, newNetworkUnlessExisting NetworkWrapper, | |
assert.Equal(t, messages[len(messages)-1].ID, ms[0].ID) | ||
|
||
t.Run("on another network", func(t *testing.T) { | ||
_, p := newNetwork(t, ctx) | ||
ms, tc, _, err := p.ListMessages(ctx, filter, []keysetpagination.Option{}) | ||
nid1, p1 := newNetwork(t, ctx) | ||
ms, tc, _, err := p1.ListMessages(ctx, filter, []keysetpagination.Option{}) | ||
|
||
require.NoError(t, err) | ||
require.Len(t, ms, 0) | ||
require.Equal(t, int64(0), tc) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what a failure of the test case looks like: https://github.com/ory/kratos/actions/runs/3856498251/jobs/6572792385#step:15:3563 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's terrifying. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, luckily this was very unlikely, as there had to have been three preconditions:
|
||
// Due to a bug in the pagination query definition, it was possible to retrieve messages from another `network` | ||
// using the pagination query. That required that 2 message's `created_at` timestamps were equal, to trigger | ||
// the `OR` clause of the paginated query. | ||
// This part of the tests "simulates" this behavior, by forcing the same timestamps on multiple messages across | ||
// different networks. | ||
nid2, p2 := newNetwork(t, ctx) | ||
const timeFormat = "2006-01-02 15:04:05.99999" | ||
msg1 := courier.Message{ | ||
ID: uuid.FromStringOrNil("10000000-0000-0000-0000-000000000000"), | ||
NID: nid1, | ||
Status: courier.MessageStatusProcessing, | ||
} | ||
err = p1.GetConnection(ctx).Create(&msg1) | ||
require.NoError(t, err) | ||
|
||
msg2 := courier.Message{ | ||
ID: uuid.FromStringOrNil("20000000-0000-0000-0000-000000000000"), | ||
NID: nid1, | ||
Status: courier.MessageStatusProcessing, | ||
} | ||
err = p1.GetConnection(ctx).Create(&msg2) | ||
require.NoError(t, err) | ||
msg3 := courier.Message{ | ||
ID: uuid.FromStringOrNil("30000000-0000-0000-0000-000000000000"), | ||
NID: nid2, | ||
Status: courier.MessageStatusProcessing, | ||
} | ||
err = p2.GetConnection(ctx).Create(&msg3) | ||
require.NoError(t, err) | ||
now := time.Now().UTC().Truncate(time.Second).Format(timeFormat) | ||
|
||
// Set all `created_at` timestamps to the same value to force the `OR` clause of the paginated query. | ||
// `created_at` is set by "pop" and does not allow a manual override, apart from using `pop.SetNowFunc`, but that also influences the other tests in this | ||
// suite, as it just overrides a global function. | ||
require.NoError(t, p1.GetConnection(ctx).RawQuery("UPDATE courier_messages SET created_at = ? WHERE id = ? AND nid = ?", now, msg1.ID, nid1).Exec()) | ||
// get the "updated" message from the | ||
require.NoError(t, p1.GetConnection(ctx).Where("id = ? AND nid = ?", msg1.ID, msg1.NID).First(&msg1)) | ||
require.NoError(t, p1.GetConnection(ctx).RawQuery("UPDATE courier_messages SET created_at = ? WHERE id = ? AND nid = ?", now, msg2.ID, nid1).Exec()) | ||
require.NoError(t, p2.GetConnection(ctx).RawQuery("UPDATE courier_messages SET created_at = ? WHERE id = ? AND nid = ?", now, msg3.ID, nid2).Exec()) | ||
|
||
// Use the updated first message's PageToken as the basis for the paginated request. | ||
ms, _, _, err = p1.ListMessages(ctx, filter, []keysetpagination.Option{keysetpagination.WithToken(msg1.PageToken())}) | ||
require.NoError(t, err) | ||
|
||
// The response should just contain the "next" message from network1, and not the message from network2 | ||
require.Len(t, ms, 1) | ||
assert.Equal(t, ms[0].ID, msg2.ID) | ||
}) | ||
}) | ||
|
||
|
@@ -174,6 +222,7 @@ func TestPersister(ctx context.Context, newNetworkUnlessExisting NetworkWrapper, | |
err := p.SetMessageStatus(ctx, id, courier.MessageStatusProcessing) | ||
require.ErrorIs(t, err, sqlcon.ErrNoRows) | ||
}) | ||
|
||
}) | ||
|
||
t.Run("case=FetchMessage", func(t *testing.T) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
DROP INDEX courier_messages_nid_created_at_id_idx; | ||
|
||
DROP INDEX courier_messages_nid_status_created_at_id_idx; | ||
|
||
DROP INDEX courier_messages_nid_recipient_created_at_id_idx; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
DROP INDEX courier_messages_nid_created_at_id_idx ON courier_messages; | ||
|
||
DROP INDEX courier_messages_nid_status_created_at_id_idx ON courier_messages; | ||
|
||
DROP INDEX courier_messages_nid_recipient_created_at_id_idx ON courier_messages; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
CREATE INDEX courier_messages_nid_created_at_id_idx ON courier_messages (nid, created_at DESC); | ||
|
||
CREATE INDEX courier_messages_nid_status_created_at_id_idx ON courier_messages (nid, status, created_at DESC); | ||
|
||
CREATE INDEX courier_messages_nid_recipient_created_at_id_idx ON courier_messages (nid, recipient, created_at DESC); |
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.
Nice test