Optimising a simple graphql mutation resolver

Question

I am writing a resolver for a typical updateUser mutation with node, Apollo Server and mongoDB. I want to make it such that, when the email input is provided, it will update the email; when the password input is provided, it will hash and then update the password; and when the plan input is provided, it will update the plan - but I only want the compulsory field when calling the mutation at the front-end to be id (and at least one of email, password or plan).

Here is my code:

export const updateUser = async (
  _: undefined,
  { input: { id, email, password, plan } }: any
): Promise<any> => {
  const collection = getCollection(USERS_COLLECTION_NAME);

  try {
    const updateWith = (() => {
      const toUpdate: any = {};

      if (email) toUpdate['email'] = email;
      if (password)
        toUpdate['hashedPassword'] = (async () =>
          await bcrypt.hash(password, 10))();
      if (plan) toUpdate['plan'] = plan;

      return toUpdate;
    })();
    const updateResult = await collection.findOneAndUpdate(
      {
        _id: new mongodb.ObjectId(id), // new mongodb.ObjectId needed, otherwide null; converts to BSON
      },
      {
        $set: updateWith,
      },
      { returnDocument: 'after' } // returns updated user rather than user before updates
    );

    if (!updateResult?.value) {
      throw new HttpError(500, 'Could not update user');
    }
    // note: it updates the user even if the value provided is the same as the existing value

    return updateResult.value;
  } catch (error: any) {
    throw new HttpError(error.statusCode, error.message);
  }
};

It would be great to get some feedback and become a better developer, hence I have a few questions:

1. Could the updateWith function be improved?
2. Is it correct to add another try-catch block inside the inner async function (near if (password)), despite there being an outer one?
3. Is it bad practice to have one resolver to update multiple fields rather than creating separate resolvers for each field (e.g. a mutation for changeUserEmail, changeUserPlan, changeUserPassword, etc)? The code has to run many if statements, so I’m wondering whether it’s using unnecessary bandwidth
4. What should the type be for error in the catch block, rather than any?
5. Should updateWith be outside the try block?

Thanks for any advice.

Answer 1

Introduction

Exposing a single mutation to update a whole model is one of the pitfalls for most inexperienced developers in a domain driven environment. I've made the mistake countless times.

The queries and mutations you expose should be driven by the use-cases of your business.

In this answer, I'll be assuming a few things regarding your use-cases. You'll have to adapt them to your needs.

What do you mean use-cases?

When I talk about use-cases, I think a better phrase would be "user oriented actions" or "user oriented scenarios". Essentially, what actions can a user trigger? The specific "action" names from the user's point of view should be reflected in your backend.

For example, it's unlikely that there is a use-case where the user will at the same time update their email, password and plan all at once? Most flows are separated.

1. A user can change plans
2. A user can reset their password
3. A user can change their email

These are specific use-cases which have their own independant flows. For example, in the password reset flow, the user clicks "reset password" button, the user needs to check their email for the reset password link, then the user needs to choose a new password.

Ubiquitous language

In the book, Domain Driven Design by Eric Evans, he illustrates the importance of building a vocabulary understandable between the developer team and the domain expert. A domain expert is simply someone who understands the business that your application will reflect. In this scenario, it would be your user.

When you interact with your users and talk about some of the processes that exist, the concept of "udpateUser" probably wouldn't make much sense to the user. If the user has an issue with the password reset, they're not going to contact you and say "I'm having issues updating my user in your database", they'll probably say something like "I'm unable to reset my password".

And this is an interesting phenomenon. Sure, if you work by yourself on an application you'll probably be able find the issue at hand quickly since it's your code. However, realistically, in a business you'll work on a large codebase that may have many developers and you may find yourself having to debug a scenario that you may have never worked on.

It would be helpful if what the user is describing is reflected exactly in the backend. So you don't have to do some mental gymnastics to understand that "reset password" = "update user" (even if this scenario is simple).

Expose a resolver for each use-case

I would probably do something like this.

input ForgetPasswordInput {
  email: String!
}

input ResetPasswordInput {
  password: String!
  # The reset email contains a automatically generated url, 
  # usually containg some form of a hash, which can be passed 
  # here along with the new password to validate that it comes from the user
  secretHash: String!
}

enum Plan {
  PLAN_ONE
  PLAN_TWO
}

input ChangePlanInput {
  plan: Plan!
}

input ChangeEmailInput {
  email: String!
}

type Mutation {
  # Unauthenticated actions
  forgetPassword(input: ForgetPasswordInput!): Boolean!
  resetPassword(input: ResetPasswordInput!): Boolean!

  # Authenticated actions
  changePlan(input: ChangePlanInput!): User!
  changeEmail(input: ChangeEmailInput!): User!
}

export const forgetPassword = () => {
  // todo
}

export const resetPassword = () => {
  // todo
}

export const changePlan = () => {
  // todo
}

export const changeEmail = () => {
  // todo
}

Here you're probably wondering where did the user id go? Well, you don't need the id for the forgetPassword and resetPassword mutation since the user probably won't be authenticated. And if they do request a password change while authenticated, it can go through the same flow (doesn't matter).

For the changePlan and the changeEmail mutation, you don't need to pass the current user's id since they are already authenticated. You already have access to the user's access_token (or jwt token, etc). You can provide apollo a custom function (where you fetch the user id using the access token) and inject it in graphql's context.

Something like:

const server = new ApolloServer({
  context: ({ req }) => ({
    // create a function to fetch the user id from the token
    userId: getUserIdFromAccessToken(req.headers.authorization)
  })
  // other props
});

Once you inject it properly in the context, you can then access it via the resolver like so:

export const changePlan = (parent, args, context) => {
  const { userId } = context; // I named it "userId" but it's completely customizable
}