Server needs to store user ID to push URL mapping(s)

VERIFIED FIXED

Status

Hello (Loop)
Server
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: abr, Assigned: alexis)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

User Story

We need to store a mapping from user id to a set of simple push URIs that this user's device(s) are attached to.

Attachments

(1 attachment)

Comment hidden (empty)
(Reporter)

Updated

4 years ago
Blocks: 971986
(Reporter)

Updated

4 years ago
Component: General → Server

Comment 1

4 years ago
We need to store a mapping from user id to a set of simple push URIs that this user's device(s) are attached to.
(Assignee)

Comment 2

4 years ago
It seems to me that we don't get much from decoupling bug 971991 and bug 971993; is there a granularity we're looking at? for server side components, it seems preferable to have the whole flow in one bug (accepting & processing requests) otherwise it adds management overhead for little value.

Comment 3

4 years ago
If you want to treat this as an umbrella bug, I am sure nobody would mind. I'm just cautious when a bug involves more than a few distinct patches, since they get pretty unwieldy.
Blocks: 972866
(Assignee)

Comment 4

4 years ago
Creates an in memory storage for the PoC. Still need to store this in a real db somewhere.

https://github.com/ametaireau/pants-server/commit/a462e6f2c4b7311a57f9a92a564fb2c25b7fa16f

It seems this data is not really long lived: if I understand correctly, the simplepush clients will be given a new simple push url (to give to the server each time), so we don't need this to be really persisted for long (I guess a cache would be enough).

Depending how we plan on deploying things can affect the db we chose. Do we know what we plan to deploy on?

This also makes me think we probably will want to get the last simple push url in the list, and we probably want to expire them on the server (on retrieval, I suppose) to avoid having unused data there.

Thoughts?
User Story: (updated)
Depends on: 976223
(Reporter)

Comment 5

4 years ago
(In reply to Alexis Metaireau (:alexis) from comment #2)
> It seems to me that we don't get much from decoupling bug 971991 and bug
> 971993; is there a granularity we're looking at? for server side components,
> it seems preferable to have the whole flow in one bug (accepting &
> processing requests) otherwise it adds management overhead for little value.

The idea is to prevent a single bug with a bunch of patches. If you happen to fix several bugs in a single patch because it was faster to do so than to break apart into several patches, that should be okay: just pick one bug to be the "main" one, rename it accordingly, and dupe the other bugs to it.
(Reporter)

Comment 6

4 years ago
(In reply to Alexis Metaireau (:alexis) from comment #4)
> https://github.com/ametaireau/pants-server/commit/a462e6f2c4b7311a57f9a92a564fb2c25b7fa16f

I thought we were working in mozilla/loop rather than ametaireau/pants-server now.
(Reporter)

Comment 7

4 years ago
(In reply to Alexis Metaireau (:alexis) from comment #4)

> It seems this data is not really long lived: if I understand correctly, the
> simplepush clients will be given a new simple push url (to give to the
> server each time), so we don't need this to be really persisted for long (I
> guess a cache would be enough).

In discussions with Doug Turner, I was led to believe that the simple push URLs would actually be quite long-lived (like on the order of days or weeks). I'm not sure if this means that we get the same one if we ask for it multiple times, or if we (as the client) are supposed to cache the simple push URL for use across browser sessions.

> This also makes me think we probably will want to get the last simple push
> url in the list...

Nope. We need to use all of them. If I'm on my laptop, desktop, and FirefoxOS device simultaneously, an incoming call needs to find all three of them.

> ...and we probably want to expire them on the server (on
> retrieval, I suppose) to avoid having unused data there.

I don't know why you would want to expire them on retrieval: just because I got one call doesn't mean that I never want to get another one.

Nonetheless, there's an open issue here around the duration of validity of these URLs, and the closest I can find to documentation is here: https://wiki.mozilla.org/WebAPI/SimplePush#Notes

"The lifetime of registrations for web pages needs specification."

Doug: is there something you can point us at that would let us know how to manage push URL validity durations; or at least some kind of thumbnail sketch?
Flags: needinfo?(dougt)
(Assignee)

Comment 8

4 years ago
(some of the messages you had were old ones when we were working on the python proto. Just discard them).

Right: we need to get all the simple push urls and notify all the clients. Because we need to keep the data on the server until all the devices got it, we then need some expiration of these, but that's a separate discussion (e.g. how much time do I need to keep the session id + tokens from the provider?).

Regarding expiration of the push urls (if they eventually expire), we could do this expiration on retrieval. Not meaning we want to remove the ones we just retrieved, but we could have a look at them at this stage and discard them if they're no longer valid, rather than doing that in a separate process.

Or maybe the best way to handle this is to have a cron running somewhere and removing the data that's not valid anymore.

Or both, actually, but that's something we would need to discuss, probably in a separate bug though.
(Assignee)

Updated

4 years ago
Assignee: nobody → alexis+bugs
(Assignee)

Comment 9

4 years ago
Created attachment 8386744 [details] [review]
link to github PR
Attachment #8386744 - Flags: feedback?(nperriault)
(Assignee)

Comment 10

4 years ago
Here is a simple implementation of persisting userid to pushurl mappings.

I wonder if that's how stores were meant to be used.

In this implementation, I don't do anything about generating indexes for the database, and I think we should do it somewhere. I was thinking about doing that in a specific storage layer (which would add some business logic on top of the stores) but after a quick discussion with Nico, we are thinking that may be overkill for now.

If the approach I took is correct, I can go ahead and add some logic to the stores (and specifically the mongo one) so that it's able to expose a way to create indexes.

This patch doesn't do any expiration on the simplepush urls since we don't know for how long they'll last yet.
> I can go ahead and add some logic to the stores so that it's able to expose a way to create indexes.

MongoStore (and MemoryStore) already allow to define unique indexes using the `unique` option. 

I've updated the patch with minor tweaks regarding the store configuration (patch is attached to the initial pull request).
(Assignee)

Updated

4 years ago
Attachment #8386744 - Flags: feedback?(nperriault) → review?(adam)
(Assignee)

Comment 12

4 years ago
dougt feedback is not needed anymore, expiration of URLs is handled at Bug 980289
Status: NEW → ASSIGNED
Flags: needinfo?(dougt)
(Reporter)

Comment 13

4 years ago
Comment on attachment 8386744 [details] [review]
link to github PR

I have a handful of nit comments on the patch, but it overall looks right. r+, assuming the nits are addressed.
Attachment #8386744 - Flags: review?(adam) → review+
(Assignee)

Comment 14

4 years ago
https://github.com/mozilla/loop-server/commit/3beef097f5fe1a1d3181559389b488f027f5ee2c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Closing since the code has changed a lot since this commit was made.
Status: RESOLVED → VERIFIED
QA Contact: jbonacci
You need to log in before you can comment on or make changes to this bug.