[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

lib/profileCache.js

@@ -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

lib/routes/avatar/get.js

-  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

lib/routes/avatar/delete.js

@@ -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

lib/routes/avatar/upload.js

-  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

lib/routes/email.js

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


no callbacks in inject?

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

lib/server/web.js

+      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

lib/routes/heartbeat.js

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


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

lib/server/web.js

  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

lib/server/worker.js

    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

lib/routes/display_name/post.js

    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

bin/worker.js

+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

bin/worker.js

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


style: start() { (space before {

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

lib/profileCache.js

    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

lib/routing.js

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


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

lib/server/_static.js

    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

lib/server/web.js

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


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

lib/server/web.js

  // 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

lib/server/web.js

  });

-  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

bin/_static.js

+
+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

lib/routes/avatar/delete.js

-          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

lib/routes/avatar/upload.js

+        // 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

lib/routes/display_name/post.js

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


this will fail, no return

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

lib/routes/display_name/post.js

+    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

lib/server/_static.js

      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

lib/profileCache.js

-      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

lib/profileCache.js

-        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

lib/profileCache.js

@@ -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

.eslintrc

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


let's make this 2018 here

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

lib/config.js

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


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

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

lib/routes/avatar/upload.js

+    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

lib/routes/display_name/post.js

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

lib/routes/email.js

+        // 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

lib/routes/root.js

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


what happened here?

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

lib/profileCache.js

@@ -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

lib/profileCache.js

        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