[WIP] [ISSUE-1163] Inject OpenPaaS user contacts into Tmail's auto-complete database #1215

HoussemNasri · 2024-10-03T12:33:25Z

chibenwa · 2024-10-03T12:41:58Z

.../src/main/scala/com/linagora/tmail/james/jmap/contact/openpaas/OpenPaasContactsConsumer.java

+    // TODO: Create a separate RabbitMQ module for OpenPaaS communication so the injected channel pool
+    //  would be custom configured


I support this! A Lot!

We could...

Have a file named openpaas.properties which allow overriding RABBITMQ connection settings, if absent we can fallback to the default ChannelPool. It would be annotated with @nAmed("openpaas")

Then we can use it for OpenPaaS related communications in particular have a OpenPaaSAMQPForwardAttribute that extends `AmqpForwardAttribute and reuses this.

Actually such a task might be a good prelimiary worrk to this PR!

chibenwa · 2024-10-03T12:46:23Z

.../src/main/scala/com/linagora/tmail/james/jmap/contact/openpaas/OpenPaasContactsConsumer.java

+            Boolean.TRUE.equals(channelPool.getChannelMono()
+                .map(channel -> {
+                    try {
+                        channel.queueDeclarePassive(QUEUE_NAME);
+                        return true;
+                    }
+                    catch (IOException e) {
+                        return false;
+                    }
+                }).block());


That's a VERY good finding, I wasn't aware of this methods.

So if the queue already exist but with slightly different arguments does it throws? Can you please quickly test that? If no, then we have a solution to our major RabbitMQ migration nightmares Cc @QuangHoang1210 @Arsnael @vttranlina and we could use it more widely in the source code...

.../src/main/scala/com/linagora/tmail/james/jmap/contact/openpaas/OpenPaasContactsConsumer.java

chibenwa · 2024-10-03T12:51:48Z

.../src/main/scala/com/linagora/tmail/james/jmap/contact/openpaas/OpenPaasContactsConsumer.java

+        }
+
+        consumeContactsDisposable = channelPool.createReceiver()
+            .consumeAutoAck(QUEUE_NAME)


Consume autoACK is a bad idea that pretty much guaranty message loss

no nack on error means after 1 day consumer shutdown. Oups.

Please use manual ACKs.

.../src/main/scala/com/linagora/tmail/james/jmap/contact/openpaas/OpenPaasContactsConsumer.java

quantranhong1999

Read it

...nsions/src/test/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactsConsumerTest.java

chibenwa · 2024-10-04T12:45:21Z

...extensions/src/main/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactsConsumer.java

+                sender.declareExchange(ExchangeSpecification.exchange(TOPIC)
+                    .durable(DURABLE)),


declare the exchange as fanout (because that's the settings openpaas uses.

Where can I find the RabbitMQ config OpenPaaS uses?

Because I can check it in real world RabbitMQ admin ;-)

chibenwa · 2024-10-04T12:45:51Z

...extensions/src/main/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactsConsumer.java

+                    .durable(DURABLE)),
+                sender.declareQueue(QueueSpecification
+                    .queue(QUEUE_NAME)
+                    .durable(evaluateDurable(DURABLE, commonRabbitMQConfiguration.isQuorumQueuesUsed()))),


Queue MUST always be durable.

And ideally queue settings shall be obtained from commonRabbitMQConfiguration

...extensions/src/main/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactsConsumer.java

...p/extensions/src/main/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactMessage.java

tmail-backend/jmap/extensions/pom.xml

.../extensions/src/main/java/com/linagora/tmail/james/jmap/contact/jCardObjectDeserializer.java

...backend/jmap/extensions/src/main/java/com/linagora/tmail/james/jmap/contact/jCardObject.java

...extensions/src/main/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactsConsumer.java

chibenwa · 2024-10-07T12:27:55Z

...extensions/src/main/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactsConsumer.java

        Mono.just(messagePayload)
-            .map(message -> gson.fromJson(message, OpenPaasContactMessage.class))
+            .<ContactAddedRabbitMqMessage>handle((message, sink) -> {


We likely do not need that "just" and can inline the reactor transformations.

...extensions/src/main/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactsConsumer.java

...nsions/src/test/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactsConsumerTest.java

chibenwa · 2024-10-07T12:34:10Z

...extensions/src/main/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactsConsumer.java

+            MailAddress mailAddress = new MailAddress(jCardObject.email());
+            Username emailAsUsername = Username.fromMailAddress(mailAddress);
+
+            Mono.from(contactSearchEngine.index(AccountId.fromUsername(emailAsUsername),


Bob creates a contact named "alice" and we add here alice contact into alice address book.

What would be needed here is to resolve the email address of bob (to compute his AccountId) and then add alice contact in it.

For this we need the call to openpaas user API

We can hide such an operation behind an interface to isolate this from the rest of the code here.

chibenwa · 2024-10-08T10:00:14Z

...d/jmap/extensions/src/main/java/com/linagora/tmail/james/jmap/contact/OpenPaasWebClient.java

+        String userPassword = openPaasConfiguration.getWebClientUser() + ":" + openPaasConfiguration.getWebClientPassword();
+        byte[] base64UserPassword = Base64.getEncoder().encode(userPassword.getBytes(StandardCharsets.UTF_8));


We can store this as a field to avoid to recompute it on every request

chibenwa

Cool but IMO we would deserve to have dedicated tests for OpenPaaSWebClient class.

(Test in isolation for this)

...d/jmap/extensions/src/main/java/com/linagora/tmail/james/jmap/contact/OpenPaasWebClient.java

...extensions/src/main/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactsConsumer.java

chibenwa · 2024-10-17T13:01:14Z

tmail-backend/jmap/extensions/pom.xml

@@ -24,6 +24,16 @@
            <groupId>${project.groupId}</groupId>
            <artifactId>team-mailboxes</artifactId>
        </dependency>
+        <dependency>


Why are there modifs in jmap extensions pom file? Accident?

chibenwa · 2024-10-17T13:18:15Z

tmail-backend/tmail-third-party/openpaas/pom.xml

+    <dependencies>
+        <dependency>
+            <groupId>${project.groupId}</groupId>
+            <artifactId>jmap-extensions</artifactId>


Ideally we should refactor the maven module architecture to extract the "contact data model" (and its implementations?) into a separate maven module.

(ticket?)

chibenwa · 2024-10-17T13:20:34Z

...nd/tmail-third-party/openpaas/src/main/java/com/linagora/tmail/api/OpenPaasUserResponse.java

+public record OpenPaasUserResponse(@JsonProperty("id") String id,
+                                   @JsonProperty("firstname") String firstname,
+                                   @JsonProperty("lastname") String lastname,
+                                   @JsonProperty("preferredEmail") String preferredEmail,
+                                   @JsonProperty("emails") List<String> emails,
+                                   @JsonProperty("main_phone") String mainPhone,
+                                   @JsonProperty("displayName") String displayName) {


Strip uneeded fields?

chibenwa · 2024-10-17T13:26:07Z

...l-third-party/openpaas/src/main/java/com/linagora/tmail/contact/JCardObjectDeserializer.java

+        if (!jCardProperties.containsKey(FN)) {
+            throw new RuntimeException("The FN field is required according to specification.");
+        }


I believe we should be more lenient (be lenient on what you accept strict on what you send)

Can we make FN optional in our datmodel? Or position it to an empty string?

chibenwa · 2024-10-17T13:27:30Z

...l-third-party/openpaas/src/main/java/com/linagora/tmail/contact/JCardObjectDeserializer.java

+            .collect(Collectors.toMap(Pair::getKey, Pair::getValue));
+    }
+
+    private static ImmutablePair<String, String> getPropertyKeyValuePair(JsonNode propertyNode) {


Suggested change

private static ImmutablePair<String, String> getPropertyKeyValuePair(JsonNode propertyNode) {

private static ImmutablePair<String, String> getPropertyKeyValuePair(JsonNode propertyNode) {

Suggested change

private static ImmutablePair<String, String> getPropertyKeyValuePair(JsonNode propertyNode) {

private static Optional<ImmutablePair<String, String>> getPropertyKeyValuePair(JsonNode propertyNode) {

Or Stream<...> so that we can then use flatMap

chibenwa · 2024-10-17T13:31:22Z

...-third-party/openpaas/src/main/java/com/linagora/tmail/contact/OpenPaasContactsConsumer.java

+
+    public static final String EXCHANGE_NAME = "contacts:contact:add";
+    public static final String QUEUE_NAME = "ConsumeOpenPaasContactsQueue";
+    private Disposable consumeContactsDisposable;


Move it after final fields.

chibenwa · 2024-10-17T13:31:32Z

...-third-party/openpaas/src/main/java/com/linagora/tmail/contact/OpenPaasContactsConsumer.java

+    // TODO: Create a separate RabbitMQ module for OpenPaaS communication so the injected channel pool
+    //  would be custom configured


Suggested change

// TODO: Create a separate RabbitMQ module for OpenPaaS communication so the injected channel pool

// would be custom configured

chibenwa · 2024-10-17T13:32:09Z

...-third-party/openpaas/src/main/java/com/linagora/tmail/contact/OpenPaasContactsConsumer.java

+    public OpenPaasContactsConsumer(@Named(EmailAddressContactInjectKeys.AUTOCOMPLETE) ReceiverProvider receiverProvider,
+                                    @Named(EmailAddressContactInjectKeys.AUTOCOMPLETE) Sender sender,


Inject a RabbitMQCHannelPool

chibenwa · 2024-10-17T13:35:02Z

...-third-party/openpaas/src/main/java/com/linagora/tmail/contact/OpenPaasContactsConsumer.java

+                sender.declareQueue(QueueSpecification
+                    .queue(QUEUE_NAME)
+                    .durable(DURABLE)),


.arguments(configuration.workQueueArgumentsBuilder()
.deadLetter("xxx-dead-letter")
.build()));

We likly need also to create the deadletter queue

chibenwa · 2024-10-17T13:37:07Z

...-third-party/openpaas/src/main/java/com/linagora/tmail/contact/OpenPaasContactsConsumer.java

+            Receiver::close);
+    }
+
+    private void messageConsume(AcknowledgableDelivery ackDelivery, String messagePayload) {


chibenwa · 2024-10-17T13:37:36Z

...-third-party/openpaas/src/main/java/com/linagora/tmail/contact/OpenPaasContactsConsumer.java

+                    .flatMap(contactEmail -> {
+                        try {
+                            return Optional.of(new MailAddress(contactEmail));
+                        } catch (AddressException e) {


Logger.warn please

chibenwa · 2024-10-17T13:39:35Z

...-third-party/openpaas/src/main/java/com/linagora/tmail/contact/OpenPaasContactsConsumer.java

+                        }
+                    });
+
+                if (contactMailAddressOpt.isEmpty()) {


Let's extract the ContactAddedRabbitMqMessage => ContactFields

chibenwa · 2024-10-17T13:39:50Z

...-third-party/openpaas/src/main/java/com/linagora/tmail/contact/OpenPaasContactsConsumer.java

+
+                return Mono.from(contactSearchEngine.index(ownerAccountId,
+                        new ContactFields(contactMailAddressOpt.get(), contactFullname, "")
+                    )).block();


.block in production code is a red flag

chibenwa · 2024-10-17T13:40:45Z

...rd-party/openpaas/src/test/java/com/linagora/tmail/contact/OpenPaasContactsConsumerTest.java

+            assertThat(
+                Flux.from(searchEngine.autoComplete(AccountId.fromString(OpenPaasServerExtension.ALICE_EMAIL()), "jhon", 10))
+                    .collectList().block())
+                .hasSize(1));


We could be more precise on the assert: what is the result given by the search engine?

chibenwa · 2024-10-17T13:41:01Z

...rd-party/openpaas/src/test/java/com/linagora/tmail/contact/OpenPaasContactsConsumerTest.java

+import com.linagora.tmail.james.jmap.contact.EmailAddressContactSearchEngine;
+import com.linagora.tmail.james.jmap.contact.InMemoryEmailAddressContactSearchEngine;
+
+class OpenPaasContactsConsumerTest {


Test edge cases.

- Create OpenPaasContactsConsumer to handle contact addition messages from RabbitMQ - Add OpenPaasContactMessage class to represent incoming contact data - Implement message consumption and indexing logic - Add unit tests for OpenPaasContactsConsumer

- For now, it only consumes the /api/users/{openpaas_user_id} endpoint

- Done some good stuff in Reactive programming