CODEWITHSUNDEEP
javascriptnode.jsapiexpressroutes
andrejmonteiro
andrejmonteiro

Reputation: 67

NodeJS Express overlapping routers

I've encountered some bug(?) while building a NodeJS express API. Apparently some endpoints overlap each other which means I can't reach them, the request doesn't end until it timeouts.

ex:

const load_dirs = (dirs, filename) => {
    const router_files = readdirSync(`./${dirs}`).filter(file => file.endsWith(`${filename}.js`));
    router_files.forEach(file => {
        const { 
            getUsers, getUser, addUser, editUser, removeUser, loginUser, updateUserStatus
        } = require(`../${dirs}/${file}`);

        router
            .route('/')
            .get(cors(corsOptions), getUsers)
            .post(cors(corsOptions), addUser);

        router
            .route('/login')
            .post(cors(corsOptions), loginUser);
        
        router
            .route('/:userId')
            .get(cors(corsOptions), getUser)
            .post(cors(corsOptions), editUser)
            .put(cors(corsOptions), removeUser);

        router
            .route('/:userId/status')
            .post(cors(corsOptions), updateUserStatus);
    });
}

['methods'].forEach(e => load_dirs(e, 'users'));

module.exports = (router);

If the login method is above the get user by Id, i can't reach the user by Id or vice versa. And since this is a somewhat unique way of using node (haven't found anyone else who uses it), i can't figure it out...

What am I missing? Thanks in advance!

Edit 1:

As requested in the comments, i leave here the getUsers method:

exports.getUsers = async (req, res, next) => {
    try  {
        await new Promise(async (resolved, rejected) => {
            let conn;

            try {
                conn = await db.pool.getConnection();
                const result = await conn.query(`
                    SELECT * FROM users
                `)

                .then((resp) => {                    
                    if (!resp.length) {
                        res
                            .status(200)
                            .json({ success: true, message: general.noResults });
                        resolved(resp);
                        return;
                    }

                    const data = resp;

                    res
                        .status(200)
                        .json({ success: true, utilizadores: data });
                    resolved(resp);
                })

                .catch ((err) => {
                    if (mode() === 'development' || mode() === 'test') console.log(`Error: ${ err }.`);
                    res
                        .status(400)
                        .json({ success: true, message: general.badRequest });
                    rejected(err);
                })
            } catch (err) {
                if (mode() === 'development' || mode() === 'test') console.log(`Error: ${ err }`);
                res
                    .status(400)
                    .json({ success: false, message: general.badRequest });
                rejected(err);
            } finally {
                if (conn) {
                    conn.end();
                }
            }
        })

        .then((message) => {
            if (mode() === 'development' || mode() === 'test') console.log(message);
        })

        .catch((err) => {
            if (mode() === 'development' || mode() === 'test') console.log(err);
        });
    } catch (err) {
        if (mode() === 'development' || mode() === 'test') console.log(`Error: ${ err }.`);
        res
            .status(400)
            .json({ success: false, message: general.badRequest });
    }
}

Upvotes: 0

Views: 762

Answers (2)

jfriend00
jfriend00

Reputation: 707158

This structure:

      router
        .route('/:userId')
        .get(cors(corsOptions), getUser)
        .post(cors(corsOptions), editUser)
        .put(cors(corsOptions), removeUser);

Creates a wildcard route that matches ANY http request with a top level path such as:

/abc
/xxx
/123

So, when you go through the first iteration of your loop and create the first one of these top level wildcard routes, it will take precedence over any other top level route that is added to the router after it. Thus, it will take the requests for the future iterations of the loop that try to define handlers for /login.

But, this whole model is busted. Even if you remove the wildcard route, it's not the only problem. Since each iteration of your loop is just defining the exact same routes over and over again, the routes defined in the first iteration of the loop will take precedence over all the other routes in subsequent iterations of the loop.

Your subsequent iterations of the loop need to be defining different routes. If we remove all the complication you have here and just show:

app.get("/login", someHandler1);
app.get("/login", someHandler2);

Then, the second definition will never get a chance to see the /login route unless someHandler1 happens to decide to not send a response and instead call next() to pass control on down the chain.

So, the whole model of defining the same routes over and over again in a loop seems flawed. In addition, I would tend to avoid a top level wildcard route because it creates conflicts with other top level routes as you grow your features. Instead, either do /id/:userId or define a regex that matches only a top level userId, but leaves room for other top level routes that won't match that regex.

To come up with a solution, I'd have to understand better what exactly you're trying to do when you have multiple files with all the same exported request handlers and all the same route paths. On the surface, that just doesn't make sense to me. You don't handle the exact same incoming request multiple times. You handle it once so there needs to be one piece of code (not N pieces of code) that handle any given route.

FYI, here's a simplified version of your getUsers() function. You don't need the wrap in a promise and don't need so much duplicated error handling:

exports.getUsers = async (req, res, next) => {
    let conn;
    try {
        conn = await db.pool.getConnection();
        const result = await conn.query(`SELECT * FROM users`);
        if (!result.length) {
            res.json({ success: true, message: general.noResults });
        } else {
            res.json({ success: true, utilizadores: results });
        }
    } catch(err) {
        if (mode() === 'development' || mode() === 'test') console.log(`Error:`, err);
        res.status(400).json({ success: true, message: general.badRequest });
    } finally {
        if (conn) {
            conn.end();
        }
    }
});

Summary of changes:

  1. Remove await new Promise() wrapper as that is not necessary
  2. Remove .then() and .catch() so we're only using await and try/catch
  3. Put all error handling in one catch block
  4. Coalesce all errors handlers into one place
  5. Fix logging of error object
  6. Remove .status(200) since 200 is already the default.

FYI, I would argue that you do want to log errors, even in production, not only in development. If you start having problems in the deployed, production server, you will want to have log info that tells you what the actual internal errors are. So, I'd suggest you remove the if that only logs errors in development or test.

Upvotes: 1

kevintechie
kevintechie

Reputation: 1521

The loop is the problem. I'm guessing that you're trying to set up a route per file. You don't need to do this because you can use route parameters to substitute userId in your file access code.

import express from 'express';

const app = express();
const router = express.Router();
app.use(router);

const port = 3000;

const render1 = (req, res) => {
  res.send(`Render 1 says: hello world`);
}

const render2 = (req, res) => {
  res.send(`Render 2 says: logging in user...`);
}

const render3 = (req, res) => {
  res.send(`Render 3 says: reading user data from file ./userdata/${req.params.userId}.json`);
}

const render4 = (req, res) => {
  res.send(`Render 4 says: User ${req.params.userId} is logged in.`);
}

router.route('/').get(render1);

router.route('/login').get(render2);

router.route('/:userId').get(render3);

router.route('/:userId/status').get(render4);


app.listen(port, () => {
  console.log(`Example app listening at http://localhost:${port}`)
})

Upvotes: 1

Related Questions