Add username translation

[j3k0]

See the first commit for specifications.

[elmigranto]

Just to make sure.

// Suppose we have
{ messageArgs: ["hello_world", "Hello, ", "alice", "!"],
  messageArgsTypes: ["string", "username", "string"]
}

// Should become
{ messageArgs: ["hello_world", "Hello, ", "Alice of the Wonderland", "!"],
  messageArgsTypes: ["string", "username", "string"]
}

That replacement only happens when directory server replies to /users/id/alice with 200 OK and body contains aliases.name. Like this:

{
    "id": "alice",
    "aliases": {
        "name": "Alice of the Wonderland"
    }
}

Otherwise we leave original array as it was with username being userId ("alice").

[elmigranto]

I am a bit rusty on this part of ganomede, so I'll put some thoughts here, maybe you can take a look while I browse the codebase and specs.

So my first instinct was to do this replacement when worker is about to send notification, however, I think that is not correct for multiple reasons:

• I assume this will slow down sending out push notifications quite a bit;
• we already do a lot of stuff in push senders;
• conversion to APN/GCM format is a sync operation atm, awkward to make changes;
• many workers might "spam" directory for the same username;
• workers are multi-process, so no way to cache it effectively without offloading stuff to redis (at which point why not just put correct info there already);
• very minor a bit of a confusing lag: push created -> user changes name -> push sent out (though, this one is not great either way).

So I am thinking we convert username -> alias:name before adding it to the push queue (somewhere around push-api/queue.coffee#add()).

• Pros
  • single place where we look for aliases (easier and more efficient caching possible);
  • redis will have push message in final form;
  • our push syntax that is not specific to GCM/APN (specifically "messageArgsTypes" part). Coupled with previous entry, this means no changes needed in sender code.
• Cons
  • takes a bit longer for message to go into redis queue (expected and less extreme than in case where conversion is before sending a push).

[j3k0]

Yes you got it all right.

I went through the same reasoning, it'd more logical to convert from the worker (because then you have the most up-to-date name for the user)... but it's just a question of seconds really. Also it'd allow for other modules to put stuff in the push-queue and allow "translations", but most probably: YAGNI (you aren't gonna need it), it seems easier and more efficient to do that from the push-api.

BTW, instead of username for the argument type, maybe we should use something like directory:name. (so later we can extend the concept to stuff like directory:email, usermeta:location, whatever...)

[elmigranto]

This one should work with some caveats, so if it is urgent to deploy, we can try this on test server. Problems:

• [x] each alias is processed individually, so duplicates and multiple calls for single user (e.g. if we for directory:name and directory:email of alice and bob — that's 4 calls instead of 2; if those are met multiple times in the string, even more);
• [x] haven't updated README yet;
• [ ] some awkward patterns, will refactor a bit later. (no need to wait on this)

[elmigranto]

Question: do we need to support the case when DIRECTORY address is not configured? Otherwise, it'll have to wait for retries to finish (multiple seconds by default).

[j3k0]

1. We don't need to translate more than 1 field currently. It's good to know that we can if we need, but it's really not important at this point if it's not optimised.
2. When directory is not configured (based on env var existence), the translation module can simply ignore the request for translation and keep the field as it is. I suppose it's easy to add? If so, let's just do it.

[elmigranto]

Right now, we are looking through all items inside *ArgTypes array and check that against a bunch of functions that can perform expansion. We can define anything there as long as it is arg typed in type:subtype format, where type is the name of export from ./src/push-api/translators.coffee and subtype helps that function to identify what to do. Actual string from array and its index will be received by that function, as well as field name of that array. E.g.:

Translatable {
  field: 'title',
  type: 'directory:email',  // arg type
  index: 3,  // index in the title array
  value: 'alice'  // original string at that index
}

[elmigranto]

When directory is not configured

Added a check for truthyness of DIRECTORY_PORT_8000_TCP_ADDR, otherwise, not exporting directory expander function, so it won't be used by PushTranslator.

[elmigranto]

https://github.com/j3k0/ganomede-notifications/pull/40/commits/b39b305b4cf48eb95dac02cdf2a2b88ee7e98f81 makes any arg type with colon translatable. If that's okay, we can merge this one.

[j3k0]

Sure, the : convention is ok. I'll test and deploy this after the week-end. I read through the code, seems alright, thanks for the great job!

[j3k0]

You can ignore this message: I send it because I wrote it. 😃

My mistake was that I thought the worker was doing the translation, while it was the push api service.

snip-snap

I can't make it work: player name's are not translated. Anything I missed?

Here's a sample message handled by the pushworker:

{
      from: 'turngame/v1',
      to: 'bubli729',
      type: 'move',
      data: 
       { game: 
          { id: 'cedf2a6f35e12a3563864158233bba2c',
            players: [Object],
            status: 'active',
            turn: 'bubli729',
            type: 'wordsaxe/v1' } },
      push: 
       { app: 'wordsaxe/v1',
         title: [ 'your_turn_title' ],
         message: [ 'your_turn_message', 'Jeko488' ],
         messageArgsTypes: [ 'directory:name' ] },
      timestamp: 1506453226079,
      id: 1362 }

Here are directory related env vars configured in the push worker.

DIRECTORY_PORT_8000_TCP_PROTOCOL="https"
DIRECTORY_PORT_8000_TCP_PORT="443"
DIRECTORY_PORT_8000_TCP_ADDR="<some-hostname>"

I wonder if I shouldn't set those in the notifications service instead of the worker... Let's try.

and... that was it

[j3k0]

It's deployed and seems to be working just fine. Thanks!

[elmigranto]

Okay, nice to hear, though looking at your example of push object, I'm wondering whether it handles optional stuff nicely (like there is no messageArgsTypes). AFAIR there is STATIC_KEYS.forEach on the push object which will pull undefined instead of array, but it might be okay because javascript 😕

My assumptions on spec of push:

• we always have title and message;
• we always have *ArgsTypes for each present field;
• if *ArgsTypes is present, it is correct: flat array of strings, lengths match.

Seems 2nd one is wrong? Anyways, will double check tomorrow.

---

On push-worker…

Shouldn't be too hard to move it there. Both places make sense and have their own downsides (we briefly discussed it in #38). Though it was far easier to do it when notification is accepted, instead of when sent out (but hey, we'll have to redo senders sometimes anyways, ha-ha :)

[j3k0]

Yes, all assumptions are correct, except for 2: we should still support messages without *ArgsTypes. But as I do not see error reports so far from the server, it might already work. Worth adding a unit test to make sure of that though.

No need to move to pushworker, I just forgot our discussion about that. One shouldn't assume anything and just configure the link to DIRECTORY from both.

[j3k0]

@elmigranto I got some bug reports, notifications that don't contain *ArgTypes are not delivered (those from the chat and invitations modules)

[elmigranto]

Yeah, fails silently? Process crash? I believe adding them to push queue is something like queue.add(notification, noop), though there are some logging messages.

Anyways, on it!

[j3k0]

I didn't check the server. That's just complains from the client. I reverted to older version until this is solved.

On Wed, Sep 27, 2017 at 4:37 PM Alexey Zabrodsky notifications@github.com wrote:

Yeah, fails silently? Process crash? I believe adding them to push queue is something like queue.add(notification, noop), though there are some logging messages.

Anyways, on it!

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/j3k0/ganomede-notifications/pull/40#issuecomment-332523078, or mute the thread https://github.com/notifications/unsubscribe-auth/AALtieOHDSVY3YkcayRcOVTdib-97YZlks5smk-NgaJpZM4OZRaS .

[elmigranto]

Though I am pretty sure what the problem is and will start working on that, could you double check when you have time? I believe we should have process crash and something like TypeError: Cannot convert undefined or null to object somewhere from push translation stuff (or maybe TypeError: Cannot read property '0' of undefined).

[j3k0]

I don't see any crash or error reports, maybe it goes silently and the worker process is restarted? I don't think so, if I'm right the worker logs uncaught exceptions... It's also possible that the exception is caught silently as well somewhere in the stack...?

On Wed, Sep 27, 2017 at 5:36 PM Alexey Zabrodsky notifications@github.com wrote:

Though I am pretty sure what the problem is and will start working on that, could you double check when you have time? I believe we should have process crash and something like TypeError: Cannot convert undefined or null to object somewhere from push translation stuff (or maybe TypeError: Cannot read property 'map' of undefined).

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/j3k0/ganomede-notifications/pull/40#issuecomment-332542128, or mute the thread https://github.com/notifications/unsubscribe-auth/AALtiVEPFXh7z_34LuwaNKvkv7eC8oI8ks5sml1OgaJpZM4OZRaS .

[elmigranto]

Our code definetly throws TypeError that is not catched anywhere and should hard crash node process, however:

restify.createServer({
    handleUncaughtExceptions: true,
    // …
});

Not sure how to access restify docs for older versions, but if I remember correctly, this should make any exceptions into HTTP 500 and log stuff. Newer version says:

When true (default is false) restify will use a domain to catch and respond to any uncaught exceptions that occur in it’s handler stack.

[j3k0]

Yes, usually I see HTTP 500 and stack traces on errors. Not sure what happens here.

On Wed, Sep 27, 2017 at 6:02 PM Alexey Zabrodsky notifications@github.com wrote:

Our code definetly throws, however:

restify.createServer({ handleUncaughtExceptions: true, // … });

Not sure how to access restify docs for older versions, but if I remember correctly, this should make any exceptions into HTTP 500 and log stuff. Newer version says http://restify.com/docs/server-api/:

When true (default is false) restify will use a domain to catch and respond to any uncaught exceptions that occur in it’s handler stack.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/j3k0/ganomede-notifications/pull/40#issuecomment-332550653, or mute the thread https://github.com/notifications/unsubscribe-auth/AALticLA6Hj2mP9Iva7j3VhvLH_tU15Lks5smmMegaJpZM4OZRaS .

[elmigranto]

Here's what I would expect to see:

TypeError: Cannot read property '0' of undefined
      at src/push-api/push-translator.coffee:69:24
      at Array.forEach (native)
      at PushObject.PushTranslator.PushObject.PushObject.translatables (src/push-api/push-translator.coffee:63:28)
      …