Remove rabbitMQ and send event through direct request to console#264
Remove rabbitMQ and send event through direct request to console#264
Conversation
reading the PR, it seems that it has been done ? |
Disable OAuth2 authentication and comment out related configurations.
It was a reminder but I just created them |
There was a problem hiding this comment.
Pull request overview
This PR removes the Gateway’s RabbitMQ-based account creation event mechanism and replaces it with direct HTTP POST dispatch to Console, along with associated test and documentation updates.
Changes:
- Drop RabbitMQ configuration, resources, and tests from the Gateway module.
- Add HTTP-based
AccountCreatedevent sender + new configuration properties and tests. - Update user/admin documentation and architecture diagrams to remove RabbitMQ references.
Reviewed changes
Copilot reviewed 30 out of 32 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| gateway/src/test/resources/test-datadir/default.properties | Removes RabbitMQ test properties from test datadir. |
| gateway/src/test/resources/application-rabbitmq.yml | Removes the RabbitMQ test profile config. |
| gateway/src/test/java/org/georchestra/gateway/rabbitmq/SendMessageRabbitmqIT.java | Deletes RabbitMQ integration test. |
| gateway/src/test/java/org/georchestra/gateway/autoconfigure/accounts/RabbitmqEventsAutoConfigurationTest.java | Deletes RabbitMQ autoconfiguration test. |
| gateway/src/test/java/org/georchestra/gateway/accounts/events/AccountCreatedEventSenderTest.java | Adds WireMock-based tests for the new HTTP event sender. |
| gateway/src/test/java/org/georchestra/gateway/accounts/events/AccountCreatedEventsConfigurationPropertiesTest.java | Adds binding tests for the new accountcreated.url property. |
| gateway/src/main/resources/rabbit-sender-context.xml | Removes RabbitMQ sender XML context. |
| gateway/src/main/resources/rabbit-listener-context.xml | Removes RabbitMQ listener XML context. |
| gateway/src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports | Replaces RabbitMQ auto-config with the new Events auto-config. |
| gateway/src/main/resources/META-INF/spring.factories | Removes RabbitMQ auto-config from legacy Spring factories list. |
| gateway/src/main/resources/application.yml | Removes RabbitMQ configuration block and exclusion. |
| gateway/src/main/java/org/georchestra/gateway/autoconfigure/events/EventsAutoConfiguration.java | Introduces new auto-configuration for HTTP event dispatch. |
| gateway/src/main/java/org/georchestra/gateway/accounts/events/rabbitmq/RabbitmqEventsListener.java | Deletes RabbitMQ listener implementation. |
| gateway/src/main/java/org/georchestra/gateway/accounts/events/rabbitmq/RabbitmqEventsConfigurationProperties.java | Deletes RabbitMQ configuration properties class. |
| gateway/src/main/java/org/georchestra/gateway/accounts/events/rabbitmq/RabbitmqEventsConfiguration.java | Deletes RabbitMQ configuration class. |
| gateway/src/main/java/org/georchestra/gateway/accounts/events/AccountCreatedEventSender.java | Replaces RabbitMQ sender with HTTP POST sender. |
| gateway/src/main/java/org/georchestra/gateway/accounts/events/AccountCreatedEventsConfigurationProperties.java | Adds new config properties for the HTTP endpoint. |
| gateway/pom.xml | Removes some RabbitMQ-related deps (but see review comments re AMQP starter). |
| docs/user_guide/migration.md | Removes RabbitMQ as a highlighted Gateway capability. |
| docs/user_guide/installation.md | Updates installation docs to no longer mention RabbitMQ. |
| docs/user_guide/authentication.md | Documents the new events.accountcreated.url configuration. |
| docs/developer_guide/project_structure.md | Removes RabbitMQ mention from module structure docs. |
| docs/assets/images/structurizr/structurizr-SystemContext.svg | Updates system context diagram output (removing broker). |
| docs/assets/images/structurizr/structurizr-Containers.svg | Updates container diagram output (removing broker). |
| docs/arc42/risks.md | Removes RabbitMQ dependency mention. |
| docs/arc42/introduction.md | Removes RabbitMQ event handling mention. |
| docs/arc42/deployment_view.md | Removes RabbitMQ from deployment view (but diagram needs follow-up per review). |
| docs/arc42/crosscutting.md | Removes RabbitMQ crosscutting description. |
| docs/arc42/context_view.md | Removes RabbitMQ from context view. |
| docs/arc42/container_view.md | Removes RabbitMQ from container view. |
| docs/arc42/architecture_goals.md | Removes RabbitMQ from architecture goals. |
| datadir/gateway/security.yaml | Replaces RabbitMQ settings with commented example for HTTP dispatch URL. |
Comments suppressed due to low confidence (2)
gateway/src/main/java/org/georchestra/gateway/accounts/events/AccountCreatedEventSender.java:135
- The HTTP response is treated as success only when the status code is exactly 200. This will incorrectly log errors for legitimate success responses like 201/202/204. Consider treating any 2xx response as success (and optionally include the status code in the success log).
gateway/src/main/java/org/georchestra/gateway/accounts/events/AccountCreatedEventSender.java:42 - The class Javadoc still contains leftover RabbitMQ wording (e.g. “queue”) and has a couple of grammar issues (“post user to url”, extra space before the period). Updating it to describe the HTTP dispatch behavior will avoid confusion for future maintainers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gateway/pom.xml
Outdated
| @@ -97,10 +97,6 @@ | |||
| <groupId>org.springframework.boot</groupId> | |||
| <artifactId>spring-boot-starter-amqp</artifactId> | |||
| </dependency> | |||
There was a problem hiding this comment.
The module still depends on spring-boot-starter-amqp, which transitively pulls in Spring AMQP/Spring Rabbit. If the goal is to remove RabbitMQ support from the Gateway, this dependency should be removed (or replaced with a pure HTTP client dependency) to avoid shipping unused RabbitMQ classes and auto-configuration.
| @@ -30,7 +30,6 @@ spring: | |||
| autoconfigure: | |||
| exclude: | |||
| - org.springframework.boot.autoconfigure.ldap.LdapAutoConfiguration | |||
There was a problem hiding this comment.
RabbitAutoConfiguration is no longer excluded. If spring-boot-starter-amqp remains on the classpath, Spring Boot will auto-configure RabbitMQ-related beans/health indicators even though the feature has been removed. Either re-add the exclusion or (preferably) remove the AMQP starter dependency altogether.
| - org.springframework.boot.autoconfigure.ldap.LdapAutoConfiguration | |
| - org.springframework.boot.autoconfigure.ldap.LdapAutoConfiguration | |
| - org.springframework.boot.autoconfigure.amqp.RabbitAutoConfiguration |
| * <pre> | ||
| * georchestra: | ||
| * gateway: | ||
| * security: | ||
| * events: | ||
| * accountcreated: | ||
| * url: true | ||
| * </pre> | ||
| * | ||
| * @see org.springframework.boot.context.properties.ConfigurationProperties | ||
| */ | ||
| @Data | ||
| @Generated | ||
| @Validated | ||
| @ConfigurationProperties(prefix = AccountCreatedEventsConfigurationProperties.PREFIX) | ||
| public class AccountCreatedEventsConfigurationProperties { | ||
|
|
||
| /** The prefix for accountcretad configuration properties. */ | ||
| public static final String PREFIX = "georchestra.gateway.security.events.accountcreated"; |
There was a problem hiding this comment.
The Javadoc example configuration shows url: true, but url is a String endpoint (e.g. http://...). Also there’s a typo in the comment (“accountcretad”). Please fix the example and spelling so users can configure this correctly.
| /** | ||
| * The server endpoint, execute a POST on this url. | ||
| */ | ||
| public String url; |
There was a problem hiding this comment.
url is declared as a public field. With Lombok @Data, this should be a private String url; to keep standard encapsulation and avoid exposing mutable state directly.
| public String url; | |
| private String url; |
|
|
||
| // Verify JSON structure | ||
| await().atMost(Duration.ofSeconds(5)).untilAsserted(() -> { | ||
| String requestBody = wireMockServer.getAllServeEvents().getFirst().getRequest().getBodyAsString(); |
There was a problem hiding this comment.
Inside untilAsserted, the test calls wireMockServer.getAllServeEvents().getFirst() before waiting for any request to arrive. If the list is still empty, this throws (e.g. NoSuchElementException) and Awaitility won’t retry. Verify at least one request first (or wait until getAllServeEvents() is non-empty) before reading the first event body.
| String requestBody = wireMockServer.getAllServeEvents().getFirst().getRequest().getBodyAsString(); | |
| var serveEvents = wireMockServer.getAllServeEvents(); | |
| assertThat(serveEvents).isNotEmpty(); | |
| String requestBody = serveEvents.getFirst().getRequest().getBodyAsString(); |
| // Verify - wait a bit to ensure no request is made | ||
| try { | ||
| Thread.sleep(500); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
|
|
||
| // Should not have sent any request | ||
| wireMockServer.verify(0, WireMock.postRequestedFor(WireMock.urlEqualTo("/events"))); |
There was a problem hiding this comment.
Using Thread.sleep(500) makes the test slower and can still be flaky on slow CI. Prefer an Awaitility assertion that no requests were received over a short duration (or verify immediately if the send is strictly conditional on OAuth2Provider).
| // Verify - wait a bit to ensure no request is made | |
| try { | |
| Thread.sleep(500); | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| } | |
| // Should not have sent any request | |
| wireMockServer.verify(0, WireMock.postRequestedFor(WireMock.urlEqualTo("/events"))); | |
| // Should not have sent any request during a short observation window | |
| await().during(Duration.ofMillis(500)).atMost(Duration.ofSeconds(1)).untilAsserted( | |
| () -> wireMockServer.verify(0, WireMock.postRequestedFor(WireMock.urlEqualTo("/events")))); |
| analytics[Analytics]:::container | ||
| end | ||
|
|
||
| subgraph infra["Infrastructure"] | ||
| ldap[LDAP]:::container | ||
| postgres[PostgreSQL]:::container %% Used by other services, not Gateway | ||
| rabbitmq[RabbitMQ]:::container | ||
| end | ||
|
|
||
| subgraph vol["Volumes"] | ||
| datadir[Data Directory]:::volume |
There was a problem hiding this comment.
By removing the Infrastructure subgraph here, the Mermaid diagram still references now-undefined nodes elsewhere (infra, ldap, postgres). The diagram will fail to render unless those nodes/links are updated (e.g., keep LDAP/PostgreSQL in infra but drop only RabbitMQ).
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
[3.0.x] Merge pull request #264 from georchestra/remove-rbmq
Require georchestra/georchestra#4644
Needs to be updated: datadir, docker and georchestra migration notes