Feat: Multi import support (Backward Compatible)

[falahati]

This PR addresses #42, #44, and partially #20.

The idea with this PR was to change as little as possible of the code yet still allow multiple RPCServices to be available side by side. Following is the list of changes made to make this possible:

• RMQRoute changed to include one or more service names to bind
• ExtendedMessage is edited to have the name of the service available.
• A new RMQInjectService decorator was added to select a specific RMQService instance based on the passed name
• RMQExplorer is now moved to a separate module so that we can initialize it only once and share it between different RMQServices
• A new forFeature() method added to override the default RMQService instance in a module, if more than one service is used by a module or if RMQInjectService is used to explicitly inject a specific RMQService, this function is unnecessary as services are added in global space and can directly be used via the RMQInjectService decorator
• Module is separated into an RMQGlobalModule and an RMQModule, even tho is not necessary for the library to work right now, it is useful for later expansion of the project (globally shared providers should be defined in RMQGlobalModule and local providers in the RMQModule)
• Tests updated to work with the new event names
• A new test for explicit inject and implicit forFeature() import added

Take a look at this and tell me what you think. There are a couple of places in which I have doubt if this is the best approach. In particular, I don't like the idea of having services in the global space and rather have them in per module scope. It would be preferable to have a connection manager defined in the global space with host connection information and then in module space we could have the actual services. This way, we can also share the AmqpConnectionManager. But this is a breaking change so maybe with v3 you can do this. I also don't know why we are adding meta data to the RMQService and not keep them in a variable of the RMQExplorer class instead. It should be faster than using the Reflector.

Also, I don't like the fact that the emitter is used heavily in the library; I mean, it was already complicated for the purpose of the project but now with this additional code it is too complicated for my taste and I think separating it for each module might be a better idea.

I also hate the fact that unless for some reason the user of the library doesn't want to use the RMQInjectService decorator, forFeature is almost useless. I mean we could remove it and features would still be the same. The only use for it is to change the default RMQService for a module if and only if it uses a single RMQService.

I also resisted the urge to change more of the code unrelated to the actual feature, but I think there might be room for improvement with the explorer and especially with cases like generating the regex string. In any case, these can be addressed in a separate issue and PR if necessary. But I had to fix some of the linter errors as false positives were making the development hard.

Please take a look into this and share your thoughts on it and the points raised above. If we can get this into the current version (maybe beta release) it would be great. I have a project blocked by this feature xD

I would be happy to allocate some of my time for version 3 to get it more pretty and clear in a new PR if you are ok with it.

[falahati]

Also, it is not good that we already have a service name, which makes the new name and serviceName fields that I have added here and there a little confusing. But I could not think of a better name for it. Feel free to suggest a new name.

[AlariCode]

@falahati, wow! Great PR. I will look into it later today. I think we can break backward compatibility and include this in version 3 with better code quality as you mentioned.

[AlariCode]

I also resisted the urge to change more of the code unrelated to the actual feature, but I think there might be room for improvement with the explorer and especially with cases like generating the regex string. In any case, these can be addressed in a separate issue and PR if necessary. But I had to fix some of the linter errors as false positives were making the development hard.

Yes!

I also hate the fact that unless for some reason the user of the library doesn't want to use the RMQInjectService decorator, forFeature is almost useless. I mean we could remove it and features would still be the same. The only use for it is to change the default RMQService for a module if and only if it uses a single RMQService.

That's right. I think we need to make forFeature() more useful. For example, you have multiple RMQ instances, and you want to connect to both of them. That's why I suggest we design method following way:

forRoot() must be included in the root of app.module.ts with the following configs:

export interface IRMQServiceBaseOptions {
    defaultConfig?: IRMQConnectionOptions;
    serviceName?: string;
    logMessages?: boolean;
    logger?: LoggerService;
}

export interface IRMQConnectionOptions extends IRMQInstanceOptions, IRMQTransortOptions {
}

export interface IRMQInstanceOptions {
    connections: IRMQConnection[];
    reconnectTimeInSeconds?: number;
    heartbeatIntervalInSeconds?: number;
}

export interface IRMQTransortOptions {
    exchangeName: string;
    queueName?: string;
    queueArguments?: {
        [key: string]: string;
    };
    prefetchCount?: number;
    isGlobalPrefetchCount?: boolean;
    isQueueDurable?: boolean;
    isQueueExclusive?: boolean;
    isExchangeDurable?: boolean;
    assertExchangeType?: Parameters<Channel['assertExchange']>[1];
    exchangeOptions?: Options.AssertExchange;
    messagesTimeout?: number;
    middleware?: typeof RMQPipeClass[];
    intercepters?: typeof RMQIntercepterClass[];
    errorHandler?: typeof RMQErrorHandler;
}

This will allow us to get default connection if no other needed. Backward compatability can be supported if we can work with old and new config at the same time: IRMQServiceBaseOptionsNew | IRMQServiceBaseOptions (naming is just for illustration purpuse).

Then in child module one can define another connection with different options:

export interface IRMQFeatureConnectionOptions {
    config: IRMQConnectionOptions | IRMQTransortOptions;
    name: string | Symbol;
}

If full config is provided - we make another connection to another instance. If not - using default connection, but may assert different exchange or listen to different queue.

Then, you can use provided name to get rmqService instance of a specific connecton (or default if name is not provided):

@RMQInjectService('my-name') rmqService: RMQService

And in RMQRoute we add name as you sugessted:

export interface IRouteOptions {
    name?: string | string[];
    manualAck?: boolean;
    msgFactory?: (msg: IRMQMessage) => any[];
}

If no name provided, route will be binded to default connection. Also global rmqService can be used for backward compatability. Also forTest() method should support both root and feature options.

This way we can support multiple instanses of RMQ, multiple exchanges or queues and logic splitting. What do you think?

[falahati]

The way I have decided to go about this was to follow the TypeORM nestjs module. TypeORM creates the connections via the forRoot as we do now and then uses the forFeature to create the repositories that are connected to global connections defined by forRoot. So connections are shared but repositories are scoped. We don't have anything representing a repository so forFeature is quite useless here.

Regardless of how we end up implementing this, it is good to know what limitations we have beforehand; We need to keep connections as global as it allows us to:

• First, use the same connection manager and hopefully use the same channel for a single exchange, queue.
• And secondly, we don't really have a choice for this since routing is only possible globally so we cant target only controllers in the module that we are in.

So really, no matter how I like to move most of the code to the forFeature, I don't really see how and that's why I asked you about it to see if you have any ideas.

The thing is, to capture routing and handle connections we have to have the channel in global space which contains the queue and the exchange that the queue is connected to. So what do we really have left for the forFeature? forFeature can only contain settings that are used for sending messages, like the exchange name (which is redundant since we already have it in global space for routes) and maybe RPC settings (that we don't have any).

And what really would we gain by separating these? If InjectRMQService is necessary for more than one connection, why should we even force the user to use the forFeature? And really let me go one step farther and argue that since the only use of the InjectRMQService is to select an instance of the service to send, what is preventing us to add an argument to the send and notify methods to simply select the right connection name instead of forcing the user to use this decorator to inject another instance of the service?

This all seems like this library can not really benefit much from forFeature or at least I don't clearly see how.

One idea is to take the class(es) for routing as an argument to the forFeature.

forFeature(
   {
    exchangeName: "exchange",
    queueName: "queue"
   },
   [
      MyServiceRMQMicroservice,
   ]
),

But it does add a lot of redundancy to the user's code if more than one module is going to use it, not to mention that usually, it is better to load configs used for IRMQConnection in global space instead of module space. Not to mention that it never will be backward compatible this way.

Thoughts? Counter arguments?

[falahati]

Not to mention that with neither this implementaion, or the one you proposed, we can offer the ability to join a queue in multiple exchanges. It might be useful to have a queue in multiple exchanges and if we are going to redesign the config here, we better find a way to provide this functionality too.

[falahati]

I have looked into possible ways to get the providers defined in the parent module to allow for scoped configuration and routing for forFeature. ModuleRef and ModulesContainer sound promising but after a little investigation ModuleRef seems useless and I couldn't find a way to identify the current instance of a module in ModulesCointainer let alone creating a hierarchy of modules to figure out the parent feature module.

And even if we do, I am not sure how reliable it would be to tinker with the inner workings of NestJS DI like this.

[falahati]

This is the best approach that I can think of to the configuration of this module:

• RMQModule is used for routing, discovery, and connection management and is exported once per each forRoot allowing the user of the library to connect to more than one RabbitMQ instance. For a single RabbitMQ host, only one forRoot is allowed and should contain information about all exchanges and queues.
  • RMQChannelManager provider: Is a provider in charge of keeping an instance of the AmqpConnectionManager and managing the channels required for the rest of the library.
  • RMQChannel provider: Is used to provide a connection between multiple exchanges, a queue, and multiple topics and again exported globally. Also takes care of discovery for routes and if a route is of interest, bind the topic. One channel is created per queue.
  • RMQService provider: Is created globally for each exchange and takes care of send and notify.
• InjectRMQService(exchangeAllias?: string = DEFAULT_EXCHANGE, connectionAlias?: string = DEFAULT_CONNECTION): decorator for getting an instance of the RMQService for an exchange.
• RMQRoute(topic: string, queueAlias?: string | string[] = DEFAULT_QUEUE, exchangeAlias?: string | string[] = DEFAULT_EXCHANGE, connectionAlias?: string = DEFAULT_CONNECTION): Decorator to assign a topic to a method and implicitly create a binding between a queue and an exchange (or more).

With forRoot accepting a configuration interface like this:

interface IRMQOptionsBase {
   exchanges: IRMQExchange | IRMQExchange[];
   queues: IRMQQueue | IRMQQueue[];
   hosts: IRMQHost | IRMQHost[];
   serviceName: string;
   logMessages?: boolean;
   logger?: LoggerService;
   middleware?: typeof RMQPipeClass[];
   intercepters?: typeof RMQIntercepterClass[];
   errorHandler?: typeof RMQErrorHandler;
}

interface IRMQOptions extends IRMQOptionsBase {
   alias?: string = DEFAULT_CONNECTION;
}

interface IRMQExchange extends Options.AssertExchange {
   alias?: string = DEFAULT_EXCHANGE;
   exchangeName: string;
   messagesTimeout?: number = DEFAULT_TIMEOUT;
   prefetchCount?: number = DEFAULT_PREFETCH_COUNT;
   isGlobalPrefetchCount?: boolean = false;
   intercepters?: typeof RMQIntercepterClass[];
}

interface IRMQQueue extends Options.AssertQueue {
   alias?: string = DEFAULT_QUEUE;
   queueName: string;
   middleware?: typeof RMQPipeClass[];
}

interface IRMQHost {
  login?: string;
  password?: string;
  host?: string;
  port?: number;
  vhost?: string;
  url?: string;
  reconnectTimeInSeconds?: number = DEFAULT_RECONNECT_TIME;
  heartbeatIntervalInSeconds?: number = DEFAULT_HEARTBEAT_TIME;
}

With NO forFeature.

With major refactoring of the explorer and structure of the project, this would be a viable solution for the library and I think is a good candidate for v3. All properties are in the right place and also allows the user to override one or more properties for one or more exchanges or queues and also removed the duplicate configurations, like durability. Not to mention that even tho the config interface changes, but it is far cleaner and also is still easy to migrate as it accepts both an array and an instance for connection, exchange, and queue configs. If a simple usage with a single exchange, queue, and the connection is needed, there is no need to enter additional parameters in RMQRoute and therefore no change to the routes is needed in the migration. For RMQService we can also export the [default connection, exchange, route] RMQService instance that allows sending to all exchanges (with a new optional parameter for exchange alias and connection alias) and to the default exchange if no alias is selected and therefore make migration even easier for users by not requiring them to use the InjectRMQService decorator. In the end, the migration process for users would simply be a couple of small changes to the forRoot argument and that's it. Meanwhile, the library is now far more flexible, allows for multiple connections, multiple exchanges, multiple queues, and multiple exchange-queue binding, and even tho it is still global, the code is scoped enough to have two or more global instances for different RabbitMQ servers allowing the user's code to be in touch with multiple instances of RabbitMQ running in different layers of their network for different porpuses.

Please share your thoughts about this approach for v3. It is not quite backward compatible, but migration is easy enough and isolated to one function.

[AlariCode]

@falahati thanks. I'll think about suggestions and will return with my thoughts. Got hard schedule this week.

[falahati]

@AlariCode I was checking the nestjs issues the other day and stumbled on this: https://docs.nestjs.com/microservices/custom-transport

which allows this library to be fully integrated into the already established nestjs microservice library. since this library is more feature-rich compared to the build-in rabbitmq client/server, rewriting this library to be compatible with nestjs microservices would be dope and also makes it quite easy for people that are migrating from the building rabbitmq driver to use it. consider it.

[AlariCode]

@falahati the only problem is that we can't user module separation as other libs. I will considered using custom transport but this will change everything in lib and will be entirely different lib)

[falahati]

well, it is not perfect, but it should cover most of the added features provided by this library. I have accidentally stumbled upon this when we had to convert our hybrid project to microservice, but since this library is not really microservice compatible had to change it to standalone. works no problem but in the middle of investigations that resulted in this conclusion I had stumbled upon the said article and was thinking about porting this to act as a custom transport, or maybe write my own library based on the code and experience of this project but as a transport.

module separation is something that we already talked about here and I don't see a way to do it with the current lib either; so that part is not a problem. MessagePattern also contains metadata and transport values, now I am not very familiar with these values and how can we extract them as part of the eventHandlers property, but it seems promising. And even if they don't fit our specific needs, we can always add additional decorators, nestjs seems to do just that for grpc transport anyway.

on the other hand, the fact that it forces the library to define clients and server separately saves us a decision on how to handle clients' registration. there is also build in naming support for clients, although we have to manually add multi-server support I suppose.

in any case, just my two cents on this; figured that this might be of use in your decision.

[maraero]

Hey @AlariCode! Is there anything that can be done to speed up the implementation of this feature?

[AlariCode]

@maraero, unfortunately, I do not have time for further big features implementation due to other priorities.

[SergioArrighi]

Hello, was this feature ever merged? Thanks and regards