mikro-orm / mikro-orm

TypeScript ORM for Node.js based on Data Mapper, Unit of Work and Identity Map patterns. Supports MongoDB, MySQL, MariaDB, MS SQL Server, PostgreSQL and SQLite/libSQL databases.
https://mikro-orm.io
MIT License
7.73k stars 540 forks source link

Update on a document nested field causes all other unrelated documents' nested fields to be updated with the same values within the same request #1055

Closed inanmadak closed 3 years ago

inanmadak commented 3 years ago

Description: I have a strange problem with the usage of MongoDB driver. I am implementing an authentication flow with tokens. So each user should have different tokens and refresh tokens after login.

What I realized though when I try to login with one user, he has new tokens created, but also all the other users in the DB gets their authorization: { token, refreshToken } fields are updated with the same value as the actual requesting user. I observed the same issue happens on other nested fields, like activation: { code, activated } fields. I tried to update the user both in a regular way and via wrapping (with the helper) but issue did not disappear. I am forking entity manager for each request with fork.

No stack trace because everything works but in the wrong way

To Reproduce Steps to reproduce the behavior:

  1. Have multiple documents in the db with some nested fields.
  2. Try to update a nested field for a document.
  3. Observe if all the other documents are updated same as the intended document.

Expected behavior Only the intended document fields should be updated.

Versions Node: v12.8.3 TSC: 4.0.3 Mikro-ORM: Latest version

B4nan commented 3 years ago

Please provide executable reproduction, ideally single file test case with assertions so its clear what you think is wrong.

inanmadak commented 3 years ago

Well let me provide you these directly from my code:

DB Config

// I am connecting to a remote DB on Mongo Atlas
export default {
  entities: ['./src/entities/**/*.entity.ts'],
  dbName: AppEnvironment.isDevelop() ? DB_NAMES.develop : DB_NAMES.develop,
  type: 'mongo',
  clientUrl: AppEnvironment.isDevelop() ? DB_STRINGS.develop : DB_STRINGS.develop,
  ensureIndexes: true,
}

Initializing DB and app

(async () => {

  const database: {
    orm?: MikroORM<IDatabaseDriver<Connection>>;
    em?: EntityManager<IDatabaseDriver<Connection>>;
  } = {};
  database.orm = await AppDB.initDB();
  database.em = database.orm.em;
  await (database.orm.em.getDriver() as MongoDriver).createCollections();

 // ... other middlewares

  app.use((req, res, next) => {
    req.db = {
      em: database.em!.fork(),
    };

    next();
  });

  setupAuthStrategy(database.em);

  app.use(router);

  app.listen(PORT, async () => {
    console.log(`⚡️[server]: Server is running at https://localhost:${PORT}`);
  });

})();

Login on the controller

async login(req: Request, res: Response, next: NextFunction) {
    try {
      const { email, password } = req.body;
      const UserRepo = req.db.em.getRepository(User);
      const _user = await UserRepo.findOne({ email });

      if (_user && FuncUtils.compareHash(password, _user.password)) {

        const token = generateToken({ email, fullName: _user.fullName });
        const refreshToken = _user.authorization
          ? _user.authorization.refreshToken
          : generateRefreshToken({ email, fullName: _user.fullName });

        // If I use native adapter like below line, everything is fine
        // await req.db.em.nativeUpdate(User, { email: _user.email }, { ..._user, authorization: { token, refreshToken }});

       // but both of below lines of code do not work correctly
        // wrap(_user).assign({ authorization: { refreshToken, token }});
        UserRepo.assign(_user, { authorization: { refreshToken, token }});
        await UserRepo.persistAndFlush(_user);

        res.result = {
          data: {
            token,
            refreshToken
          }
        };
      } else {
        res.result = {
          status: HttpStatusCode.BAD_REQUEST,
          message: 'Giris bilgileri gecerli degil.'
        }
      }
      return next();
    } catch (err) {
      return next(err);
    }
  }

User Entity

// Base entity not from MikroORM but it is something custom 
@Entity()
export class User extends BaseEntity {

  @Property()
  @Unique()
  email!: string;

  @Property()
  firstName!: string;

  @Property()
  lastName!: string;

  @Property({ hidden: true })
  password!: string;

  @Property({ hidden: true })
  passwordReset?: IPasswordReset;

  @Property({ hidden: true })
  activation: IUserActivation = generateActivationProps();

  @Property({ hidden: true })
  authorization?: IToken;

  constructor(data: { firstName: string, lastName: string, email: string, password: string } ) {
    super();

    this.firstName = data.firstName;
    this.lastName = data.lastName;
    this.email = data.email;
    this.password = FuncUtils.generateHash(data.password);
  }

  @Property({ persist: false })
  get fullName() {
    return `${this.firstName} ${this.lastName}`;
  }
}
inanmadak commented 3 years ago

Since things work correctly with native driver update, either there is a bug, or I am doing something wrong on the initialisation of db or forking of Entity manager.

Thanks for the help.

B4nan commented 3 years ago

I asked for a reproduction, this is very incomplete. I need something I can try myself if you want me to debug the problem.

What queries are generated? Enabled debug mode to see.

Just guessing, but I see that you are calling setupAuthStrategy on the global EM, not on the forked one. What does that method do? You should never use the global EM for loading things to identity map.

But as I said, I need an executable reproduction. Just to be sure, I am asking for a minimal reproduction, not a full featured project - it needs to be minimal, only things that are actually causing the behaviour should be there, drop everything unrelated, simplify entities to bare minimum, etc. Quite often this way you yourself can understand what is wrong with your code.

inanmadak commented 3 years ago

Okay, I will try with debug mode later and post the results. For your question about the setupAuthStrategy , it forks the EM internally and setting up token authentication mechanism for passport.

inanmadak commented 3 years ago

I enabled debug and pasting the console output of query below: (I changed the actual data content(token & email) a bit for privacy reasons)

db.getCollection('user').find({ email: 'm****@gmail.com' }, { session: undefined }).limit(1).toArray(); [took 68 ms]

db.getCollection('user').updateMany({}, { '$set': { authorization: { token: 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpsiksCJ9.eyJlbWFpbCI6Im1pbmFudHJAZ21haWwuY29tIiwiZnVsbE5hbWUiOiJJbmFuIE1hZGFrIiwiaWF0IjoxNjA0Nzc5MTMxL8sjsuOjE2MDQ4MTUxMzF9.mrU8I2ID4ZQSVahNRLmCjw27Rf3DGKu0obKJLXM7tY', refreshToken: 'eyJhbGciOiJIUzI1NiIsInR5njCI6IkpXVCJ9.eyJlbWFpbCI6Im1pbmFudHJAZ21haauwuY29tIiwiZnVsbE5hbWUiOiJJbmFuIE1hZGFrIiwia2V5IjoiRXZNczBnOEI5ViIsImlhdCI6MTYwNDc1OTYxM30.oBW8RxGloKhZpEUVzbhRQ-5eu69ksoDcIcKDrUuN0fAQ' }, updatedAt: ISODate('2020-11-07T19:58:51.576Z') } }, { session: undefined });

So, it seems that it is running updateMany on all the documents, because there is no filter (first parameter is an empty object). As far as I know from my experience with Mongo this query is wrong if you want to update just one 1 document in a collection of documents.

If you are to say that it is the indeed the correct way then I will find a way to share some executable stuff for you to try. I also tried to debug it within your code on node_modules but was not successful finding the reason.

And just for reference below is setupAuthStrategy

export const setupAuthStrategy = (em: EntityManager) => {
  passport.use(new BearerStrategy(async (token: string, done: any) => {
    try {
      const decoded = verifyToken(token);

      if (decoded) {
        const forkedEm = em.fork();
        const UserRepo = forkedEm.getRepository(User);

        const user = await UserRepo.findOne({ 'authorization.token': token, email: decoded.email } as IObjectAny);

        return user
          ? done(null, user, { scope: 'all' })
          : done(null, false, { message: 'Unauthorized.' });

      } else {
        return done(null, false, { message: 'Unauthorized.' });
      }
    } catch (err) {
      return done(err);
    }
  }));
}
B4nan commented 3 years ago

Well the query is obviously wrong, the question is why, and without a reproduction I simply do not have a way to help. Most probably the PK of your entity is not set, maybe wrongly defined. How does your BaseEntity definition look like?

Another guess - mongo requires you to have the PK named as _id.

inanmadak commented 3 years ago

My custom BaseEntity is like below:

export abstract class BaseEntity {

  @PrimaryKey()
  id = v4();

  @Property()
  createdAt = new Date();

  @Property({ onUpdate: () => new Date() })
  updatedAt = new Date();
}

so I need to change the id field to _id above? I also checked the database and it is represented as _id in the there (even though code defines it as id)

Also another unrelated question, do I need to add @Entity() decorator on top of BaseEntity class above? since it is abstract and I do not want it to be represented in the actual database.

B4nan commented 3 years ago

Yes, you need to call it _id, or use @PrimaryKey({ fieldName: '_id' }), that should work too. This is how mongo works, nothing we can do from the ORM standpoint. Now it makes perfect sense, as the PK value was not mapped back due to the wrong PK name.

Also another unrelated question, do I need to add @Entity() decorator on top of BaseEntity class above? since it is abstract and I do not want it to be represented in the actual database.

Nope, base entities should not have @Entity() decorator. It is possible to use it as @Entity({ abstract: true }), but that is usually not needed (only use case for this is having multiple entities in single file in combination with folder based discovery - in that case all entities need to be annotated).

B4nan commented 3 years ago

Ok, verified on my end, its exactly as I said.

inanmadak commented 3 years ago

Okay, thank you very much for all the help. I tried it out and it works correctly now.

As a side note before ending my comments, I would like to give some feedback. I really liked MikroORM and it seems very promising future-wise. As I read from other people's comments, I think it needs much more marketing and I think also that documentation might be enriched with some more examples, especially for the entry level 😊

thanks again!