Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Microservices Support missing #37

Open
moparlakci opened this issue Nov 13, 2022 · 12 comments
Open

Microservices Support missing #37

moparlakci opened this issue Nov 13, 2022 · 12 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@moparlakci
Copy link

Hello

Thanks for the great tenancy module!

The only this that is missing is, microservices support.

We are using the Request/Response pattern from ClientProxy to talk to other microservices.

We use it also in our JwtStrategy (Passport), to validate user data from the database, which is stored in another microservice called 'auth'. We do that by sending a messagepattern to auth microservice.
But this is a tcp connection and therefore doesnt have Request object, so we get req.get is not a function error.

To give an example

We have an auth microservice with an auth module, and auth controller that looks like the below

`

@Controller('')
export class AuthController {
  constructor(
    private readonly authService: AuthService
    ) {} 


 @Post('login')
  create(@Body() loginDto: LoginDto) {
    return this.authService.login(loginDto);
  }


@MessagePattern({ cmd: 'get_user_with_permissions' })
  async getUser(@Payload() data, @Ctx() context: RmqContext): Promise<any> { 

  const userWithPermissions = await this.authService.getUserWithPermissions(data.id, data.tenant);

  const channel = context.getChannelRef();
  const originalMsg = context.getMessage();

  channel.ack(originalMsg); 
 
  
  return userWithPermissions;
  }}

  }

`

in our JwtStrategy our validate function looks like below

`

async validate(payload: any) { 

const { id, tenant } = payload;

const userWithPermissions = await firstValueFrom(this.authClient.send({ cmd: 'get_user_with_permissions' }, {
  id,
  tenant
}).pipe(
  tap((res) => { 
     return res;
  }),
  catchError((err) => {
    throw new UnauthorizedException();
  }))); 


return userWithPermissions;

}

`

in the Auth context we should be able to switch to another tenant connection, somehow.
and in the jwtStrategy we should send the Tenant Identifier by some means via a json property or so.

So a little change should be made to getTenantFromRequest or getTenant function, to check if the call is rpc or http..

So in our Auth Controller, the message comes in via TCP, RPC. And the nestjs-tenancy module cannot know which tenant to use. So basically microservices support is missing.

I hope we can add microservices support.

@moparlakci
Copy link
Author

Knipsreq_GetErrorel

@moparlakci
Copy link
Author

moparlakci commented Nov 13, 2022

The function where it needs to be checked

sdfdsfdsfsdfds546

The body of Req property when its a RPC/Microservices call looks like below

req

@sandeepsuvit sandeepsuvit added enhancement New feature or request help wanted Extra attention is needed labels Nov 13, 2022
@sandeepsuvit
Copy link
Contributor

Hi @moparlakci thanks for writing in. Currently this library is intended to work under a http REQUEST scoped session. Hence microservice based request isn't something that this library would be a able to process at the moment. This is a possible candidate for a PR. Happy to review it if someone can give it a try.

@moparlakci
Copy link
Author

moparlakci commented Nov 13, 2022

I think I fixed it myself by changing the TenancyCoreModule a little..

Just if you add a check to the following code inside
static getTenantFromRequest(isFastifyAdaptor, req, tenantIdentifier)
will be sufficient at the moment.

`

 private static getTenantFromRequest(
    isFastifyAdaptor: boolean,
    req: Request,
    tenantIdentifier: string,
  ) {

    let request = req;
    const { context } = req;
    if(context.getType() === 'rpc') {
        request = context.switchToRpc().getData();
        // inside microservice call..
        // just return a property from the sent data object -- could be dynamic using tenantIdentifier
        return request.data[tenantIdentifier];
    } else if(context.getType() === 'http') {
        request = context.switchToHttp().getRequest();
    }  


    let tenantId = '';

    if (isFastifyAdaptor) {
      // For Fastify
      // Get the tenant id from the header
      tenantId =
      request.headers[`${tenantIdentifier || ''}`.toLowerCase()]?.toString() ||
        '';
    } else {
      // For Express - Default
      // Get the tenant id from the request
      tenantId = request.get(`${tenantIdentifier}`) || '';
    }

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

    return tenantId;
  }
`

@moparlakci
Copy link
Author

moparlakci commented Nov 13, 2022

Because otherwise any controller endpoint using a MessagePattern or EventPattern decorator is not useable with this package.

@sandeepsuvit
Copy link
Contributor

@moparlakci true that and glad that you solved it on your own. Can you raise a PR for this with a unit test case as-well for the microservice change bit that you have made. I would review it and release a new change.

@moparlakci
Copy link
Author

moparlakci commented Nov 14, 2022

Hello, changed the functions to use context in stead of req properties. But injection of the ExecutionContext is still a problem at the moment.

the following injects a Request object into context, but its expecting a ExecuitionContext and therefore getType() method rasies an error.

private static createTenantContextProvider(): Provider { return { provide: TENANT_CONTEXT, scope: Scope.REQUEST, useFactory: ( context: ExecutionContext, moduleOptions: TenancyModuleOptions, adapterHost: HttpAdapterHost, ) => this.getTenant(context, moduleOptions, adapterHost), inject: [CONTEXT, TENANT_MODULE_OPTIONS, DEFAULT_HTTP_ADAPTER_HOST], }; }

@moparlakci
Copy link
Author

Hi @sandeepsuvit,

Can you check my fork?
Currently the Request type check is not pretty coded, but commits are always welcome.
Typescript check for the incoming request was a bit difficult because of the class Request for HTTP calls and RequestContext for anyother call.

Couldnt use context functions, something to do with the injection of the CONTEXT token which is an alias for the REQUEST token.

See my fork here
https://github.com/moparlakci/nestjs-tenancy/blob/master/lib/tenancy-core.module.ts

Tests are passing..
https://github.com/moparlakci/nestjs-tenancy/blob/master/tests/src/dogs/dogs.service.ts

Thanks in advance

@moparlakci
Copy link
Author

@sandeepsuvit any updates on this? I will also debug further..

@Robokishan
Copy link

This is exactly what i was trying to do @moparlakci . This entire library is written on Request architecture. anything which touches this flow will become request scopped. meaning nestjs will create new instance on every request for each repository each services. i think this is not efficient. if it would have been interceptor or some other logic then it would be more efficient. i have used this in production it this is causing issues with large traffic. I am re writing entire logic for multi tenancy. i would checked this before using this library. @moparlakci I think I can help you rewrite logic

@Robokishan
Copy link

Robokishan commented May 2, 2023

@moparlakci i tried your code. test cases are passing but

app.connectMicroservice({
transport: Transport.TCP,
});

await app.startAllMicroservices();

is not working. When i make http request it hangs is there any reason for not working this ? does not throw any error but application just hangs. i am not getting any response.

@Robokishan
Copy link

Robokishan commented May 3, 2023

Alright i now found the issue here.
app.listen should have different port other than 3000 since tcp microservice is using 3000 port. i changed port for tcp service and everthing is working fine. now i have implemented same thing for rabbitMQ, and google pub sub message. Everything is on multi tenant superb ! thanks @moparlakci . i didn't used your entire pr. i just changed TENANT_CONTEXT. and this.getTenant function. everything else used same as this library since i have done some of my own implementation for mongoose

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants