Skip to content
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

Inform admins when new oauth2 account is created using spring rabbit events (gateway) #54

Conversation

marwanehcine
Copy link
Contributor

On this PR, we made use of spring rabbit events to send message from gateway to console in order to inform admin about new created OAuth2 account

@marwanehcine marwanehcine marked this pull request as draft August 24, 2023 08:15
@marwanehcine marwanehcine marked this pull request as ready for review August 24, 2023 08:18
@marwanehcine marwanehcine marked this pull request as draft August 24, 2023 08:18
gateway/pom.xml Outdated
@@ -66,6 +66,16 @@
<artifactId>lombok</artifactId>
<optional>true</optional>
</dependency>
<dependency>
<groupId>org.springframework.amqp</groupId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gateway is a spring-boot project, use the spring-boot-starter-amqp dependency instead, version managed by the spring-boot BOM

gateway/pom.xml Outdated Show resolved Hide resolved
gateway/pom.xml Outdated Show resolved Hide resolved
gateway/src/main/resources/rabbit-sender-context.xml Outdated Show resolved Hide resolved
gateway/src/main/resources/rabbit-listener-context.xml Outdated Show resolved Hide resolved
@Slf4j(topic = "org.georchestra.gateway.events")
public class EventsListener implements MessageListener {

private static Set<String> receivedMessageUid = new HashSet<String>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought receivedMessageUid was not used, didn't see the dependency on synReceivedMessageUid. Still confusing, why not instead of:

   private static Set<String> receivedMessageUid = new HashSet<String>();

    private static Set<String> synReceivedMessageUid = Collections.synchronizedSet(receivedMessageUid);

do just:

    private static Set<String> synReceivedMessageUid = Collections.synchronizedSet(new HashSet<>());

@marwanehcine marwanehcine force-pushed the inform_admins_when_new_oauth2_account_is_created_using_spring-rabbit_events branch 2 times, most recently from 50bd6c0 to 7e8d0c4 Compare August 27, 2023 10:49
@marwanehcine marwanehcine marked this pull request as ready for review August 27, 2023 10:49
Copy link
Member

@groldan groldan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EventsConfiguration is an autoconfiguration, please rename as EventsAutoConfiguration for the sake of conventions. Also, it is imperative that it's conditional in some form, and not enabled by default, for backwards compatibility. The gateway shall keep being able to deploy without a rabbitmq at all.
You can add for example @Profile("rabbitmq"), then the configuration class will only engage when the app is launched with the spring "rabbitmq" profile.

gateway/pom.xml Outdated Show resolved Hide resolved
gateway/pom.xml Outdated
<version>20230618</version>
</dependency>
<dependency>
<groupId>javax</groupId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment on why this is needed? otherwise it'll look strange on a webflux project

@marwanehcine marwanehcine force-pushed the inform_admins_when_new_oauth2_account_is_created_using_spring-rabbit_events branch from 7e8d0c4 to ef46c38 Compare August 30, 2023 18:44
Copy link
Member

@groldan groldan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@marwanehcine marwanehcine force-pushed the inform_admins_when_new_oauth2_account_is_created_using_spring-rabbit_events branch from ef46c38 to af3ed2d Compare September 2, 2023 16:05
@marwanehcine
Copy link
Contributor Author

EventsConfiguration is an autoconfiguration, please rename as EventsAutoConfiguration for the sake of conventions. Also, it is imperative that it's conditional in some form, and not enabled by default, for backwards compatibility. The gateway shall keep being able to deploy without a rabbitmq at all. You can add for example @Profile("rabbitmq"), then the configuration class will only engage when the app is launched with the spring "rabbitmq" profile.

I made use of property "enableRabbitmqEvents" because I need to check it before calling rabbitMQSender in "OpenIdConnectUserMapper.java". I think this way we can activate to desactivate rabbitmq autoconfiguration.
Thanks

@marwanehcine marwanehcine force-pushed the inform_admins_when_new_oauth2_account_is_created_using_spring-rabbit_events branch 3 times, most recently from 3e41ecb to c56c0f0 Compare September 13, 2023 21:11
@fvanderbiest
Copy link
Member

Can someone merge this one if it's OK with you ?
Thanks !

@marwanehcine
Copy link
Contributor Author

Can someone merge this one if it's OK with you ? Thanks !

Please wait I finish with port issues, Thanks

@marwanehcine marwanehcine force-pushed the inform_admins_when_new_oauth2_account_is_created_using_spring-rabbit_events branch from c56c0f0 to b2cb416 Compare September 19, 2023 09:37
@marwanehcine marwanehcine merged commit 8180e6a into georchestra:main Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants