Skip to content

Conversation

@davidmestr
Copy link
Contributor

We want yo migrate from another JMS implentations to RabbitMQ, and we would like to use this library. But we have some security constraints related to permissions and exchanges and queues naming.

With this PR, we have extracted some hard-coded constants to a class using Singleton pattern. By default the behavior is the same as before, but with this change it is possible to change:

  • JMS_DURABLE_TOPIC_EXCHANGE_NAME = "jms.durable.topic";
  • JMS_TEMP_TOPIC_EXCHANGE_NAME = "jms.temp.topic";
  • JMS_DURABLE_QUEUE_EXCHANGE_NAME = "jms.durable.queues";
  • JMS_TEMP_QUEUE_EXCHANGE_NAME = "jms.temp.queues";
  • JMS_CONSUMER_QUEUE_NAME_PREFIX = "jms-cons-";
  • JMS_DURABLE_TOPIC_SELECTOR_EXCHANGE_PREFIX = "jms-dutop-slx-";
  • JMS_NON_DURABLE_TOPIC_SELECTOR_EXCHANGE_PREFIX = "jms-ndtop-slx-";
  • JMS_TEMP_QUEUE_PREFIF = "jms-temp-queue-";
  • JMS_TEMP_TOPIC_PREFIF = "jms-temp-topic-";

@acogoluegnes
Copy link
Contributor

I would rather create a naming strategy interface and inject it into the RMQConnectionFactory. It would be a more flexible and reliable solution (changing the state of a static instance is fragile).

BTW, the singleton implementation is broken: the singleton can be created several times because of a race condition. It is not that big a deal because it is cheap to create. There is also no guarantee that it will become visible to a thread different from the one that created it. Same thing with the properties, whether they have been just initialized or updated. The implementation of a naming strategy with final properties would avoid these problems.

There are also typos (JMS_TEMP_QUEUE_PREFIF and JMS_TEMP_TOPIC_PREFIF).

@davidmestr davidmestr marked this pull request as draft September 30, 2024 08:03
@davidmestr
Copy link
Contributor Author

I agree that using singleton is not the best solution, but it seemed to me the least intrusive and most straightforward.
I try to implement your proposal.

@davidmestr davidmestr changed the title feature: Extract destinations constants to singleton feature: Extract destinations constants to strategy naming interface Sep 30, 2024
@davidmestr davidmestr marked this pull request as ready for review September 30, 2024 11:01
@davidmestr
Copy link
Contributor Author

Hi @acogoluegnes! A proposal of implementation based on interface strategy. Any name change or improvement idea, let me know. When the implementation is fine, we can add some unit tests.

@acogoluegnes
Copy link
Contributor

@davidmestr Thanks for the changes. I am merging it as-is and I will do some polishing (e.g. we should keep the existing constructors of RMQDestination for backward compatibility).

@acogoluegnes acogoluegnes merged commit 448e5e8 into rabbitmq:main Sep 30, 2024
1 check passed
github-actions bot pushed a commit that referenced this pull request Sep 30, 2024
feature: Extract destinations constants to strategy naming interface
acogoluegnes added a commit that referenced this pull request Oct 1, 2024
github-actions bot pushed a commit that referenced this pull request Oct 1, 2024
@acogoluegnes
Copy link
Contributor

@davidmestr I pushed some changes: f1e6926.

Can you have a look before I cut a minor release?

acogoluegnes added a commit that referenced this pull request Oct 1, 2024
References #513

(cherry picked from commit f1e6926)

Conflicts:
	src/main/java/com/rabbitmq/jms/admin/RMQConnectionFactory.java
	src/main/java/com/rabbitmq/jms/client/RMQConnection.java
acogoluegnes added a commit that referenced this pull request Oct 1, 2024
github-actions bot pushed a commit that referenced this pull request Oct 1, 2024
@davidmestr davidmestr deleted the feature/custom-rmqdestination branch October 1, 2024 14:47
@davidmestr
Copy link
Contributor Author

davidmestr commented Oct 1, 2024

@acogoluegnes It is working fine.

One drawback of not using default methods in the interface is that it forces you to implement all the methods, even when you might only need a few. But it is okay.

Thank you!

@acogoluegnes
Copy link
Contributor

One drawback of not using default methods in the interface is that it forces you to implement all the methods, even when you might only need a few. But it is okay.

Right. I tend to use default methods when adding new methods to an existing interface, to avoid a breaking change. I am a bit opinionated about this, sorry :-)

I also changed the name of some of the methods, to make them hopefully more explicit.

If this is OK for you I'll cut a release this week.

@davidmestr
Copy link
Contributor Author

It is OK! Thanks

@acogoluegnes acogoluegnes added this to the 3.4.0 milestone Oct 4, 2024
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.

2 participants