feat: complete (mostly) the WebPushClient #379
Conversation
| }); | ||
| } | ||
| } | ||
| // NOTE: when the client's specified a uaid but get_user returns |
There was a problem hiding this comment.
Wanted to point this out as one difference between the other state machine. Previously we only set the defer_add_user (previously called deferred_registration) for when clients did not specify a uaid.
Now we're doing it for any case where we would have created a brand new user record -- including when a user specifies a uaid and its lookup failed or its record was invalid.
but ignore the 4 current failures SYNC-3688
pjenvey
force-pushed
the
feat/sm-storage-SYNC-3688
branch
from
57eb0dc to
50e9d12
Compare
|
Added an integration test run against this to ci |
| @@ -15,20 +15,37 @@ pub const HELLO: &str = r#"{"messageType": "hello", "use_webpush": true}"#; | |||
| pub const HELLO_AGAIN: &str = r#"{"messageType": "hello", "use_webpush": true, | |||
| "uaid": "deadbeef-0000-0000-deca-fbad00000000"}"#; | |||
|
|
|||
| pub const CURRENT_MONTH: &str = "message_2023_5"; | |||
There was a problem hiding this comment.
I'm tempted to suggest that we either use the current, production value here (message_2017_09 IIRC), or remove the table rotation code entirely since it's no longer useful and just adds complications.
There was a problem hiding this comment.
Changed this test value to match prod, of course we can't hardcode the result of Db::message_month to such a value though (in case you're suggesting that which you probably aren't).
There was a problem hiding this comment.
With the metrics we saw yesterday I'll definitely remove the table migration code in a separate PR (mostly to document it in isolation). I think it will end up having us match autoendpoint's validate_webpush_user even more closely.
autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/mod.rs
Outdated
Show resolved
Hide resolved
autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/on_client_msg.rs
Show resolved
Hide resolved
| self.app_state.db.fetch_messages(&self.uaid, 11).await? | ||
| } else { | ||
| Default::default() | ||
| }; |
There was a problem hiding this comment.
This bit feels a bit confusing.
We check for topics first, returning them if there are any, otherwise we set the timestamp if we're supposed to include topics. Then we try fetching any pending messages.
It might be useful to add some comment blocks around the two parts, so it's a bit more obvious the division we're doing, and maybe change the first resp to something like topics_resp so it's a bit more obvious that there are two fetches going on?
There was a problem hiding this comment.
I definitely meant to document the guts of check_storage more, I just added that as a TODO for now, but renamed the resp vars.
| @@ -510,6 +511,35 @@ impl DbClient for DdbClientImpl { | |||
| }) | |||
| } | |||
|
|
|||
| async fn increment_storage(&self, uaid: &Uuid, timestamp: u64) -> DbResult<()> { | |||
There was a problem hiding this comment.
TODO: jr to add increment_storage and save_messages to #364
|
FWIW: I pre-emptively merged this PR into #364. One thing that I spotted was that I don't know if we actually use the user |
|
Which |
|
Sorry, the one we write to the database in the User meta record for DynamoDB. It's the field updated in |
SYNC-3688