hoangvvo / next-connect

The TypeScript-ready, minimal router and middleware layer for Next.js, Micro, Vercel, or Node.js http/http2
https://www.npmjs.com/package/next-connect
MIT License
1.62k stars 65 forks source link

404 error when using async functions #159

Open bramvbilsen opened 2 years ago

bramvbilsen commented 2 years ago

I am trying to make a call to my database in a next-connect handler as follows:

const handler = common().get(
    async (req: NextApiRequest, res: NextApiResponse) => {
        const uid = req.user._id;

        try {
            const user = await User.findById(uid).exec();
            console.log(user);
            return res.json({ cool: "cool" });
        } catch (e) {
            return res.json({
                status: "error",
                message: "Could not load user data",
            });
        }
    }
);

The user gets logged correctly on the server, but client side, I get a 404.
When I do not make the call to the database, the handler works just fine. I also tried to use other async methods like fetch and they also resulted In a 404 which makes me believe that is where the problem is.

Is this an implementation flaw on my end? Or does next-connect not work with async calls?

EDIT 1 common() is defined as follows:

let initialized = false;
let initializedSession;
let initializedPassport;
let initializedPassportSession;

if (!initialized) {
  initializedSession = session({
    secret: process.env.SESSION_SECRET || "Test1234",
    resave: false,
    saveUninitialized: false,
    store: MongoStore.create({
      clientPromise: new Promise((resolve, reject) =>
        resolve(mongoose.connection.getClient())
      ),
      dbName: "X",
      stringify: false,
      autoRemove: "interval",
      autoRemoveInterval: 60,
    }),
  });
  initializedPassport = passport.initialize();
  initializedPassportSession = passport.session();
  initialized = true;
}

export default function common() {
  return (
    nc()
      .use(db)
      //@ts-ignore
      .use(initializedSession)
      //@ts-ignore
      .use(initializedPassport)
      .use(initializedPassportSession)
  );
}
hoangvvo commented 2 years ago

next-connect works just fine with async. what type of request did you make from client side (POST or GET) and to which API paths? Where do you store the file above?

bramvbilsen commented 2 years ago

next-connect works just fine with async. what type of request did you make from client side (POST or GET) and to which API paths? Where do you store the file above?

The path for the API is: /api/v1/auth/userInfo.
But, I did manage to find the cause of the problem, I'm just not sure why it occurs. On the page where I call this endpoint, I do also call another endpoint. Only one of the two ever completes. If I make a request to only one, everything works fine. I suspect the other one always completes because it is much shorter. Is it possible for two endpoints to interfere with each other?

hoangvvo commented 2 years ago

It sounds like you are facing the first issue in https://github.com/hoangvvo/next-connect#common-errors. Can you check that your code does not have such pattern?

hoangvvo commented 2 years ago

On second thought looking at your provided code common though, it was probably not the case. But I am not familar with the pattern you are using (initialized = true). Would it be possible just to write:

export default function common() {
  return (
    nc()
      .use(db)
      .use(session({
    secret: process.env.SESSION_SECRET || "Test1234",
    resave: false,
    saveUninitialized: false,
    store: MongoStore.create({
      clientPromise: new Promise((resolve, reject) =>
        resolve(mongoose.connection.getClient())
      ),
      dbName: "X",
      stringify: false,
      autoRemove: "interval",
      autoRemoveInterval: 60,
    }),
  }))
      .use(passport.initialize())
      .use(passport.session())
  );
}
bramvbilsen commented 2 years ago

Yes, I think using my common method prevents the issue you mentioned.
The reason why I am working with the initialized is because I do not run my application serverless but on a normal VM instead. I want to prevent things from reinitializing every single request. Do you think this is not the correct way of handling this?

bramvbilsen commented 2 years ago

On second thought looking at your provided code common though, it was probably not the case. But I am not familar with the pattern you are using (initialized = true). Would it be possible just to write:

export default function common() {
  return (
    nc()
      .use(db)
      .use(session({
    secret: process.env.SESSION_SECRET || "Test1234",
    resave: false,
    saveUninitialized: false,
    store: MongoStore.create({
      clientPromise: new Promise((resolve, reject) =>
        resolve(mongoose.connection.getClient())
      ),
      dbName: "X",
      stringify: false,
      autoRemove: "interval",
      autoRemoveInterval: 60,
    }),
  }))
      .use(passport.initialize())
      .use(passport.session())
  );
}

I tried it like this but got the same result.

bramvbilsen commented 2 years ago

I just tried the exact same next-connect code, but using a custom express server and everything runs fine

hoangvvo commented 2 years ago

Could you create a reproduction please? Is there any chance you use a subapp (that is nc.use(sth).use(sth)) inside a `.get/post/put/etc since next-connect currently does not support that pattern.

The example app nextjs-mongodb-db has all of their handlers as async but did not get this issue.

aosteraas commented 2 years ago

I've run into this issue in a Next.js project bootstrapped off of nextjs-mongodb-app.

There's one API route that returns a 404, but the code inside the handler is still executed.

I've been unable to create a repro as I can't figure out the conditions that lead to this occurring, but I'd be happy to add you to the repo where it is happening?

hoangvvo commented 2 years ago

I've run into this issue in a Next.js project bootstrapped off of nextjs-mongodb-app.

There's one API route that returns a 404, but the code inside the handler is still executed.

I've been unable to create a repro as I can't figure out the conditions that lead to this occurring, but I'd be happy to add you to the repo where it is happening?

Yeah feel free to do so. Thanks!

aosteraas commented 2 years ago

Done. It's available on branch pain/oauth and can be triggered by going to http://localhost:3000/webex/callback?code=anything

aosteraas commented 2 years ago

I've managed to find the issue, which was ultimately caused by the implementation of passport.deserializeUser.

Apparently on one endpoint (potentially others though I've never seen it elsewhere) the done(err) was also being hit. I have no idea why this is happening only on one handler though.

// fails
passport.deserializeUser((req, id, done) => {
  UserModel.findById(id, { password: 0 }, undefined, (err, user) => {
    if (user) {
      done(null, user);
    }
    done(err);
  });
});

// works
passport.deserializeUser((req, id, done) => {
  UserModel.findById(id, { password: 0 }, undefined, (err, user) => {
    if (user) {
      done(null, user);
    } else {
      done(err);
    }
  });
});

// also works
passport.deserializeUser(async (req, id, done) => {
  try {
    const user = await UserModel.findById(id, { password: 0 }).exec();
    if (!user) {
      done(new Error('user not found'));
    } else {
      done(null, user);
    }
  } catch (err) {
    done(err);
  }
});
hoangvvo commented 2 years ago

@aosteraas Oh okay that makes sense. Are you using the default error handler of next-connect? In the error handler https://github.com/hoangvvo/next-connect/blob/master/src/index.js#L4, it tries to see if the error object has a status property and will use it as status code. If that status code is 404 for some reason, it will respond with 404!

So I would try to console log the err here to see.

passport.deserializeUser((req, id, done) => {
  UserModel.findById(id, { password: 0 }, undefined, (err, user) => {
    if (user) {
      done(null, user);
    }
    done(err);
  });
});