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.

Sign up for GitHub

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

Jump to bottom

GH-3432: Add MQTT v5 channel adapters #3639

Merged
merged 4 commits into from
Oct 6, 2021
Merged

GH-3432: Add MQTT v5 channel adapters #3639

merged 4 commits into from
Oct 6, 2021

Conversation

artembilan
Copy link
Member

Fixes #3432

  • Add optional dependency for org.eclipse.paho:org.eclipse.paho.mqttv5.client
  • Add MqttProtocolErrorEvent and emit it from the mqttErrorOccurred() callback of the MQTT v5 client
  • Add MqttHeaderMapper since MQTT v5 has introduced user properties pair to transfer over the protocol
  • Add Mqttv5PahoMessageHandler as one more extension of the AbstractMqttMessageHandler
  • Add more convenient MqttHeaders constants for easier headers mapping configuration
  • Ensure via Mqttv5BackToBackTests that MQTT v5 is supported by the provided components
  • Change pr-build-workflow.yml to use eclipse-mosquitto container for testing all the MQTT interactions
  • Change cyrilix/rabbitmq-mqtt service to the rabbitmq:management since RabbitMQ does not support MQTT v5

@artembilan
spring-projectsGH-3432: Add MQTT v5 channel adapters
7bdb2a1 
Fixes spring-projects#3432

* Add `optional` dependency for `org.eclipse.paho:org.eclipse.paho.mqttv5.client`
* Add `MqttProtocolErrorEvent` and emit it from the `mqttErrorOccurred()` callback of the MQTT v5 client
* Add `MqttHeaderMapper` since MQTT v5 has introduced user properties pair to transfer over the protocol
* Add `Mqttv5PahoMessageHandler` as one more extension of the `AbstractMqttMessageHandler`
* Add more convenient `MqttHeaders` constants for easier headers mapping configuration
* Ensure via `Mqttv5BackToBackTests` that MQTT v5 is supported by the provided components
* Change `pr-build-workflow.yml` to use `eclipse-mosquitto` container for testing all the MQTT interactions
* Change `cyrilix/rabbitmq-mqtt` service to the `rabbitmq:management` since RabbitMQ does not support MQTT v5
@artembilan
Copy link
Member Author

If this is OK, I'll go ahead with Doc.

NOTE: RabbitMQ does not support MQTT v5, so I had to go with Mosquitto for both protocols testing.
We probably need to do something on the CI server to avoid problems with not supported protocol.

Thanks

@artembilan artembilan mentioned this pull request Oct 4, 2021
Closed
@artembilan
* Handle manual acks
084044f 
* Add `Mqttv5PahoMessageDrivenChannelAdapter.persistence` property
garyrussell
garyrussell suggested changes Oct 5, 2021
View reviewed changes
Copy link
Contributor

@garyrussell garyrussell left a comment

Choose a reason for hiding this comment

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

LGTM; you could use TestContainers with mosquitto for the tests

https://stackoverflow.com/questions/51383126/mqtt-client-testing-with-junit/51384651

...java/org/springframework/integration/mqtt/inbound/Mqttv5PahoMessageDrivenChannelAdapter.java Outdated
/**
* The {@link AbstractMqttMessageDrivenChannelAdapter} implementation for MQTT v5.
*
* The {@link MqttProperties} are mapped via provided {@link HeaderMapper};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* The {@link MqttProperties} are mapped via provided {@link HeaderMapper};
* The {@link MqttProperties} are mapped via the provided {@link HeaderMapper};

...tegration-mqtt/src/test/java/org/springframework/integration/mqtt/Mqttv5BackToBackTests.java Outdated
import org.springframework.messaging.converter.AbstractMessageConverter;
import org.springframework.messaging.converter.SmartMessageConverter;
import org.springframework.test.annotation.DirtiesContext;
import org.springframework.test.context.junit4.SpringRunner;
Copy link
Contributor

Choose a reason for hiding this comment

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

JUnit4 for a new test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right., because our BrokerRunning is JUnit 4 TestWatcher.

I don't want to introduce TestContainers into the project yet...

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but this won't work on CI because it will fail (RabbitMQ plugin detected).

@artembilan
* Add documentation
ee95a9c 
* Add `MosquittoContainerTest` for TestContainers support with Mosquitto image
@artembilan
Copy link
Member Author

artembilan commented Oct 6, 2021 •
edited
Loading

If this is OK, I can go ahead and change all the MQTT tests to use that MosquittoContainerTest aspect instead of JUnit 4 rule.
The Mosquitto easily supports all the MQTT protocols.
Then we can disable respective RabbitMQ plugin everywhere.

NOTE: to reuse container you should add testcontainers.reuse.enable=true into your ~/.testcontainers.properties.
Otherwise it is closed after test class and emits plain warn that reuse is not enabled.

garyrussell
garyrussell suggested changes Oct 6, 2021
View reviewed changes
Copy link
Contributor

@garyrussell garyrussell left a comment

Choose a reason for hiding this comment

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

I would say, yes, go ahead with the other tests, and deprecate BrokerRunning.

...egration-mqtt/src/test/java/org/springframework/integration/mqtt/MosquittoContainerTest.java

@Container
GenericContainer<?> MOSQUITTO_CONTAINER =
new GenericContainer<>("eclipse-mosquitto:2.0.12")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not latest?

Copy link
Member Author

Choose a reason for hiding this comment

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

The latest is somehow points to 2.0.7 on Docker Hub, which does not come with the /mosquitto-no-auth.conf config and does not expose a listener to external world for connection.
It allows only loopback connections from the same host. Kinda black box inside the same container.
That's why I was so struggle to find how to use this container in our tests: eclipse-mosquitto/mosquitto#2119.
As you see the fix has made it only starting 2.0.8 and we cannot use ENV vars smoothly either. So, only the custom command .withCommand("mosquitto -c /mosquitto-no-auth.conf") gave me a happy pass.

@garyrussell
Copy link
Contributor

Actually, deprecation is not needed; it can be removed - it's not a public API.

@artembilan
Copy link
Member Author

I would say, yes, go ahead with the other tests

Let's merge this first, then I'll revise the rest of test in the separate commit not bothering you with review!

Anyway let me know if all of this works well for you locally, before we merge and "break" other tests!

@garyrussell garyrussell merged commit e7c0d8d into spring-projects:main Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garyrussell garyrussell garyrussell requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

spring-integration-mqtt: Any plan to integrate with org.eclipse.paho.mqttv5.client
2 participants
@artembilan @garyrussell