Skip to content

Commit cf9295b

Browse files
snicollsnicoll
committed
Invoke ClientInterceptor#afterCompletion once per request
This commit reviews how afterCompletion is invoked. First of all, any exception thrown by an interceptor does not bubble up the stack to give a chance for other interceptors to clean things up. Then, rather than calling afterCompletion before handling a fault, an error, or the response body, it is invoked after. This gives a chance to any exception to break the flow and handle the completion in the catch block. Tests have been added to ensure that the callback is only invoked once. Closes gh-1054
1 parent 0c9cb71 commit cf9295b

File tree

2 files changed

+254
-4
lines changed

2 files changed

+254
-4
lines changed

‎spring-ws-core/src/main/java/org/springframework/ws/client/core/WebServiceTemplate.java‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -627,8 +627,9 @@ protected <T> T doSendAndReceive(MessageContext messageContext, WebServiceConnec
627627
if (!messageContext.hasResponse() && !intercepted) {
628628
sendRequest(connection, messageContext.getRequest());
629629
if (hasError(connection, messageContext.getRequest())) {
630+
Object fallback = handleError(connection, messageContext.getRequest());
630631
triggerAfterCompletion(interceptorIndex, messageContext, null);
631-
return (T) handleError(connection, messageContext.getRequest());
632+
return (T) fallback;
632633
}
633634
WebServiceMessage response = connection.receive(getMessageFactory());
634635
messageContext.setResponse(response);
@@ -637,13 +638,15 @@ protected <T> T doSendAndReceive(MessageContext messageContext, WebServiceConnec
637638
if (messageContext.hasResponse()) {
638639
if (!hasFault(connection, messageContext.getResponse())) {
639640
triggerHandleResponse(interceptorIndex, messageContext);
641+
T result = responseExtractor.extractData(messageContext.getResponse());
640642
triggerAfterCompletion(interceptorIndex, messageContext, null);
641-
return responseExtractor.extractData(messageContext.getResponse());
643+
return result;
642644
}
643645
else {
644646
triggerHandleFault(interceptorIndex, messageContext);
647+
Object fallback = handleFault(connection, messageContext);
645648
triggerAfterCompletion(interceptorIndex, messageContext, null);
646-
return (T) handleFault(connection, messageContext);
649+
return (T) fallback;
647650
}
648651
}
649652
else {
@@ -820,7 +823,12 @@ private void triggerAfterCompletion(int interceptorIndex, MessageContext message
820823
throws WebServiceClientException {
821824
if (this.interceptors != null) {
822825
for (int i = interceptorIndex; i >= 0; i--) {
823-
this.interceptors[i].afterCompletion(messageContext, ex);
826+
try {
827+
this.interceptors[i].afterCompletion(messageContext, ex);
828+
}
829+
catch (Exception interceptorEx) {
830+
logger.error("ClientInterceptor.afterCompletion threw exception", ex);
831+
}
824832
}
825833
}
826834
}

‎spring-ws-core/src/test/java/org/springframework/ws/client/core/WebServiceTemplateTest.java‎

Lines changed: 242 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,17 @@
1616

1717
package org.springframework.ws.client.core;
1818

19+
import java.io.IOException;
1920
import java.net.URI;
2021

2122
import javax.xml.transform.Result;
2223
import javax.xml.transform.Source;
24+
import javax.xml.transform.TransformerException;
2325

26+
import org.assertj.core.api.AbstractObjectAssert;
27+
import org.assertj.core.api.AbstractThrowableAssert;
28+
import org.assertj.core.api.AssertProvider;
29+
import org.assertj.core.api.Assertions;
2430
import org.junit.jupiter.api.BeforeEach;
2531
import org.junit.jupiter.api.Test;
2632

@@ -29,6 +35,7 @@
2935
import org.springframework.ws.MockWebServiceMessage;
3036
import org.springframework.ws.MockWebServiceMessageFactory;
3137
import org.springframework.ws.WebServiceMessage;
38+
import org.springframework.ws.client.WebServiceClientException;
3239
import org.springframework.ws.client.WebServiceTransportException;
3340
import org.springframework.ws.client.support.destination.DestinationProvider;
3441
import org.springframework.ws.client.support.interceptor.ClientInterceptor;
@@ -43,6 +50,8 @@
4350
import static org.assertj.core.api.Assertions.assertThat;
4451
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
4552
import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
53+
import static org.mockito.ArgumentMatchers.any;
54+
import static org.mockito.BDDMockito.given;
4655
import static org.mockito.Mockito.isA;
4756
import static org.mockito.Mockito.isNull;
4857
import static org.mockito.Mockito.mock;
@@ -418,6 +427,141 @@ public void testInterceptorsInterceptedCreateResponse() throws Exception {
418427
assertThat(result).isEqualTo(extracted);
419428
}
420429

430+
@Test
431+
void afterCompletionInvokedOnlyOnceWithSuccess() throws Exception {
432+
NoOpClientInterceptor clientInterceptor1 = new NoOpClientInterceptor();
433+
NoOpClientInterceptor clientInterceptor2 = new NoOpClientInterceptor();
434+
this.template.setInterceptors(new ClientInterceptor[] { clientInterceptor1, clientInterceptor2 });
435+
436+
WebServiceMessageCallback requestCallback = mock(WebServiceMessageCallback.class);
437+
requestCallback.doWithMessage(any(WebServiceMessage.class));
438+
Object extracted = new Object();
439+
WebServiceMessageExtractor<Object> extract = createSimpleExtractor(extracted);
440+
441+
this.connectionMock.send(isA(WebServiceMessage.class));
442+
when(this.connectionMock.hasError()).thenReturn(false);
443+
when(this.connectionMock.receive(this.messageFactory)).thenReturn(new MockWebServiceMessage("<response/>"));
444+
when(this.connectionMock.hasFault()).thenReturn(false);
445+
this.connectionMock.close();
446+
447+
Object result = this.template.sendAndReceive(requestCallback, extract);
448+
449+
assertThat(result).isEqualTo(extracted);
450+
assertThat(clientInterceptor1).hasHandledExchange();
451+
assertThat(clientInterceptor2).hasHandledExchange();
452+
}
453+
454+
@Test
455+
void afterCompletionInvokedOnlyOnceWitFailureInAfterCompletion() throws Exception {
456+
IllegalStateException testException = new IllegalStateException("test");
457+
NoOpClientInterceptor clientInterceptor1 = new NoOpClientInterceptor() {
458+
@Override
459+
public void afterCompletion(MessageContext messageContext, Exception ex) throws WebServiceClientException {
460+
super.afterCompletion(messageContext, ex);
461+
throw testException;
462+
}
463+
};
464+
NoOpClientInterceptor clientInterceptor2 = new NoOpClientInterceptor();
465+
this.template.setInterceptors(new ClientInterceptor[] { clientInterceptor1, clientInterceptor2 });
466+
467+
WebServiceMessageCallback requestCallback = mock(WebServiceMessageCallback.class);
468+
requestCallback.doWithMessage(any(WebServiceMessage.class));
469+
Object extracted = new Object();
470+
WebServiceMessageExtractor<Object> extract = createSimpleExtractor(extracted);
471+
472+
this.connectionMock.send(isA(WebServiceMessage.class));
473+
when(this.connectionMock.hasError()).thenReturn(false);
474+
when(this.connectionMock.receive(this.messageFactory)).thenReturn(new MockWebServiceMessage("<response/>"));
475+
when(this.connectionMock.hasFault()).thenReturn(false);
476+
this.connectionMock.close();
477+
478+
Object result = this.template.sendAndReceive(requestCallback, extract);
479+
480+
assertThat(result).isEqualTo(extracted);
481+
assertThat(clientInterceptor1).hasHandledExchange().hasNoCompletionException();
482+
assertThat(clientInterceptor2).hasHandledExchange().hasNoCompletionException();
483+
}
484+
485+
@Test
486+
void afterCompletionInvokedOnlyOnceWithError() throws Exception {
487+
NoOpClientInterceptor clientInterceptor1 = new NoOpClientInterceptor();
488+
NoOpClientInterceptor clientInterceptor2 = new NoOpClientInterceptor();
489+
this.template.setInterceptors(new ClientInterceptor[] { clientInterceptor1, clientInterceptor2 });
490+
491+
Object extracted = new Object();
492+
WebServiceMessageExtractor<Object> extract = createSimpleExtractor(extracted);
493+
494+
this.connectionMock.send(isA(WebServiceMessage.class));
495+
when(this.connectionMock.hasError()).thenReturn(true);
496+
when(this.connectionMock.hasFault()).thenReturn(false);
497+
String errorMessage = "errorMessage";
498+
when(this.connectionMock.getErrorMessage()).thenReturn(errorMessage);
499+
this.connectionMock.close();
500+
501+
assertThatExceptionOfType(WebServiceTransportException.class)
502+
.isThrownBy(() -> this.template.sendAndReceive(null, extract))
503+
.satisfies(exception -> {
504+
assertThat(clientInterceptor1).hasHandledError().completionException().isSameAs(exception);
505+
assertThat(clientInterceptor2).hasHandledError().completionException().isSameAs(exception);
506+
});
507+
}
508+
509+
@Test
510+
void afterCompletionInvokedOnlyOnceWithFault() throws Exception {
511+
NoOpClientInterceptor clientInterceptor1 = new NoOpClientInterceptor();
512+
NoOpClientInterceptor clientInterceptor2 = new NoOpClientInterceptor();
513+
this.template.setInterceptors(new ClientInterceptor[] { clientInterceptor1, clientInterceptor2 });
514+
this.template.setFaultMessageResolver(null);
515+
516+
WebServiceMessageExtractor<Object> extractorMock = createSimpleExtractor(new Object());
517+
MockWebServiceMessage response = new MockWebServiceMessage("<response/>");
518+
response.setFault(true);
519+
520+
this.connectionMock.send(isA(WebServiceMessage.class));
521+
when(this.connectionMock.hasError()).thenReturn(false);
522+
when(this.connectionMock.hasFault()).thenReturn(true);
523+
when(this.connectionMock.receive(this.messageFactory)).thenReturn(response);
524+
this.connectionMock.close();
525+
526+
assertThatExceptionOfType(WebServiceTransportException.class)
527+
.isThrownBy(() -> this.template.sendAndReceive(null, extractorMock))
528+
.satisfies(exception -> {
529+
assertThat(clientInterceptor1).hasHandledFault().completionException().isSameAs(exception);
530+
assertThat(clientInterceptor2).hasHandledFault().completionException().isSameAs(exception);
531+
});
532+
}
533+
534+
@Test
535+
void afterCompletionInvokedOnlyOnceWithFaultAndFaultMessageResolver() throws Exception {
536+
NoOpClientInterceptor clientInterceptor1 = new NoOpClientInterceptor();
537+
NoOpClientInterceptor clientInterceptor2 = new NoOpClientInterceptor();
538+
this.template.setInterceptors(new ClientInterceptor[] { clientInterceptor1, clientInterceptor2 });
539+
540+
WebServiceMessageExtractor<Object> extractorMock = createSimpleExtractor(new Object());
541+
FaultMessageResolver faultMessageResolverMock = mock(FaultMessageResolver.class);
542+
faultMessageResolverMock.resolveFault(isA(WebServiceMessage.class));
543+
this.template.setFaultMessageResolver(faultMessageResolverMock);
544+
545+
MockWebServiceMessage response = new MockWebServiceMessage("<response/>");
546+
response.setFault(true);
547+
548+
this.connectionMock.send(isA(WebServiceMessage.class));
549+
when(this.connectionMock.hasError()).thenReturn(false);
550+
when(this.connectionMock.hasFault()).thenReturn(true);
551+
when(this.connectionMock.receive(this.messageFactory)).thenReturn(response);
552+
this.connectionMock.close();
553+
554+
this.template.sendAndReceive(null, extractorMock);
555+
assertThat(clientInterceptor1).hasHandledFault().hasNoCompletionException();
556+
assertThat(clientInterceptor2).hasHandledFault().hasNoCompletionException();
557+
}
558+
559+
private <T> WebServiceMessageExtractor<T> createSimpleExtractor(T target) throws IOException, TransformerException {
560+
WebServiceMessageExtractor<T> extractor = mock(WebServiceMessageExtractor.class);
561+
given(extractor.extractData(any(WebServiceMessage.class))).willReturn(target);
562+
return extractor;
563+
}
564+
421565
@Test
422566
public void testDestinationResolver() throws Exception {
423567

@@ -456,4 +600,102 @@ public boolean supports(URI uri) {
456600
assertThat(result).isNull();
457601
}
458602

603+
private static class NoOpClientInterceptor
604+
implements ClientInterceptor, AssertProvider<NoOpClientInterceptorAssert> {
605+
606+
private boolean handledRequest;
607+
608+
private boolean handledResponse;
609+
610+
private boolean handledFault;
611+
612+
private boolean afterCompletion;
613+
614+
private Exception afterCompletionException;
615+
616+
@Override
617+
public boolean handleRequest(MessageContext messageContext) throws WebServiceClientException {
618+
if (this.handledRequest) {
619+
throw new IllegalStateException("handleRequest has already been called");
620+
}
621+
this.handledRequest = true;
622+
return true;
623+
}
624+
625+
@Override
626+
public boolean handleResponse(MessageContext messageContext) throws WebServiceClientException {
627+
if (this.handledResponse) {
628+
throw new IllegalStateException("handleResponse has already been called");
629+
}
630+
this.handledResponse = true;
631+
return true;
632+
}
633+
634+
@Override
635+
public boolean handleFault(MessageContext messageContext) throws WebServiceClientException {
636+
if (this.handledFault) {
637+
throw new IllegalStateException("handleFault has already been called");
638+
}
639+
this.handledFault = true;
640+
return true;
641+
}
642+
643+
@Override
644+
public void afterCompletion(MessageContext messageContext, Exception ex) throws WebServiceClientException {
645+
if (this.afterCompletion) {
646+
throw new IllegalStateException("afterCompletion has already been called");
647+
}
648+
this.afterCompletion = true;
649+
this.afterCompletionException = ex;
650+
}
651+
652+
@Override
653+
public NoOpClientInterceptorAssert assertThat() {
654+
return new NoOpClientInterceptorAssert(this);
655+
}
656+
657+
}
658+
659+
private static class NoOpClientInterceptorAssert
660+
extends AbstractObjectAssert<NoOpClientInterceptorAssert, NoOpClientInterceptor> {
661+
662+
NoOpClientInterceptorAssert(NoOpClientInterceptor actual) {
663+
super(actual, NoOpClientInterceptorAssert.class);
664+
}
665+
666+
NoOpClientInterceptorAssert hasHandledExchange() {
667+
assertThat(this.actual.handledRequest).isTrue();
668+
assertThat(this.actual.handledResponse).isTrue();
669+
assertThat(this.actual.handledFault).isFalse();
670+
assertThat(this.actual.afterCompletion).isTrue();
671+
return this.myself;
672+
}
673+
674+
NoOpClientInterceptorAssert hasHandledError() {
675+
assertThat(this.actual.handledRequest).isTrue();
676+
assertThat(this.actual.handledResponse).isFalse();
677+
assertThat(this.actual.handledFault).isFalse();
678+
assertThat(this.actual.afterCompletion).isTrue();
679+
return this.myself;
680+
}
681+
682+
NoOpClientInterceptorAssert hasHandledFault() {
683+
assertThat(this.actual.handledRequest).isTrue();
684+
assertThat(this.actual.handledResponse).isFalse();
685+
assertThat(this.actual.handledFault).isTrue();
686+
assertThat(this.actual.afterCompletion).isTrue();
687+
return this.myself;
688+
}
689+
690+
NoOpClientInterceptorAssert hasNoCompletionException() {
691+
assertThat(this.actual.afterCompletionException).isNull();
692+
return this.myself;
693+
}
694+
695+
AbstractThrowableAssert<?, ? extends Exception> completionException() {
696+
return Assertions.assertThat(this.actual.afterCompletionException);
697+
}
698+
699+
}
700+
459701
}

0 commit comments

Comments
 (0)