How I wasted hours debugging a silly mistake

How I wasted hours debugging a silly mistake

Never return reference of a private object to the function caller

Instead of telling the real story, I'll walk you through a case where the silly mistake can happen very easily.

Imagine, we have a nodejs server and we have got a requirement, the API needs to send a welcome message to the logged-in user. This is very simple, right?

type Message = { title: string; body: string; };

function getWelcomeMessage(): Message {
    const welcomeMessage: Message = {
        title: "Welcome",
        body: "Welcome to the Fun World!"
    };
    return welcomeMessage;
}

Now we can call this getWelcomeMessage function from the /login route handler when login succeeded.

app.post('/login', authMiddleware, (req, res) => {
    // Run other logics here, maybe set auth token in cookie.
    const message = getWelcomeMessage();
    res.send(message);
});

The client will want to change the message once they get a better welcome message for sure. Instead of hardcoding the message in the source code, it will be better to move it to a file or environment variable.

# .env file
WELCOME_MESSAGE_TITLE="Welcome"
WELCOME_MESSAGE_BODY="Welcome to the Fun World!"
type Message = { title: string; body: string; };

function getWelcomeMessage(): Message {
    const welcomeMessage: Message = {
        title: process.env.WELCOME_MESSAGE_TITLE,
        body: process.env.WELCOME_MESSAGE_BODY
    };
    return welcomeMessage;
}

Now the client do not need to change the source code just for changing the welcome message. Cool!

Very soon another requirement comes, we need to support another language. For this case, we need to support Bangla and English. So, lets update the .env file and getWelcomeMessage function like below.

# .env file
WELCOME_MESSAGE_TITLE_EN="Welcome"
WELCOME_MESSAGE_BODY_EN="Welcome to the Fun World!"
WELCOME_MESSAGE_TITLE_BN="স্বাগতম"
WELCOME_MESSAGE_BODY_BN="মজার বিশ্বে স্বাগতম!"
type Language = 'en' | 'bn';
type Message = { title: string; body: string; };

function getWelcomeMessage(language: Language): Message {
    let welcomeMessage: Message;
    switch(language) {
        case 'en':
            welcomeMessage = {
                title: process.env.WELCOME_MESSAGE_TITLE_EN,
                body: process.env.WELCOME_MESSAGE_BODY_EN
            }
            break;
        case 'bn':
            welcomeMessage = {
                title: process.env.WELCOME_MESSAGE_TITLE_BN,
                body: process.env.WELCOME_MESSAGE_BODY_BN
            }
            break;
        default:
            // Set default message in English
            welcomeMessage = {
                title: process.env.WELCOME_MESSAGE_TITLE_EN,
                body: process.env.WELCOME_MESSAGE_BODY_EN
            }
    }
    return welcomeMessage;
}

This function works fine. However, notice that, this function will read from environment each time it will be executed. Reading from file or environment is costly. It is a good practice to load environment variables in memory. Let's do that.

type Language = 'en' | 'bn';
type Message = { title: string; body: string; };
type WelcomeMessages = Record<Language, Message>;

// When the file first executed the environment variables will be
// loaded and stored into "welcomeMessages" variable.
const welcomeMessages: WelcomeMessages = {
    en: {
        title: process.env.WELCOME_MESSAGE_TITLE_EN,
        body: process.env.WELCOME_MESSAGE_BODY_EN
    },
    bn: {
        title: process.env.WELCOME_MESSAGE_TITLE_BN,
        body: process.env.WELCOME_MESSAGE_BODY_BN
    }
}

function getWelcomeMessage(language: Language): Message {
    // Read from the cached variable, no need to read from .env again
    return welcomeMessages[language];
}

Looks very simple and easy right? But we have already made a mistake. If you did not notice that, don't worry, we'll figure it out soon.

A few weeks later, a new requirement came. Client wanted to display current user's name in the welcome message body. That means, we need a placeholder in the welcome message body and we need to be able to replace the placeholder with the name of the current user. To do that, I added a placeholder like this <name>.

# .env file
WELCOME_MESSAGE_TITLE_EN="Welcome"
WELCOME_MESSAGE_BODY_EN="Hi <name>, welcome to the Fun World!"
WELCOME_MESSAGE_TITLE_BN="স্বাগতম"
WELCOME_MESSAGE_BODY_BN="<name>, মজার বিশ্বে স্বাগতম!"

Now, once we get the message, we can replace the placeholder in our route handler.

app.post('/login', authMiddleware, (req, res) => {
    const message = getWelcomeMessage(req.user.language);
    // You may argue we should implement this replace logic inside
    // getWelcomeMessage function. For the sake of this example we
    // are doing this here.
    message.body.replace('<name>', req.user.name);
    res.send(message);
});

Did you notice the mistake this time? Let's say we test our project locally with our test user account. We will get expected response from our login route. However, once we deploy this project in server, and many users started to login, everyone will get only one name is there. Seems like the name is hardcoded.

So the summary is, when the login route handler function executed for the first time it will work fine. But it will not work for the subsequent executions.

const message = getWelcomeMessage('en');
message.body.replace('<name>', 'Mazedul');
// message.body == "Hi Mazedul, welcome to the Fun World!"
const message2 = getWelcomeMessage('en');
message.body.replace('<name>', 'Hashnode');
// message2.body == "Hi Mazedul, welcome to the Fun World!"
// Name did not changed for the second execution!!!

Why it is not working for the subsequent executions? Well the bug is very simple, our getWelcomeMessage function is buggy. Let's have a closer look.

// When the file first executed the environment variables will be
// loaded and stored into "welcomeMessages" variable.
const welcomeMessages: WelcomeMessages = {
    en: {
        title: process.env.WELCOME_MESSAGE_TITLE_EN,
        body: process.env.WELCOME_MESSAGE_BODY_EN
    },
    bn: {
        title: process.env.WELCOME_MESSAGE_TITLE_BN,
        body: process.env.WELCOME_MESSAGE_BODY_BN
    }
}

function getWelcomeMessage(
    language: Language,
    userName: string
): Message {
    // Read from the cached variable, no need to read from .env again.
    const message = welcomeMessages[language];
    // message variable is now pointing to the cached variable
    // welcomeMessages.en or welcomeMessages.bn depending on the
    // language.
    return message;
}

When we replace the placeholder in the route handler function it mutate the cached welcomeMessages variable. Therefore, subsequent executions do not get any <name> placeholder to replace.

In this small example project it is very easy to understand the bug. However, in a complex large project with several level of abstractions it can be very difficult to debug this issue.

The fix is very easy. Instead of mutating the cached object, we need to create a copy and then modify it. The getWelcomeMessage function should not return the reference to the cached object, instead it should return a copy object.

function getWelcomeMessage(
    language: Language,
    userName: string
): Message {
    const message = { ...welcomeMessages[language] };
    return message;
}

Now the function creates a new object each time it is executed. So the bug is fixed. You can also consider using Lodash cloneDeep function for a complex object.

Moral: Never return reference of a private object to the function caller.