-
Notifications
You must be signed in to change notification settings - Fork 22
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
Create a separate Guice module for OpenPaaS communication #1261
base: master
Are you sure you want to change the base?
Conversation
@@ -312,7 +313,8 @@ protected void configure() { | |||
new TeamMailboxModule(), | |||
new TMailMailboxSortOrderProviderModule(), | |||
new TmailEventModule(), | |||
new TmailEventDeadLettersModule()); | |||
new TmailEventDeadLettersModule(), | |||
new OpenPaasModule()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
...ckend/tmail-third-party/openpaas/src/main/java/com/linagora/tmail/OpenPaasConfiguration.java
Outdated
Show resolved
Hide resolved
...ckend/tmail-third-party/openpaas/src/main/java/com/linagora/tmail/OpenPaasConfiguration.java
Outdated
Show resolved
Hide resolved
LOGGER.error("Could not find configuration file '{}.properties'", | ||
OPENPAAS_CONFIGURATION_NAME); | ||
throw new RuntimeException(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shall likely do a module chooser to start or not the openpaas extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@Provides | ||
@Named(OPENPAAS_INJECTION_KEY) | ||
@Singleton | ||
public RabbitMQConfiguration provideRabbitMQConfiguration(OpenPaasConfiguration openPaasConfiguration, RabbitMQConfiguration fallbackRabbitMQConfiguration) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we would connect twice to RabbitMQ.
Can you please elaborate on this part?
- Inject openpass-specific RabbitMQ configuration to OpenPaasContactsConsumer
8464469
to
eef7eef
Compare
…configuration settings. - If the OpenPaas module is loaded of course.
if (this == o) { | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can likely get rid of this micro optimization
@@ -86,4 +86,22 @@ private RabbitMQConfiguration.ManagementCredentials parseUserInfo(String userInf | |||
.join(passwordParts) | |||
.toCharArray()); | |||
} | |||
|
|||
@Override | |||
public boolean equals(Object o) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall be final
.
(Is this tested by equals-verifier?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not yet. I'm currently testing AmqpURI but haven't pushed the changes yet because I'm encountering some issues with the URI class in the Java standard. For instance, BAD_URI is considered a valid URI and does not throw an exception.
String adminPassword | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String adminPassword | |
) { | |
String adminPassword) { |
(Please do not do it on purpose!)
@Provides | ||
@Named(OPENPAAS_INJECTION_KEY) | ||
@Singleton | ||
public RabbitMQConfiguration provideRabbitMQConfiguration( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
import com.linagora.tmail.AmqpUri; | ||
import com.linagora.tmail.configuration.OpenPaasConfiguration; | ||
|
||
public class DistributedOpenPaasAmqpFallbackModule extends AbstractModule { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need tests for both the regular behaviour (when rabbitmq uri is configured) and the fallback, for both distributed and mamory App.
In order to do that:
- we will not create the
openpaas.properties
file - edit the configuration of the JamesServerExtension we are using in order to turn on the OpenPaaS extension
- we will then need a (server) overide where we define the OpenPaaS extension configuration. It ;s ok as we already have solid tests for configuration parsing.
So 4 little tests to write!
"admin", | ||
"admin" | ||
) | ||
))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"admin",
"admin"))))
Those categories of comments are not worth my time. Please pay attention!
@BeforeAll | ||
static void setUp() { | ||
System.setSecurityManager(new NoExitSecurityManager()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WHY?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On failure, the server does not throw an exception it exits by calling System.exit
. Here we are trying to test that the memory server fails when the OpenPaas module is enabled and there is no rabbitmq channel pool to fallback to.
The SecurityManager trick is something I have found online being used to handle this same use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is is worth a comment?
import com.linagora.tmail.encrypted.MailboxManagerClassProbe; | ||
import com.linagora.tmail.module.LinagoraTestJMAPServerModule; | ||
|
||
public class MemoryServerWithoutOpenPaasTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
import com.linagora.tmail.encrypted.MailboxManagerClassProbe; | ||
import com.linagora.tmail.module.LinagoraTestJMAPServerModule; | ||
|
||
public class DistributedServerWithoutOpenPaasTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idem this test is useless.
To test that RabbitMQ and OpenPaaS rabbitMQ are different we could leverage a "Probe" We can create a TestProbe with injections on |
|
||
@BeforeAll | ||
static void setUp() { | ||
System.setSecurityManager(new NoExitSecurityManager()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@afterall remember to unset the security manager or test isolation is broken.
void serverShouldThrowWithOpenPaasEnabled() throws Exception { | ||
try { | ||
jamesServerExtension.start(null); | ||
fail("James server didn't exit or fail!"); | ||
} catch (Exception e) { | ||
LOGGER.error("Expected failure", e); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disable auto start on the extention, get a non-started guice-james-server as a parameter and start it manually in the tests
c20c64d
to
7e6daf8
Compare
@@ -98,6 +98,14 @@ | |||
<artifactId>tmail-openpaas</artifactId> | |||
<version>1.0.0-SNAPSHOT</version> | |||
</dependency> | |||
<dependency> | |||
<groupId>com.linagora.tmail</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${project.groupId}
import com.google.inject.Module; | ||
|
||
public class OpenPaasRabbitMQExtension implements GuiceModuleTestExtension { | ||
private final DockerRabbitMQ dockerRabbitMQ = DockerRabbitMQ.withoutCookie(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this extension is not needed we could have 2 connections to the main RabbitMQ extension and we would still be able to assert if we want that the mail RabbitMQChannelPool is distinct from the one annotated with @Named("openpaas")
Arguments.of("amqp://user@host") | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguments.of("amqp://user@host") | |
); | |
Arguments.of("amqp://user@host")); |
Arguments.of("BAD_URI") // Just bad | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguments.of("BAD_URI") // Just bad | |
); | |
Arguments.of("BAD_URI")); // Just bad |
if (!(o instanceof AmqpUri amqpUri)) { | ||
return false; | ||
} | ||
return Objects.equals(uri, amqpUri.uri) && | ||
Objects.equals(managementCredentials, amqpUri.managementCredentials) && | ||
Objects.equals(userInfo, amqpUri.userInfo) && | ||
Objects.equals(vhost, amqpUri.vhost); | ||
} | ||
|
||
@Override | ||
public int hashCode() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public int hashCode() { | |
public final int hashCode() { |
} | ||
|
||
@Test | ||
void shouldRespectBeanContract() throws URISyntaxException, NoSuchAlgorithmException, KeyManagementException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void shouldRespectBeanContract() throws URISyntaxException, NoSuchAlgorithmException, KeyManagementException { | |
void shouldRespectBeanContract() throws Exception { |
|
||
@Override | ||
public String toString() { | ||
return "AmqpUri{" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MoreObjects.toStringHelpers()...
Closes #1223
TODO
OpenPaasConfiguration
(seeRabbitMQConfigurationTest
)AmqpURI