needle-innovision / nestjs-tenancy

Multi-tenancy approach for nestjs - currently supported only for mongodb with mongoose
MIT License
186 stars 58 forks source link

Problem resolving dependencies that inject tenancy models #13

Closed gabrielrezk closed 2 years ago

gabrielrezk commented 2 years ago

Hi @sandeepsuvit, thanks for developing this package! I've been trying it a little bit and it's awesome.

I've been trying to include it in one of my projects and I think I found an issue. It basically has to do with the possibility of resolving dependencies that inject tenancy models using ModuleRef.

In this particular project, we implemented the Unit of Work pattern, I don't know if you're familiar with it, but it basically solves transaction issues among different models.

One of the methods it implements is called getRepository which basically resolves a repository dependency using ModuleRef:

async getRepository<T extends TransactionalRepository>(
    RepositoryClass,
  ): Promise<T> {
    const repository = await this.moduleRef.resolve<T>(
      RepositoryClass,
      this.contextId,
      {
        strict: false,
      },
    );
    return repository;
  }

An example of a repository could be this one:

import { InjectTenancyModel } from '@needle-innovision/nestjs-tenancy';
import { Injectable, Scope } from '@nestjs/common';
import { Model } from 'mongoose';

import {
  EntityRepository,
  Filter,
} from '../database/repositories/entity.repository';
import { Cat, CatDocument } from './cat.schema';

@Injectable({ scope: Scope.TRANSIENT })
export class CatsRepository extends EntityRepository<CatDocument, Filter> {
  constructor(
    @InjectTenancyModel(Cat.name)
    private readonly catModel: Model<CatDocument>,
  ) {
    super(catModel);
  }
}

The problem is that whenever I do the following call:

const catsRepository = await this.unitOfWork.getRepository<CatsRepository>(
      CatsRepository,
);

I get this error:

[Nest] 11792  - 12/29/2021, 1:06:59 PM   ERROR [ExceptionsHandler] Cannot read property 'headers' of undefined
TypeError: Cannot read property 'headers' of undefined
    at Function.getTenantFromRequest (/Users/gabrielrezk/CreateThrive/Proof of concept/NestJS/db-per-tenant/node_modules/@needle-innovision/nestjs-tenancy/dist/tenancy-core.module.js:126:39)
    at Function.getTenant (/Users/gabrielrezk/CreateThrive/Proof of concept/NestJS/db-per-tenant/node_modules/@needle-innovision/nestjs-tenancy/dist/tenancy-core.module.js:119:29)
    at InstanceWrapper.useFactory [as metatype] (/Users/gabrielrezk/CreateThrive/Proof of concept/NestJS/db-per-tenant/node_modules/@needle-innovision/nestjs-tenancy/dist/tenancy-core.module.js:195:71)
    at Injector.instantiateClass (/Users/gabrielrezk/CreateThrive/Proof of concept/NestJS/db-per-tenant/node_modules/@nestjs/core/injector/injector.js:304:55)
    at callback (/Users/gabrielrezk/CreateThrive/Proof of concept/NestJS/db-per-tenant/node_modules/@nestjs/core/injector/injector.js:48:41)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)

Failing at the moduleRef.resolve call inside the getRepository function. Any ideas on what could be the problem?

Thanks in advance, I hope you find this context useful.

sandeepsuvit commented 2 years ago

Hi @gabrielrezk thank you for writing to us and for your kind words :)

Coming to the issue, I am guessing the unit of work pattern that you have used might not be the problem here, provided the transaction blocks that you are trying to commit/call are under the same connection (where your tenant tables are) which is resolved to you by the library.

I also noticed that you have included @Injectable({ scope: Scope.TRANSIENT }) at the top of your class. Does this mean you are trying to provide new instance of this class to every requestor that needs it?

After checking the logs i observed that it originated from https://github.com/needle-innovision/nestjs-tenancy/blob/4597ac68041228b0e4ee5b6f10e4e8e71c72a79d/lib/tenancy-core.module.ts#L211

private static getTenantFromRequest(isFastifyAdaptor: boolean, req: Request, tenantIdentifier: string) {
        let tenantId = '';

        if (isFastifyAdaptor) { // For Fastify
            // Get the tenant id from the header
            tenantId = req.headers[`${tenantIdentifier || ''}`.toLowerCase()]?.toString() || ''; // ---> This is where it failed because it was not able to resolve the `req` object. 
        } else { // For Express - Default
            // Get the tenant id from the request
            tenantId = req.get(`${tenantIdentifier}`) || '';
        }

        // Validate if tenant id is present
        if (this.isEmpty(tenantId)) {
            throw new BadRequestException(`${tenantIdentifier} is not supplied`);
        }

        return tenantId;
    }

Are you using a fastify adaptor?

Can you try removing the scope and check the result please.

gabrielrezk commented 2 years ago

Thanks for your fast reply!

Coming to the issue, I am guessing the unit of work pattern that you have used might not be the problem here, provided the transaction blocks that you are trying to commit/call are under the same connection (where your tenant tables are) which is resolved to you by the library.

Right, I'm guessing it has something to do with the way the library registers the tenancy models. Because it's in that ModuleRef line that the code breaks.

I also noticed that you have included @Injectable({ scope: Scope.TRANSIENT }) at the top of your class. Does this mean you are trying to provide new instance of this class to every requestor that needs it?

Exactly, the getRepository method does a little more than just resolve the dependency. It also sets the transaction session in the repository object in case one started. That's why the scope is transient, if we kept the same repository instance there could be problems with transactions running in parallel.

After checking the logs i observed that it originated from https://github.com/needle-innovision/nestjs-tenancy/blob/4597ac68041228b0e4ee5b6f10e4e8e71c72a79d/lib/tenancy-core.module.ts#L211

private static getTenantFromRequest(isFastifyAdaptor: boolean, req: Request, tenantIdentifier: string) {
        let tenantId = '';

        if (isFastifyAdaptor) { // For Fastify
            // Get the tenant id from the header
            tenantId = req.headers[`${tenantIdentifier || ''}`.toLowerCase()]?.toString() || ''; // ---> This is where it failed because it was not able to resolve the `req` object. 
        } else { // For Express - Default
            // Get the tenant id from the request
            tenantId = req.get(`${tenantIdentifier}`) || '';
        }

        // Validate if tenant id is present
        if (this.isEmpty(tenantId)) {
            throw new BadRequestException(`${tenantIdentifier} is not supplied`);
        }

        return tenantId;
    }

Are you using a fastify adaptor?

We checked the same line. The weird thing is that when we send the request it goes through the controller and the service until it reaches the getRepository part. But the error suggests there's a problem with the request.

Right, we are using Fastify.

Can you try removing the scope and check the result please.

I've just tried that but I got the same error.

gabrielrezk commented 2 years ago

Well, I think I found the issue. It's nothing to do with this package, it's related to how the ModuleRef class works.

If we're using the request inside one of the modules that get resolved by ModuleRef, we should do this first:

this.moduleRef.registerRequestByContextId(/* YOUR_REQUEST_OBJECT */, contextId);

Otherwise, it will be undefined. This is documented here: https://docs.nestjs.com/fundamentals/module-ref#registering-request-provider

So, my solution was to change the repository function as follows:

async getRepository<T extends TransactionalRepository>(
    RepositoryClass,
  ): Promise<T> {
    this.moduleRef.registerRequestByContextId(request, this.contextId);
    const repository = await this.moduleRef.resolve<T>(
      RepositoryClass,
      this.contextId,
      {
        strict: false,
      },
    );
    return repository;
  }

Sorry for the inconvenience and thanks for your help, you can close this issue.

May I also ask if this library stores the DB connections in order to reuse them across the application?

sandeepsuvit commented 2 years ago

Hi @gabrielrezk glad that you were able to figure it out on your own and thanks for pointing me to that link. It helps a lot

Regarding your query on the connections storage, in the current code the connections once created per request gets cached in a map and reused next time. Else a new connection is created.

        const exists = connMap.has(tenantId);

Hope this helps :)

gabrielrezk commented 2 years ago

Great @sandeepsuvit! Awesome work. Have a good one!