bug: store meta-data for push component in the database. #907

jrconlin · 2019-04-03T23:21:30Z

Push now stores uaid and the server secret auth in a table in the
push.db. the meta_data table allows any key/value data to be stored to
allow for additional future values (if needed).

Closes #905

Pull Request checklist

Quality: This PR builds and tests run cleanly
- cargo test --all produces no test failures
- cargo clippy --all --all-targets --all-features runs without emitting any warnings
- cargo fmt does not produce any changes to the code
- ./gradlew ktlint detekt runs without emitting any warnings
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

components/push/storage/src/schema.sql

CHANGELOG.md

pjenvey

Couldn't uaid and auth live in the Kotlin side's prefs? On Desktop at least uaid is stored there in dom.push.userAgentID.

What concerns me here is message dispatch (onMessageReceived). When receiving a message dispatch needs to ask Push Storage where to route the message (what uaid and what scope, e.g. FxA related for send tab or normal push messages).

There may be multiple uaids to choose from, either from different gecko profiles or potentially different android users (if we even support the latter?). So as it is currently Push Storage is not per one uaid/profile, it stores all records.

jrconlin · 2019-04-06T00:00:11Z

@pjenvey It's my understanding from @jonalmeida that there's no real data storage options for on the Kotlin side. In addition to this, I'm working on a version of Delivery manager that provides a storage mechanism for data associated with the ChannelID.

It's also our understanding that profile changes effectively change all data because they point to a different android data directory. Containers would still use the same UAID, since incoming notifications effectively share the same User Agent.

I also tried to make the meta_data store fairly generic so that if we had to use multiple UAIDs, we could do crap like prefix key names or something.

There still seem to be a lot of unknowns about how User Agents actually will operate and what features we'll be implementing, so simplifying to a base set seemed a better approach than designing for what may not happen.

Push now stores uaid and the server secret auth in a table in the push.db. the meta_data table allows any key/value data to be stored to allow for additional future values (if needed). Closes #905

CHANGELOG.md

components/push/communications/src/lib.rs

components/push/storage/src/schema.sql

components/push/subscriber/src/lib.rs

components/push/storage/src/db.rs

thomcc

Looks okay once nits are addressed.

thomcc · 2019-04-09T16:37:34Z

components/push/storage/src/db.rs

+
+    fn set_meta(&self, key: &str, value: &str) -> Result<()> {
+        let query = "INSERT or REPLACE into meta_data (key, value) values (:k, :v)";
+        self.execute_named(query, &[(":k", &key), (":v", &value)])?;


Nit execute_named_cached (missed this last time, sorry).

crap, sorry, you're right.

thomcc · 2019-04-09T16:38:26Z

components/push/subscriber/src/lib.rs

@@ -66,7 +68,15 @@ impl PushManager {
        record.app_server_key = self.config.vapid_key.clone();
        record.native_id = Some(reg_token);
        self.store.put_record(&record)?;
-        // TODO: just return Record
+        // store the meta information if we've not yet done that.
+        if self.store.get_meta("uaid")?.is_none() {


Should we (do we?) have a transaction open here? I guess we only have a single connection, and so it shouldn' tmatter.

ideally, there would only be one "uaid". There's been some discussion about how this would be handled by containers (if/when they're brought over to mobile), but even then, having a single UAID may be the answer. UAID is more about delivery to a given device, channel about segregating to recipient processes. Same is true with the registration ID, to an extent, but that's generally initialized at app start and handed to us. The meta data is more a write once, read many store.

thomcc · 2019-04-09T16:39:36Z

components/push/storage/src/schema.sql

 );
+
+DROP INDEX IF EXISTS channel_id_idx;
 CREATE UNIQUE INDEX channel_id_idx ON push_record(channel_id);


Unless I'm missing something, UNIQUE implies the existence of a UNIQUE INDEX, so I don't think we need this.

thomcc · 2019-04-09T16:44:48Z

@pjenvey It's my understanding from @jonalmeida that there's no real data storage options for on the Kotlin side.

Is this true? How do we persist the FxA state then?

jonalmeida · 2019-04-09T20:16:07Z

@pjenvey It's my understanding from @jonalmeida that there's no real data storage options for on the Kotlin side.

Is this true? How do we persist the FxA state then?

For the UAID, we can persist this but the argument was that, if each client needs to do this, wouldn't it be easier for the rust component to hide that away since it already does that?

For the Delivery Manager request, we have to store our own mapping of service to channelID on each client. Today this is a small list, but it can get quite large as we enable support for web push. For now, we're storing that on the Android side until we get a longer term solution.

thomcc · 2019-04-09T20:23:43Z

@jonalmeida Thanks, that makes sense. I agree it's better to persist here (especially since we already have a database to do it in) I was just confused since we were clearly persisting FxA on the android side, which the comment suggested was impossible.

jrconlin · 2019-04-09T21:07:38Z

Ah, good. The comment was borne of my own confusion as well. I thought that there was no (or very limited) db like storage available on the client side. My general fear was that I'd be creating a general preference storage system, which I really don't want to do. I'll fix up the comment and follow up with @jonalmeida about this offline.

jrconlin added review labels Apr 3, 2019

jrconlin added this to In progress in Push Component Build via automation Apr 3, 2019

jrconlin requested review from pjenvey, thomcc and jonalmeida April 3, 2019 23:21

thomcc reviewed Apr 5, 2019

View reviewed changes

components/push/storage/src/schema.sql Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

pjenvey reviewed Apr 5, 2019

View reviewed changes

bug: store meta-data for push component in the database.

8a19746

Push now stores uaid and the server secret auth in a table in the push.db. the meta_data table allows any key/value data to be stored to allow for additional future values (if needed). Closes #905

jrconlin force-pushed the bug/905 branch from 5e0cad9 to 8a19746 Compare April 8, 2019 21:19

jrconlin requested a review from thomcc April 8, 2019 21:29

Push Component Build automation moved this from In progress to Needs review Apr 8, 2019

thomcc suggested changes Apr 8, 2019

View reviewed changes

f address r's

ada216c

jrconlin requested a review from thomcc April 9, 2019 15:47

Push Component Build automation moved this from Needs review to Reviewer approved Apr 9, 2019

thomcc approved these changes Apr 9, 2019

View reviewed changes

f welcome nits

88436d6

jrconlin merged commit 570f3fa into master Apr 10, 2019

Push Component Build automation moved this from Reviewer approved to Done Apr 10, 2019

eoger deleted the bug/905 branch April 10, 2019 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug: store meta-data for push component in the database. #907

bug: store meta-data for push component in the database. #907

jrconlin commented Apr 3, 2019 •

edited

pjenvey left a comment •

edited

jrconlin commented Apr 6, 2019

thomcc left a comment

thomcc Apr 9, 2019

jrconlin Apr 9, 2019

thomcc Apr 9, 2019

jrconlin Apr 9, 2019

thomcc Apr 9, 2019

thomcc commented Apr 9, 2019

jonalmeida commented Apr 9, 2019

thomcc commented Apr 9, 2019

jrconlin commented Apr 9, 2019

bug: store meta-data for push component in the database. #907

bug: store meta-data for push component in the database. #907

Conversation

jrconlin commented Apr 3, 2019 • edited

Pull Request checklist

pjenvey left a comment • edited

Choose a reason for hiding this comment

jrconlin commented Apr 6, 2019

thomcc left a comment

Choose a reason for hiding this comment

thomcc Apr 9, 2019

Choose a reason for hiding this comment

jrconlin Apr 9, 2019

Choose a reason for hiding this comment

thomcc Apr 9, 2019

Choose a reason for hiding this comment

jrconlin Apr 9, 2019

Choose a reason for hiding this comment

thomcc Apr 9, 2019

Choose a reason for hiding this comment

thomcc commented Apr 9, 2019

jonalmeida commented Apr 9, 2019

thomcc commented Apr 9, 2019

jrconlin commented Apr 9, 2019

jrconlin commented Apr 3, 2019 •

edited

pjenvey left a comment •

edited