Create a separate Guice module for OpenPaaS communication #1261

HoussemNasri · 2024-10-24T15:56:06Z

TODO

Test OpenPaasConfiguration (see RabbitMQConfigurationTest)
Test AmqpURI
~~<Make the "add contacts" exchange configurable.~~ OpenPaasContactsCollection: Make the "contacts:contact:add" exchange configurable #1280
Sort pom dependecies
Throw exception when RabbitMQ URI not configured in openpaas.proeprties.

quantranhong1999 · 2024-10-25T03:42:55Z

...l-backend/apps/distributed/src/main/java/com/linagora/tmail/james/app/DistributedServer.java

@@ -312,7 +313,8 @@ protected void configure() {
            new TeamMailboxModule(),
            new TMailMailboxSortOrderProviderModule(),
            new TmailEventModule(),
-            new TmailEventDeadLettersModule());
+            new TmailEventDeadLettersModule(),
+            new OpenPaasModule());


22:23:34.258 [ERROR] c.l.t.OpenPaasModule - Could not find configuration file 'openpaas.properties' 22:23:34.271 [ERROR] o.a.j.GuiceJamesServer - Fatal error while starting James java.io.FileNotFoundException: openpaas

Likely we should make the OpenPaaS module optional?

Fully agree.

If unspecified we shall fallback to the main rabbitMQ connection and rely on / vhost.

...ckend/tmail-third-party/openpaas/src/main/java/com/linagora/tmail/OpenPaasConfiguration.java

chibenwa · 2024-10-25T07:06:00Z

tmail-backend/tmail-third-party/openpaas/src/main/java/com/linagora/tmail/OpenPaasModule.java

+            LOGGER.error("Could not find configuration file '{}.properties'",
+                OPENPAAS_CONFIGURATION_NAME);
+            throw new RuntimeException(e);


We shall likely do a module chooser to start or not the openpaas extension?

Could you confirm if this is accurate: The extension loads only if openpaas.properties is present in the classpath. If any OpenPaas configurations (e.g., apiURL or admin details) are missing, it will fail. If the RabbitMQ URI is not configured, it falls back to rabbitmq.properties (question: for the fallback, should we use all configurations in rabbitmq.properties or just the URI?)

If any OpenPaas configurations (e.g., apiURL or admin details) are missing, it will fail.

Wrong.

For Distributed if AMQP settings are missing we are falling back to the mail RabbitMQChannelPool

For Memory we are failing

(This likely means we need a little DistributedOpenPaaSAMQPFallbackModule and MemoryOpenPaaSAMQPFallbackModule to encode this difference)

For the fallback we use the RabbitMQChannelPool object already instanciated, let's not read rabbitmq.properties ourselves.

chibenwa · 2024-10-25T07:07:58Z

tmail-backend/tmail-third-party/openpaas/src/main/java/com/linagora/tmail/OpenPaasModule.java

+    @Provides
+    @Named(OPENPAAS_INJECTION_KEY)
+    @Singleton
+    public RabbitMQConfiguration provideRabbitMQConfiguration(OpenPaasConfiguration openPaasConfiguration, RabbitMQConfiguration fallbackRabbitMQConfiguration) {


Here we would connect twice to RabbitMQ.

Is there a way to connect only once when we fallback?

Also on top of memory-app there would be no fallback as we do not use RabbitMQ at all...

Here we would connect twice to RabbitMQ.

Can you please elaborate on this part?

- Inject openpass-specific RabbitMQ configuration to OpenPaasContactsConsumer

…configuration settings. - If the OpenPaas module is loaded of course.

tmail-backend/tmail-third-party/openpaas/src/main/java/com/linagora/tmail/AmqpUri.java

...ird-party/openpaas/src/main/java/com/linagora/tmail/configuration/OpenPaasConfiguration.java

chibenwa · 2024-10-28T14:12:14Z

...ibuted/src/main/java/com/linagora/tmail/james/app/DistributedOpenPaasAmqpFallbackModule.java

+    @Provides
+    @Named(OPENPAAS_INJECTION_KEY)
+    @Singleton
+    public RabbitMQConfiguration provideRabbitMQConfiguration(


Is it possible to write something like:

public RabbitMQChannelPool provideRabbitMQOpenPaaSFallback(OpenPaasConfiguration openPaasConfiguration, RabbitMQChannelPool channelPool, @Named("openpaas") Provider<RabbitMQCHannelPool> openPaaSChannelPool) { return openPaasConfiguration.maybeRabbitMqUri() .map(any -> openPaaSChannelPool.get()) .orElse(channelPool);

I don't think we can. The provider takes another provider annotated with @nAmed("openpaas"), if we annotate this provider with the same annotation, which is the attended use case we will get a BINDING_ALREADY_SET error. We could skip the annotation but that will still conflict with the default RabbitMQChannelPool provider declared elsewhere.

The only solution I see is to define a new injection key, i.e., @nAmed("openpaas_with_fallback"), provided by DistributedOpenPaasAmqpFallbackModule.

I don't think we can.

Create more intermediary abstractions to break the dependency loops. Or use other named annotations.

Please find something. Be inventive and try things.

It was decided not to implement the fallback in this PR, keeping it open so I can return to it when working on the fallback.

...ibuted/src/main/java/com/linagora/tmail/james/app/DistributedOpenPaasAmqpFallbackModule.java

...end/apps/memory/src/test/java/com/linagora/tmail/james/app/MemoryServerWithOpenPaasTest.java

chibenwa · 2024-10-29T13:51:36Z

.../apps/memory/src/test/java/com/linagora/tmail/james/app/MemoryServerWithoutOpenPaasTest.java

+import com.linagora.tmail.encrypted.MailboxManagerClassProbe;
+import com.linagora.tmail.module.LinagoraTestJMAPServerModule;
+
+public class MemoryServerWithoutOpenPaasTest {


I think all the other tests check what happens without the openpaas module enabled.

As it stands this test is useless.

We shall rather check the exception we get when we try to start tmail with the openpaas extension without configuring RabbitMQ...

...ributed/src/test/java/com/linagora/tmail/james/app/DistributedServerWithoutOpenPaasTest.java

chibenwa · 2024-10-29T14:32:44Z

To test that RabbitMQ and OpenPaaS rabbitMQ are different we could leverage a "Probe"

We can create a TestProbe with injections on RabbitMQChannelPool and @Named("openpaas") RabbitMQChannelPool that exposes a boolean method to show if those are the sames or not.

chibenwa · 2024-10-29T19:45:52Z

...end/apps/memory/src/test/java/com/linagora/tmail/james/app/MemoryServerWithOpenPaasTest.java

+
+    @BeforeAll
+    static void setUp() {
+        System.setSecurityManager(new NoExitSecurityManager());
+    }


@afterall remember to unset the security manager or test isolation is broken.

chibenwa · 2024-10-29T19:47:12Z

...end/apps/memory/src/test/java/com/linagora/tmail/james/app/MemoryServerWithOpenPaasTest.java

+    void serverShouldThrowWithOpenPaasEnabled() throws Exception {
+        try {
+            jamesServerExtension.start(null);
+            fail("James server didn't exit or fail!");
+        } catch (Exception e) {
+            LOGGER.error("Expected failure", e);
+        }
+    }


disable auto start on the extention, get a non-started guice-james-server as a parameter and start it manually in the tests

tmail-backend/apps/distributed/pom.xml

...d/tmail-third-party/openpaas/src/test/java/com/linagora/tmail/OpenPaasRabbitMQExtension.java

tmail-backend/tmail-third-party/openpaas/src/test/java/com/linagora/tmail/AmqpUriTest.java

tmail-backend/tmail-third-party/openpaas/src/main/java/com/linagora/tmail/AmqpUri.java

tmail-backend/tmail-third-party/openpaas/src/test/java/com/linagora/tmail/AmqpUriTest.java

tmail-backend/tmail-third-party/openpaas/src/main/java/com/linagora/tmail/AmqpUri.java

…rties - It was decided to handle the fallback to the existing RabbitMQ connection later.

tmail-backend/apps/distributed/pom.xml

chibenwa · 2024-11-04T03:27:15Z

...t/java/com/linagora/tmail/james/app/DistributedServerWithOpenPaasRabbitMqConfiguredTest.java

+            .build())
+        .server(configuration -> DistributedServer.createServer(configuration)
+            .overrideWith(new LinagoraTestJMAPServerModule())
+            .overrideWith(binder -> Multibinder.newSetBinder(binder, GuiceProbe.class).addBinding().to(UsersRepositoryClassProbe.class)))


Is this module override needed?

Probably not, we don't use jmap in this test. The test passes even after removing the module. It is hard to tell for sure which modules or extensions are needed and which are not.

I removed it.

tmail-backend/tmail-third-party/openpaas/src/main/java/com/linagora/tmail/OpenPaasModule.java

quantranhong1999 reviewed Oct 25, 2024

View reviewed changes

chibenwa reviewed Oct 25, 2024

View reviewed changes

...ckend/tmail-third-party/openpaas/src/main/java/com/linagora/tmail/OpenPaasConfiguration.java Outdated Show resolved Hide resolved

chibenwa reviewed Oct 25, 2024

View reviewed changes

...ckend/tmail-third-party/openpaas/src/main/java/com/linagora/tmail/OpenPaasConfiguration.java Outdated Show resolved Hide resolved

chibenwa reviewed Oct 25, 2024

View reviewed changes

...ckend/tmail-third-party/openpaas/src/main/java/com/linagora/tmail/OpenPaasConfiguration.java Outdated Show resolved Hide resolved

chibenwa reviewed Oct 25, 2024

View reviewed changes

HoussemNasri added 3 commits October 25, 2024 11:34

Create OpenPaasModule

fbea4a5

- Inject openpass-specific RabbitMQ configuration to OpenPaasContactsConsumer

Fix typo

0a093a6

Fix guice dependency resolution

eef7eef

HoussemNasri force-pushed the openpaas-module branch from 8464469 to eef7eef Compare October 25, 2024 10:36

HoussemNasri added 7 commits October 25, 2024 13:22

Selectively load the openpaas module in the DistributedServer

44cd3b9

Selectively load the openpaas module in the MemoryServer

88dba77

The OpenPaas admin username, password, and API URL are now mandatory …

4006390

…configuration settings. - If the OpenPaas module is loaded of course.

Propagate exception

e59884f

Implement equals/hashcode for AmqpUri

1dda9ee

Test OpenPaasConfiguration

a5bf07b

Refactor

71364c4

HoussemNasri self-assigned this Oct 28, 2024

chibenwa reviewed Oct 28, 2024

View reviewed changes

tmail-backend/tmail-third-party/openpaas/src/main/java/com/linagora/tmail/AmqpUri.java Outdated Show resolved Hide resolved

chibenwa reviewed Oct 28, 2024

View reviewed changes

tmail-backend/tmail-third-party/openpaas/src/main/java/com/linagora/tmail/AmqpUri.java Show resolved Hide resolved

chibenwa reviewed Oct 28, 2024

View reviewed changes

...ird-party/openpaas/src/main/java/com/linagora/tmail/configuration/OpenPaasConfiguration.java Outdated Show resolved Hide resolved

chibenwa reviewed Oct 28, 2024

View reviewed changes

...ibuted/src/main/java/com/linagora/tmail/james/app/DistributedOpenPaasAmqpFallbackModule.java Outdated Show resolved Hide resolved

chibenwa reviewed Oct 29, 2024

View reviewed changes

...end/apps/memory/src/test/java/com/linagora/tmail/james/app/MemoryServerWithOpenPaasTest.java Outdated Show resolved Hide resolved

chibenwa reviewed Oct 29, 2024

View reviewed changes

...end/apps/memory/src/test/java/com/linagora/tmail/james/app/MemoryServerWithOpenPaasTest.java Outdated Show resolved Hide resolved

chibenwa reviewed Oct 29, 2024

View reviewed changes

...ributed/src/test/java/com/linagora/tmail/james/app/DistributedServerWithoutOpenPaasTest.java Outdated Show resolved Hide resolved

chibenwa reviewed Oct 29, 2024

View reviewed changes

Add integration tests

7e6daf8

HoussemNasri force-pushed the openpaas-module branch from c20c64d to 7e6daf8 Compare October 30, 2024 13:34

Minor cleanup

2e1e303

chibenwa reviewed Oct 30, 2024

View reviewed changes

tmail-backend/apps/distributed/pom.xml Outdated Show resolved Hide resolved

chibenwa reviewed Oct 30, 2024

View reviewed changes

...d/tmail-third-party/openpaas/src/test/java/com/linagora/tmail/OpenPaasRabbitMQExtension.java Outdated Show resolved Hide resolved

HoussemNasri added 2 commits October 30, 2024 17:08

Refactor AmqpUri

7e3db77

Test AmqpUri

4c1cc72