AMQP/RabbitMQ notifications and general AMQP integration

Hi,

I have a need for the notifications over AMQP thus have been working on some coding and a couple of questions popped up, specifically on notifications side (before I throw in PR), so I can understand clearly the logic behind some decisions. I'm more than happy to move all this discussion into a PR debate as well, if this is better approach, though here it remains more "discoverable" in the later stage it seems...

General integration
Extracting and making the client reusable for notifications and event/position forwarding is very straightforward and reduces significantly the duplications.

• Is there a strong argumentation against having MQ connections centralized through a ConnectionManager or similar approach with connection URL as a key? AMQP best practice is that there is single connection many channels, so if the connection URL matches then only the channel (exchange/topic) would be really different thus connections can be stored and reused.
• Right now I stored the extracted and common AmqpConnectionManager and AmqpClient in the helper package, but as there is mail and SMS packages, would it make sense to have moved it to amqp package perhaps?
• I would assume that using injection during creating the PositionForwarder and EventForwarder in MainModule is fine (I personally prefer the constructors not to overflow with parameter lists)?

AMQP Client SSL management

• SSL for AMQP clients is straightforward, but requires JKS keystore/certificate management. I would assume that having this as an option through making it configurable (default still works with the trust all SSL manager)?
• Parameter naming - FORWARDER_AMQP_SSL_KEYSTORE vs FORWARDER_AMQP_SSLKEYSTORE (and similarly notificator.amqp.ssl.keystore vs. notificator.amqp.sslkeystore)? Personal preference being the first one, but I'm fine with whatever the best practice is.
• For notifications and user configuration, I'm wondering if for SSL it would be reasonable to store the data in BASE64 format in user attributes, pretty wild idea, but don't seem to have much alternatives to this for storing user data (are there any considerations/limitations on storing the user attributes)?

User attributes

• for notifications (not pos/event forwarding) the AMQP configuration to the level of topic/exchange on user side would be ok (similar to Telegram chat id)?
• As it is possible to also have multiple connections to different Rabbit servers, I would assume that having user based connection URLs would also be ok?
• Localisation of these parameters in web - I would assume that having the parameters put into l10n files in English and then they get localised by whoever needs them, is reasonable approach?

I don't think I would be able to discuss all of these questions at the same time. We can discuss one by one, probably starting with higher priority ones.

Fair, let's start with the part of

• are there any fundamental issues with extracting the AMQP connection management out from the Event/Position forwarding implementations to a dedicated connection manager that can be injected/passed in constructor to notification/event/position handlers;
• These handlers can then in turn acquire persistent connections from the manager from which they generate the channels (or actual sending constructs) which are short-lived;

This would follow the AMQP logic of long term connections which contain many channels and also reduce code duplication (where every class that needs AMQP has implemented the same connection/channel establishment)?

• How would you handle the case where you want to send positions and events to different places?
• Why do you think having two connections instead of one is a big problem?

• The connection URL would be different for connections, which is the key for storing the connections (e.g. amqps://positions:positions@rabbit and amqps://events:events@rabbit), thus there would be two different connections with different usernames/passwords or hostnames be created.
  E.g. Hashtable<String, Connection> activeConnections where one would get the connection out with activeConnections.get("amqps://positions:positions@rabbit")

• It's not a big problem. Just a best practice. Creating connections does consume some resources on both sides, but it's not a massive overhead. SSL a bit more, but again I won't put this into table as a major argument, just added benefit.

• What is real added benefit is having a single place where connection and client are implemented. Right now if I'd implement notifications forwarding based on the same logic, I'd create a third connection with it's own connection management etc. whereas it can be actually abstracted out to a single place. The client would be right now logically in a wrong place as well, I'd rather reuse already implemented client, which is sitting right now in the forwarders, instead of duplicating the code.

It sounds like your main motivation is some additional customization you want to do. Is that correct?

Yes, that is correct. We need to get the notification data out of the system using RabbitMQ and that functionality is missing right now. So when I implemented the code, there was duplication on client construction and lack of SSL client certs. So when implementing these parts it became clear that the whole thing can be unified and handled in single place. Question is - if and how this should merged back up, should there be any wish for that - which also drives the questions around namings and conventions.

If you want to change the way this works, we would probably need to change all other forwarders. Are you interested in taking on that work?

Not sure it has that significant change (I was very keen on ensuring that the amount of changes to existing behaviour are as minimal as possible). Currently it looks something like https://github.com/traccar/traccar/compare/master...laazik:traccar:rabbitmq-ssl-client

Main module key change is pushing in injector to the event/position forwarder so that relevant classes can be pulled in using DI (as event/position are constructed not injected). In relevant event/position forwarder main change would be fetching the connection from manager instead of creating it and avoiding exchange declaration, if needed (different type of already declared exchange will resullt in RuntimeException).

I'd personally like to add some tests there, though works in my production (not sure if better than works on my laptop).
Also please ignore the azure pipelines buildfile (would remove this) and gradle checkstyle override (used it in local dev for crlf warnings).

Though if there is an idea of how to make some things better in general for forwarders can take a look into for sure.

I don't think it's that big of a change, but I want all forwarders to have consistent implementation. I don't want to change just one to be its own thing.