[WIP] refactor(hapi): Upgrade to hapi 17 by deeptibaghel · Pull Request #334 · mozilla/fxa-profile-server

deeptibaghel · 2018-07-31T10:09:42Z

Fixes #333

vladikoff

I'll check 'static' in a bit

vladikoff · 2018-07-31T12:43:06Z

@@ -91,7 +91,7 @@ module.exports = function profileCache(server, options, next) {
        scope: scope
      }}};
      return P.fromCallback(cb => {


there is a cb here, should that be removed and function below needs to 'return'?

we should check if cache.drop still uses callbacks or not

vladikoff · 2018-07-31T12:44:06Z

-  handler: function avatar(req, reply) {
+  handler: async function avatar(req) {
    var uid = req.auth.credentials.user;
    db.getSelectedAvatar(uid)


does this need a 'return'?

vladikoff · 2018-07-31T12:45:19Z

@@ -63,8 +63,7 @@ module.exports = {
      .then(() => {
        notifyProfileUpdated(uid); // Don't wait on promise
        return EMPTY;


need to check 'EMPTY' vs the {} above

vladikoff · 2018-07-31T12:45:56Z

-  handler: function upload(req, reply) {
+  handler: async function upload(req, h) {
    const uid = req.auth.credentials.user;
    req.server.methods.profileCache.drop(uid, () => {


does this need to return?, also no more callbacks?

vladikoff · 2018-07-31T12:46:42Z

@@ -26,20 +26,20 @@ module.exports = {
      credentials: req.auth.credentials
    }, res => {


no callbacks in inject?

vladikoff · 2018-07-31T12:48:32Z

+      cors: true,
+      security: {
+        hsts: {
+          maxAge: 15552000,


I think we had a different fix for this to make sure these headers are set ?

vladikoff · 2018-07-31T12:48:59Z

-  handler: function heartbeat(req, reply) {
-    db.ping().done(reply, reply);
+  handler: async function heartbeat() {
+    db.ping().then(() => {});


vladikoff · 2018-07-31T12:50:10Z

  server.route(routes);

-  server.ext('onPreAuth', function (request, reply) {
+  server.ext('onPreAuth', function (request, h) {


are these stilp callbacks? need to check..

vladikoff · 2018-07-31T12:51:45Z

    config: {
-      handler: function upload(req, reply) {
-        reply({});
+      handler: async function upload(req) {


function name 'upload' is wrong here, lets fix

vladikoff · 2018-07-31T12:53:11Z

    const uid = req.auth.credentials.user;
    req.server.methods.profileCache.drop(uid, () => {
      const payload = req.payload;
      db.setDisplayName(uid, payload.displayName)


return here as well?

vladikoff · 2018-08-02T14:04:57Z

+async function start(){
+  const server = await require('../lib/server/worker').create();
+
+  server.start(function() {


I don't think there is a callback anymore on .start, You need to do await server.start()
and surround it with the a try and catch to throw an error if it fails to .start

vladikoff · 2018-08-02T14:05:24Z

-server.start(function() {
-  logger.info('listening', server.info.uri);
-});
+async function start(){


style: start() { (space before {

vladikoff · 2018-08-02T14:05:54Z

    cache: {
      expiresIn: options.expiresIn,
-      generateTimeout: options.generateTimeout
+      generateTimeout: options.generateTimeout || 100


I think we change that in the tests instead and should not do this here

it was throwing error here generateTimeout is required so i added it temporarily

vladikoff · 2018-08-02T14:06:11Z

    method: 'POST',
    path: v('/display_name'),
-    config: require('./routes/display_name/post')
+    options: require('./routes/display_name/post')


vladikoff · 2018-08-02T14:06:41Z

    host: config.server.host,
    port: config.server.port + 1,
-    debug: { request: ['error'] }
+    debug: false


hm not sure about this change, I wonder what { request: ['error'] } used to do

it was originally false only

vladikoff · 2018-08-02T14:06:56Z

    port: config.server.port,
    cache: cache,
-    debug: false,
+    debug: { request: ['error'] },


vladikoff · 2018-08-02T14:07:14Z

  // server method for caching profile
  try {
    await server.register({
+      name: 'fxa-profile-server',


maybe we can call this just profileCache

vladikoff · 2018-08-02T14:07:31Z

  });

-  server.ext('onPreResponse', function(request, next) {
+  server.ext('onPreResponse', function(request, h) {


do we need the h here?

we have h.continue below

deeptibaghel · 2018-08-02T21:53:58Z

@vladikoff still i am missing something :(

vladikoff · 2018-08-09T14:15:58Z

+
+async function create() {
+  const server = await require('../lib/server/_static').create();
+  server.start().then(() => {


should use await server.start(); here , move the log out of then. Just like below: https://github.com/mozilla/fxa-profile-server/pull/334/files#diff-8d44631bfd02d64ee75348ab92f466c0R27

vladikoff · 2018-08-09T14:18:44Z

-          return workers.delete(avatar.id);
-        }
-      })
+    req.server.methods.profileCache.drop(uid)


looks like this will fail because this doesn't return

vladikoff · 2018-08-09T14:19:44Z

+        // precaution to avoid the default id from being overwritten
+        assert(id !== DEFAULT_AVATAR_ID);
+        const url = avatarShared.fxaUrl(id);
+        workers.upload(id, req.payload, req.headers)


this also needs to return otherwise response will be lost

vladikoff · 2018-08-09T14:20:01Z

-        })
-        .done(reply, reply);
-    });
+    req.server.methods.profileCache.drop(uid)


this will fail, no return

vladikoff · 2018-08-09T14:20:15Z

+    req.server.methods.profileCache.drop(uid)
+      .then(() => {
+        const payload = req.payload;
+        db.setDisplayName(uid, payload.displayName)


needs return here too

vladikoff · 2018-08-09T14:22:34Z

      switch (request.params.type) {
      case '':
-        reply.file(DEFAULT_AVATAR);
+        h.file(DEFAULT_AVATAR);


should we make h.file a return h.file ?

deeptibaghel · 2018-08-13T09:51:20Z

-      return next(null, result, ttl);
-    })
-    .catch(next);
+      .then(result => {


@vladikoff , I have fixed the server stop function but it still didn't work. This method seems to be failing.

deeptibaghel · 2018-08-15T19:16:01Z

-        server.methods.profileCache.get.cache.drop(req, cb);
-      });
-    }).asCallback(next);
+      await server.methods.profileCache.get.cache.drop(req);


@vladikoff , this method is not right yet, how do we change it correctly ?

deeptibaghel · 2018-11-20T14:00:52Z

@@ -90,7 +90,10 @@ module.exports = function profileCache(server, options) {
        scope: scope
      }}};
      return await server.methods.profileCache.get.cache.drop(req);


@vladikoff this line is throwing a 500 error, am I missing something ?

@deeptibaghel do you get this when running the tests?

vladikoff · 2018-11-22T20:23:57Z

  semi: [2, "always"]
+
+parserOptions:
+  ecmaVersion: 2017


let's make this 2018 here

vladikoff · 2018-11-22T20:42:57Z

  },
  serverCache: {
    redis: {
+     // name: 'redisCache',


what happens here? is this not named anymore? do we delete it?

vladikoff · 2018-11-22T20:48:39Z

+    const url = avatarShared.fxaUrl(id);
+    await workers.upload(id, req.payload, req.headers);
+    await db.addAvatar(id, uid, url, FXA_PROVIDER);
+    await notifyProfileUpdated(uid); // Don't wait on promise


per // Don't wait on promise , we don't want an await here

vladikoff · 2018-11-22T20:53:11Z

-        })
-        .done(reply, reply);
-    });
+    return req.server.methods.profileCache.drop(uid)


if we are changing the upload.js above to use async await, then we should do it here as well. or we can use the .then structure above. Pick the one you like.

vladikoff · 2018-11-22T20:57:04Z

+        // we should always get an email field back.
+        if (! res.result.email) {
+          logger.error('request.auth_server.fail', res.result);
+          return new AppError({


should we throw here?

vladikoff · 2018-11-22T21:02:01Z

-  handler: function index(req, reply) {
-    function sendReply() {
-      reply({
+  // response: {


what happened here?

vladikoff · 2018-11-22T21:05:51Z

@@ -90,7 +90,10 @@ module.exports = function profileCache(server, options) {
        scope: scope
      }}};
      return await server.methods.profileCache.get.cache.drop(req);


@deeptibaghel do you get this when running the tests?

deeptibaghel · 2018-11-23T06:49:08Z

        scope: scope
      }}};
-      return await server.methods.profileCache.get.cache.drop(req);
+      return server.methods.profileCache.get.cache.drop(req);


@vladikoff the test to upload avatar fails at this point

shane-tomlinson · 2019-04-03T06:14:01Z

I'm leaving this PR open because we still want this work! We will need to migrate this to the new mono-repo: github.com/mozilla/fxa

refactor(hapi): Upgrade to hapi 17

c54bf24

ghost assigned deeptibaghel Jul 31, 2018

ghost added the waffle:active label Jul 31, 2018

vladikoff reviewed Jul 31, 2018

View reviewed changes

deeptibaghel added 4 commits July 31, 2018 22:37

refactor(hapi): Upgrade to hapi 17

6c6efb7

refactor(hapi): Upgrade to hapi 17

45961cc

refactor(hapi): Upgrade to hapi 17

5d6d962

refactor(hapi): Upgrade to hapi 17

e093671

vladikoff reviewed Aug 2, 2018

View reviewed changes

refactor(hapi): Upgrade to hapi 17

67baebb

deeptibaghel added 2 commits August 3, 2018 13:11

refactor(hapi): Upgrade to hapi 17

5a2120d

refactor(hapi): Upgrade to hapi 17

bd92760

vladikoff reviewed Aug 9, 2018

View reviewed changes

deeptibaghel added 2 commits August 10, 2018 15:55

refactor(hapi): Upgrade to hapi 17

c6826cb

refactor(hapi): Upgrade to hapi 17

67f7850

deeptibaghel commented Aug 13, 2018

View reviewed changes

shane-tomlinson mentioned this pull request Aug 15, 2018

fix(test): Avoid cross test pollution caused by mutating route objects. #336

Merged

refactor(hapi): Upgrade to hapi 17

42027ec

deeptibaghel commented Aug 15, 2018

View reviewed changes

shane-tomlinson mentioned this pull request Sep 11, 2018

fix(tests): split running remote and local tests mozilla/fxa-auth-server#2623

Merged

deeptibaghel added 2 commits November 20, 2018 16:45

refactor(hapi): Upgrade to hapi 17

d2e11c4

refactor(hapi): Upgrade to hapi 17

a34c6a7

deeptibaghel commented Nov 20, 2018

View reviewed changes

vladikoff reviewed Nov 22, 2018

View reviewed changes

refactor(hapi): Upgrade to hapi 17

25bf02c

deeptibaghel commented Nov 23, 2018

View reviewed changes

rfk mentioned this pull request Nov 18, 2019

oauth-server doesn't send CORS headers to requests with no Origin header mozilla/fxa#2411

Closed

		@@ -26,20 +26,20 @@ module.exports = {
		credentials: req.auth.credentials
		}, res => {

Conversation

deeptibaghel commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vladikoff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deeptibaghel Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deeptibaghel commented Aug 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

deeptibaghel commented Jul 31, 2018 •

edited

Loading

deeptibaghel Aug 2, 2018 •

edited

Loading