Skip to content

AMQP CorrelationDataWrapper Needs to Delegate New Methods #2759

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

Closed
garyrussell opened this issue Feb 21, 2019 · 2 comments
Closed

AMQP CorrelationDataWrapper Needs to Delegate New Methods #2759

garyrussell opened this issue Feb 21, 2019 · 2 comments
Assignees
Milestone

Comments

@garyrussell
Copy link
Contributor

garyrussell commented Feb 21, 2019

Affects Version(s): all

The AmqpOutboundEndpoint wraps the user CorrelationData in a CorrelationDataWrapper.

This wrapper must delegate getFuture() and setReturnedMessage() methods to the delegate.

Needs a change in Spring AMQP to make setReturnedMessage() public (or use reflection).

See https://stackoverflow.com/questions/54713121/spring-integration-publisher-confirms-with-timeout/54799635#comment96403650_54799635

@garyrussell
Copy link
Contributor Author

@artembilan Instead of making setReturnedMessage() public, I propose making it protected and with the following...

	protected static final class CorrelationDataWrapper extends CorrelationData {

		private static final Method setReturnedMessage;

		static {
			AtomicReference<Method> method = new AtomicReference<>();
			ReflectionUtils.doWithMethods(CorrelationData.class, m -> {
				m.setAccessible(true);
				method.set(m);
			}, m -> {
				return m.getName().equals("setReturnedMessage");
			});
			setReturnedMessage = method.get();
		}

		private final Object userData;

		private final Message<?> message;

		CorrelationDataWrapper(String id, Object userData, Message<?> message) {
			super(id);
			this.userData = userData;
			this.message = message;
		}

		public Object getUserData() {
			return this.userData;
		}

		public Message<?> getMessage() {
			return this.message;
		}

		@Override
		public SettableListenableFuture<Confirm> getFuture() {
			if (userData instanceof CorrelationData) {
				return ((CorrelationData) userData).getFuture();
			}
			else {
				return super.getFuture();
			}
		}

		@Override
		public org.springframework.amqp.core.Message getReturnedMessage() {
			return super.getReturnedMessage();
		}

		@Override
		public void setReturnedMessage(org.springframework.amqp.core.Message returnedMessage) {
			if (this.userData instanceof CorrelationData && setReturnedMessage != null) {
				try {
					setReturnedMessage.invoke(this.userData, returnedMessage);
				}
				catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) {
					// empty
				}
			}
			super.setReturnedMessage(returnedMessage);
		}
	}

If you agree, I'll push the change to AMQP and issue a PR for this.

@artembilan
Copy link
Member

To be honest I don't see any problems to make that CorrelationData.setReturnedMessage() as public.

I think we have similar discussion in the past about other CorrelationData properties and our decision was to let them all be public to avoid such a reflection kung-fu!

garyrussell added a commit to spring-projects/spring-amqp that referenced this issue Feb 21, 2019
garyrussell added a commit to spring-projects/spring-amqp that referenced this issue Feb 21, 2019
garyrussell added a commit to garyrussell/spring-integration that referenced this issue Feb 21, 2019
Fixes spring-projects#2759

The outbound endpoints wrap user correlation data in a wrapper.
If the user data is a `CorrelationData`, we must delegate methods
involving the `Future<?>` and `returnedMessage` to the user data.
garyrussell added a commit to garyrussell/spring-integration that referenced this issue Feb 21, 2019
Fixes spring-projects#2759

The outbound endpoints wrap user correlation data in a wrapper.
If the user data is a `CorrelationData`, we must delegate methods
involving the `Future<?>` and `returnedMessage` to the user data.

**cherry-pick to 2.1 and switch AMQP to snapshots**
artembilan pushed a commit that referenced this issue Feb 22, 2019
* GH-2759: Fix CorrelationData.future

Fixes #2759

The outbound endpoints wrap user correlation data in a wrapper.
If the user data is a `CorrelationData`, we must delegate methods
involving the `Future<?>` and `returnedMessage` to the user data.

**cherry-pick to 5.1 and switch AMQP to snapshots**

* Polishing - remove redundant override.

* Add debug log with null correlation data
artembilan pushed a commit that referenced this issue Feb 22, 2019
* GH-2759: Fix CorrelationData.future

Fixes #2759

The outbound endpoints wrap user correlation data in a wrapper.
If the user data is a `CorrelationData`, we must delegate methods
involving the `Future<?>` and `returnedMessage` to the user data.

**cherry-pick to 5.1 and switch AMQP to snapshots**

* Polishing - remove redundant override.

* Add debug log with null correlation data

# Conflicts:
#	spring-integration-amqp/src/test/java/org/springframework/integration/amqp/outbound/AmqpOutboundEndpointTests.java

* Updated Spring AMQP dependency to `2.1.5.BUILD-SNAPSHOT`
* Moved `AmqpOutboundEndpointTests` assertions to `AssertJ` to avoid
conflicts with `master`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants