Add auto-configuration for WebServiceTemplate #12707

nosan · 2018-03-31T12:11:14Z

Added the WebServiceTemplateBuilder (similar to the RestTemplateBuilder) helper class for building WebServiceTemplate.

        WebServiceTemplateBuilder webServiceTemplateBuilder = new WebServiceTemplateBuilder();
        WebServiceTemplate webServiceTemplate = webServiceTemplateBuilder
                .setConnectionTimeout(connectTimeout)
                .setReadTimeout(readTimeout)
                .addCustomizers(customizers())
                .addInterceptors(interceptors())
                .addWebServiceMessageSenders(webServiceMessageSenders())
                .setCheckConnectionForFault(true)
                .setCheckConnectionForError(true)
                .setMarshaller(marshaller)
                .setUnmarshaller(unmarshaller)
                .setDefaultUri(defaultUri)
                ...
                .build();

In a typical auto-configured Spring Boot application this builder will be available as bean and can be injected whenever a WebServiceTemplate is needed.

nosan · 2018-04-11T15:35:14Z

I've removed the Jaxb2MarshallerBuilder from this PR.
I think it's much better to have it in a separate PR if we still needed it.

nosan · 2018-04-13T09:35:04Z

@philwebb
I was hoping to hear some news from you regarding my pull request.
Would you be so kind to check that, if you are not checking it by now? Maybe you have some questions/concerns regarding some code? I will be more than happy to explain all the misunderstanding moments.

philwebb · 2018-04-13T21:35:51Z

@nosan I've been primarily focusing on bugs and regressions in the 2.0.x line so feature PRs haven't had much of my attention lately.

The code looks very complete but I guess my primary concern is with how many people are currently using WebServiceTemplate, and if the code would be best as part of Spring Boot or if some of it should live in spring-ws. @gregturn might have some thoughts about that.

I'm probably not going to have time to give much feedback on this in the near-term, but as a team we'll be looking again at PRs once work on 2.1 starts to ramp up.

nosan · 2018-04-14T10:41:56Z

@philwebb Thank you for your reply. Looking forward to hearing from you soon.

rfelgent · 2018-05-08T21:41:46Z

Out of curiosity: Aren't there other libraries already providing the feature to create webserivce client stubs like CXF? Or do I misunderstand the idea of WebServiceTemplateBuilder ?

nosan · 2018-05-29T09:48:21Z

@snicoll I saw that you have assigned a review for yourself, thank you for this. FYI I have done some small changes on this one (I removed reflections where it is possible). I hope I didn't break your review.

nosan · 2018-05-29T10:14:08Z

@rfelgent spring-ws has a WebServiceTemplate class. PR makes the creation of WebServiceTemplate much easier, also this builder could be shared between beans.

In the future, it would be great to have metrics for it as spring-boot has for RestTemplate.

See spring-projectsgh-12707

snicoll

Thanks a lot for the PR. I've started to polish and spend quite some time on it but they were a few items I'd like to discuss before going any further.

See the individual comments. I've started a polish here so I might update it based on our discussion rather than rebasing on your branch.

snicoll · 2018-05-31T10:30:24Z

...oot/src/main/java/org/springframework/boot/webservices/client/WebServiceTemplateBuilder.java

+	 * @return a new builder instance
+	 * @see WebServiceTemplate#setInterceptors(ClientInterceptor[])
+	 */
+	public WebServiceTemplateBuilder setInterceptors(ClientInterceptor... interceptors) {


I prefer if we keep the same style as what we're doing with RestTemplate (i.e. interceptors and additionalInterceptors). I've already started a polish on my fork so need to act on it.

snicoll · 2018-05-31T10:31:32Z

...oot/src/main/java/org/springframework/boot/webservices/client/WebServiceTemplateBuilder.java

+	 */
+
+	public WebServiceTemplateBuilder setWebServiceMessageSenders(
+			Collection<? extends Supplier<? extends WebServiceMessageSender>> webServiceMessageSenderSuppliers) {


I've removed the Supplier in my polish commit. I didn't found a single use of it and would argue that it is something you can determine when you build the template

You can't do this because of 'setReadTimeout' and 'setConnectionTimeout'. This test will not work.

HttpUrlConnectionMessageSender sender = new HttpUrlConnectionMessageSender(); WebServiceTemplate ws = this.builder .setConnectionTimeout(5000) .setReadTimeout(2000) .messageSenders(sender) .build(); WebServiceTemplate ws1 = this.builder .setConnectionTimeout(4000) .setReadTimeout(3000) .build(); assertThat(ReflectionTestUtils.getField(ws.getMessageSenders()[0], "connectionTimeout")) .isEqualTo(Duration.ofMillis(5000)); assertThat(ReflectionTestUtils.getField(ws.getMessageSenders()[0], "readTimeout")) .isEqualTo(Duration.ofMillis(2000)); // assertThat(ReflectionTestUtils.getField(ws1.getMessageSenders()[0], "connectionTimeout")).isEqualTo(Duration.ofMillis(4000)); assertThat(ReflectionTestUtils.getField(ws1.getMessageSenders()[0], "readTimeout")) .isEqualTo(Duration.ofMillis(3000));

With Suppliers you will not have this issue, except this case () -> WebServiceMessageSender instead of WebServiceMessageSender::new | () -> new WebServiceMessageSender(). Also, this issue will be manifest themselves only for custom WebServiceMessageSender

IMO that's one more argument to remove that feature.

Only for custom WebServiceMessagaSenders

snicoll · 2018-05-31T10:32:35Z

...oot/src/main/java/org/springframework/boot/webservices/client/WebServiceTemplateBuilder.java

+	public WebServiceTemplateBuilder setCheckConnectionForFault(
+			boolean checkConnectionForFault) {
+		return new WebServiceTemplateBuilder(this.interceptors,
+				append(this.internalCustomizers,


I don't like this pattern. If you call the setter several times, you'll append this customizer and rely on the fact the last one called will set the expected value. I guess you've done this that way to avoid adding too much parameters to the builder? I think we need to find a different option for this.

it could be handled by Boolean type, but you still need a custom customizer for FaultMessageResolver.

Yes, I did this to avoid adding too many parameters.

In the meantime I've seen we're doing that with RestTemplateBuilder and I see you're using a separate collection. Retrospectively, it's not that bad at all.

snicoll · 2018-05-31T10:34:05Z

...oot/src/main/java/org/springframework/boot/webservices/client/WebServiceTemplateBuilder.java

+	 * @see BeanUtils#instantiateClass(Class)
+	 */
+
+	public WebServiceTemplateBuilder setWebServiceMessageSender(


there are too many flavours to set a WebServiceMessageSender. Specifying a vararg of instance and a Class looks wrong to me. I've removed that in my fork.

the was an idea to have an extra method with a Class instead of a Supplier.

Got it but I think the reason why a Supplier was added is wrong.

snicoll · 2018-05-31T10:37:52Z

...oot/src/main/java/org/springframework/boot/webservices/client/WebServiceTemplateBuilder.java

+			private ClientHttpRequestFactory getRequestFactory(
+					ClientHttpRequestMessageSender source) {
+				ClientHttpRequestFactory requestFactory = source.getRequestFactory();
+				if (!(requestFactory instanceof AbstractClientHttpRequestFactoryWrapper)) {


I haven't looked in detail but I am wondering why you need to resort to reflection for a builder pattern that should "just" do what the regular template can do. Perhaps this feature should be removed in benefit from something more advanced? I don't like the idea to use reflection in production code.

snicoll · 2018-05-31T13:02:06Z

...oot/src/main/java/org/springframework/boot/webservices/client/WebServiceTemplateBuilder.java

+	 * @throws java.lang.IllegalStateException if the underlying source doesn't support a
+	 * connection timeout.
+	 */
+	public WebServiceTemplateBuilder setConnectionTimeout(int connectionTimeout) {


Considering that we may have multiple WebServiceMessageSender, I don't think we should expose this method here. You can provide a configured WebServiceMessageSender, this feels weird to me that all of them are reconfigured behind the scenes.

snicoll · 2018-05-31T13:02:17Z

...oot/src/main/java/org/springframework/boot/webservices/client/WebServiceTemplateBuilder.java

+	 * @throws java.lang.IllegalStateException if the underlying source doesn't support a
+	 * read timeout.
+	 */
+	public WebServiceTemplateBuilder setReadTimeout(int readTimeout) {


This should also go away.

snicoll · 2018-05-31T13:03:23Z

...oot/src/main/java/org/springframework/boot/webservices/client/WebServiceTemplateBuilder.java

+			webServiceTemplate.setMessageFactory(this.messageFactory);
+		}
+
+		if (!CollectionUtils.isEmpty(this.customizers)) {


This should be the last step

Completely agree, this should be the last step, I did this in favor of RestTemplateBuilder algorithm.

public <T extends RestTemplate> T configure(T restTemplate) { configureRequestFactory(restTemplate); if (!CollectionUtils.isEmpty(this.messageConverters)) { restTemplate.setMessageConverters(new ArrayList<>(this.messageConverters)); } if (this.uriTemplateHandler != null) { restTemplate.setUriTemplateHandler(this.uriTemplateHandler); } if (this.errorHandler != null) { restTemplate.setErrorHandler(this.errorHandler); } if (this.rootUri != null) { RootUriTemplateHandler.addTo(restTemplate, this.rootUri); } if (this.basicAuthorization != null) { restTemplate.getInterceptors().add(this.basicAuthorization); } if (!CollectionUtils.isEmpty(this.restTemplateCustomizers)) { for (RestTemplateCustomizer customizer : this.restTemplateCustomizers) { customizer.customize(restTemplate); } } restTemplate.getInterceptors().addAll(this.interceptors); return restTemplate; }

That was weird to me as well...

snicoll · 2018-05-31T13:04:08Z

...oot/src/main/java/org/springframework/boot/webservices/client/WebServiceTemplateBuilder.java

+		}
+
+		if (!CollectionUtils.isEmpty(this.webServiceMessageSenderCustomizers)) {
+			if (!ObjectUtils.isEmpty(webServiceTemplate.getMessageSenders())) {


This looks weird to me. Why do we check if the template has existing messageSenders?

You can easily override a WebServiceTemplate and change this logic.

nosan · 2018-05-31T18:19:20Z

@snicoll Thanks a lot.
With a high probability, you will use setReadTimeout and setConnectionTimeout and I agree that logic of setting up these timeouts is really ugly and I don't like it.
Spring Boot helps people to make their life easier, so why we should force people to set up these timeouts through the WebServiceMessageSender instead of doing this logic by ourselves? We can remove this logic from the WebServiceTemplate and create an utility class WebServiceTemplateCustomizers and have controversial features there.

nosan · 2018-06-01T16:54:22Z

@snicoll But still, If you think the current implementation is within its scope, then I will not interfere.

It would be great to reconsider this method.
public WebServiceTemplateBuilder detectHttpMessageSender(Duration connectionTimeout, Duration readTimeout)

gregturn · 2018-06-01T16:58:12Z

@nosan Another aspect to consider is that you have you wrote a custom BeanPostPostProcessor in any given application, can you apply the other 10% needed to the bean created by Boot and thus not require Boot's code too specific to one problem?

nosan · 2018-06-01T17:14:18Z

@gregturn
It is not needed to have a BeanPostProccesor because we have already had WebServiceTemplateCustomizer and you can use it.

As I mentioned before it would be better to remove aspecific functionality from WebServiceTemplateBuilder

nosan · 2018-06-01T17:48:02Z

@snicoll I have updated my PR and I have removed all the controversial things in my opinion. Please take a look. Thanks in advance.

snicoll · 2018-06-02T07:42:58Z

No, it doesn't. It requires to have ClientHttpRequestFactory with methods setReadTimeout(int) and setConnectTimeout(int) and call them through Reflection.

That's exactly what RestTemplateBuilder is doing.

I've seen you've edited your comment in the meantime as you've probably realized that spring-websocket has a compile dependency on spring-web. So we're good on that front.

We'll continue based on my original polish as I've already spent a significant amount of time on it. I am considering moving the detection of a suitable HTTP-based WebServiceMessageSender to a separate component.

nosan · 2018-06-02T08:46:43Z

@snicoll Thank you.

I've seen you've edited your comment in the meantime as you've probably realized that spring-websocket has a compile dependency on spring-web. So we're good on that front.

spring-websocket? The PR was about spring-ws. I think it was just a typo.

Spring Web Services (Spring-WS) is a product of the Spring community focused on creating document-driven Web services. Spring Web Services aims to facilitate contract-first SOAP service development, allowing for the creation of flexible web services using one of the many ways to manipulate XML payloads. The product is based on Spring itself, which means you can use the Spring concepts such as dependency injection as an integral part of your Web service.

more...

It looks wrong because a) There isn't a concept of a WebMessageServiceSender but a collection so I find it weird that a setter impacts a collection rather than a well targeted object and b) you may add a JMS or another implementation that has no notion of read/connect timeout. What the builder does effectively is detecting an HTTP-based WebServiceMessageSender so the API should be very clear about that.

I have never seen SOAP which is based on JMS :)

nosan · 2018-06-02T08:49:04Z

I've seen you've edited your comment in the meantime as you've probably realized that spring-websocket has a compile dependency on spring-web. So we're good on that front.

Yes, I have checked spring-ws and it has a compile-time dependency on spring-web.
My apologies.

snicoll · 2018-06-03T15:16:00Z

spring-websocket? The PR was about spring-ws. I think it was just a typo.

Sorry about that, my brain mixed the two concepts, here is the dependency section I intended initially.

I have never seen SOAP which is based on JMS :)

The reference documentation describes transports for HTTP, JMS, email and XMPP so we can't have a detectMessageSender method that assumes it's HTTP, even if is the dominant transport. I've given it some more thoughts and a separate builder for the WebServiceMessageSender looks a good compromise to me. I'll spike on that next week.

nosan · 2018-06-03T19:44:10Z

The reference documentation describes transports for HTTP, JMS, email and XMPP so we can't have a detectMessageSender method that assumes it's HTTP, even if is the dominant transport.

I see, thank you, so I was wrong on the beginning. Sorry about that.

I've given it some more thoughts and a separate builder for the WebServiceMessageSender looks a good compromise to me. I'll spike on that next week.

Do you need some help with that?

gregturn · 2018-06-03T20:15:31Z

@snicoll While it's true that SOAP supports all these various protocols, since we're talking Spring Boot which is based on autoconfiguring for the 80% case, perhaps we could assume HTTP (or trigger off some web-based criteria) and supply that.

HTTP is probably 80% or more of what people are doing with Spring WS, let alone the SOAP community.

snicoll · 2018-06-03T20:30:54Z

That wasn’t my point. My point is that we can’t assume message sender is HTTP based as both its contract and the doc states otherwise. See the discussion above for more details.

See spring-projectsgh-12707

See gh-12707

* pr/12707: Polish "Add auto-configuration for WebServiceTemplate" Extract ClientHttpRequestFactory detection to its own class Add auto-configuration for WebServiceTemplate

snicoll · 2018-06-04T10:34:39Z

Closed by c973488

nosan · 2018-06-04T11:08:15Z

@snicoll Thank you a lot for your time.

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 31, 2018

nosan force-pushed the feature/web-service-template branch 2 times, most recently from 1ea6f3c to 2055f9c Compare April 3, 2018 08:41

philwebb added the for: team-attention An issue we'd like other members of the team to review label Apr 3, 2018

nosan force-pushed the feature/web-service-template branch 2 times, most recently from 838aa6f to 77e2258 Compare April 11, 2018 15:31

nosan changed the title ~~WebServiceTemplateBuilder and Jaxb2MarshallerBuilder~~ WebServiceTemplateBuilder Apr 11, 2018

nosan force-pushed the feature/web-service-template branch 2 times, most recently from b2dda3b to 381aaf7 Compare April 12, 2018 10:37

philwebb removed the for: team-attention An issue we'd like other members of the team to review label Apr 18, 2018

philwebb force-pushed the master branch from 86d82f9 to 7b120c1 Compare May 4, 2018 23:38

snicoll self-requested a review May 18, 2018 13:40

nosan force-pushed the feature/web-service-template branch 4 times, most recently from 516af8f to 931bb37 Compare May 29, 2018 09:38

nosan force-pushed the feature/web-service-template branch 2 times, most recently from 063d52c to 86ca383 Compare May 29, 2018 10:11

snicoll pushed a commit to snicoll/spring-boot that referenced this pull request May 31, 2018

Add auto-configuration for WebServiceTemplate

c9a12fd

See spring-projectsgh-12707

snicoll reviewed May 31, 2018

View reviewed changes

snicoll added the status: waiting-for-feedback We need additional information before we can continue label May 31, 2018

nosan force-pushed the feature/web-service-template branch from 3bd6597 to 2dc0ce3 Compare June 1, 2018 17:47

WebServiceTemplateBuilder and WebServiceTemplateAutoConfiguration.

99ba35f

nosan force-pushed the feature/web-service-template branch from 2dc0ce3 to 99ba35f Compare June 1, 2018 22:11

snicoll pushed a commit to snicoll/spring-boot that referenced this pull request Jun 4, 2018

Add auto-configuration for WebServiceTemplate

d0bc313

See spring-projectsgh-12707

snicoll added a commit to snicoll/spring-boot that referenced this pull request Jun 4, 2018

Extract ClientHttpRequestFactory detection to its own class

f77e039

See spring-projectsgh-12707

snicoll added a commit to snicoll/spring-boot that referenced this pull request Jun 4, 2018

Hacking

56fdc53

See spring-projectsgh-12707

snicoll added type: enhancement A general enhancement and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Jun 4, 2018

snicoll self-assigned this Jun 4, 2018

snicoll added this to the 2.1.0.M1 milestone Jun 4, 2018

snicoll pushed a commit that referenced this pull request Jun 4, 2018

Add auto-configuration for WebServiceTemplate

c9df1c5

See gh-12707

snicoll added a commit that referenced this pull request Jun 4, 2018

Extract ClientHttpRequestFactory detection to its own class

ef9c1a8

See gh-12707

snicoll closed this Jun 4, 2018

nosan deleted the feature/web-service-template branch June 4, 2018 11:07

snicoll changed the title ~~WebServiceTemplateBuilder~~ Add auto-configuration for WebServiceTemplate Jun 4, 2018

Add auto-configuration for WebServiceTemplate #12707

Add auto-configuration for WebServiceTemplate #12707

Uh oh!

Conversation

nosan commented Mar 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nosan commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nosan commented Apr 13, 2018

Uh oh!

philwebb commented Apr 13, 2018

Uh oh!

nosan commented Apr 14, 2018

Uh oh!

rfelgent commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nosan commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nosan commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snicoll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nosan May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nosan May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nosan May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nosan May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nosan commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nosan commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gregturn commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

nosan commented Mar 31, 2018 •

edited

Loading

nosan commented Apr 11, 2018 •

edited

Loading

rfelgent commented May 8, 2018 •

edited

Loading

nosan commented May 29, 2018 •

edited

Loading

nosan commented May 29, 2018 •

edited

Loading

nosan May 31, 2018 •

edited

Loading

nosan May 31, 2018 •

edited

Loading

nosan May 31, 2018 •

edited

Loading

nosan May 31, 2018 •

edited

Loading

nosan commented May 31, 2018 •

edited

Loading

nosan commented Jun 1, 2018 •

edited

Loading

gregturn commented Jun 1, 2018 •

edited

Loading

nosan commented Jun 1, 2018 •

edited

Loading

nosan commented Jun 2, 2018 •

edited

Loading