Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Don't user upsert to persist new one time keys #2053

Merged
merged 6 commits into from
Mar 29, 2017

Conversation

erikjohnston
Copy link
Member

Instead we no-op duplicate one time key uploads, an error if the key_id
already exists but encodes a different key.

Instead we no-op duplicate one time key uploads, an error if the key_id
already exists but encodes a different key.
@erikjohnston
Copy link
Member Author

Oh, I suppose I should raise a more helpful exception

@erikjohnston
Copy link
Member Author

(I'm now raising a slightly more helpful exception)

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we change it to include the algorithm in the table key?

# First we check if we have already persisted any of the keys.
rows = yield self._simple_select_many_batch(
table="e2e_one_time_keys_json",
column="key_id",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically the algorithm is part of the key - so it's valid to have an "ed25519:aaaaa" as well as a "rot13:aaaaa". I rather hope the index reflects that, but haven't checked...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh bleurgh, thanks, have changed.

# We are protected from race between lookup and insertion due to
# a unique constraint. If there is a race of two calls to
# `add_e2e_one_time_keys` then they'll conflict and we will only
# insert one set.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and throw a 500 or something, presumably? I suppose that's fine because the client should retry on the 500 and hit the api again, at which point the lookup should conclude that it's all fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup.

@richvdh richvdh assigned erikjohnston and unassigned richvdh Mar 24, 2017
@erikjohnston erikjohnston assigned richvdh and unassigned erikjohnston Mar 28, 2017
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@richvdh
Copy link
Member

richvdh commented Mar 29, 2017

modulo test failures. what's all that about?

@richvdh richvdh assigned erikjohnston and unassigned richvdh Mar 29, 2017
@erikjohnston
Copy link
Member Author

Whoops, look like I was a crank. Hopefully fixed now.

@erikjohnston erikjohnston merged commit 2f8b580 into develop Mar 29, 2017
@erikjohnston erikjohnston deleted the erikj/e2e_one_time_upsert branch October 26, 2017 11:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants