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

Status: VERIFIED FIXED

(Hello (Loop) :: Server, defect)

Comment 1

[Byron Campen [:bwc]]

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.

Comment 2

[Alexis Metaireau (:alexis)]

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

[Byron Campen [:bwc]]

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.

Comment 4

[Alexis Metaireau (:alexis)]

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?

Comment 5

[Adam Roach [:abr]]

(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.

Comment 6

[Adam Roach [:abr]]

(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.

Comment 7

[Adam Roach [:abr]]

(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?

Comment 8

[Alexis Metaireau (:alexis)]

(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.

Comment 9

[Alexis Metaireau (:alexis)]

Attachment: link to github PR

Comment 10

[Alexis Metaireau (:alexis)]

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.

Comment 11

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

> 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).

Comment 12

[Alexis Metaireau (:alexis)]

dougt feedback is not needed anymore, expiration of URLs is handled at Bug 980289

Comment 13

[Adam Roach [:abr]]

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.

Comment 14

[Alexis Metaireau (:alexis)]

https://github.com/mozilla/loop-server/commit/3beef097f5fe1a1d3181559389b488f027f5ee2c

Comment 15

[James Bonacci [:jbonacci]]

Closing since the code has changed a lot since this commit was made.