Add static factory methods to RequestCallback and ResponseExtractor [SPR-8604]

[spring-projects-issues]

Craig opened SPR-8604 and commented

I'm attempting to use RestTemplate.execute() and it takes a RequestCallback instance and a ResponseExtractor instance as parameters. It seems that I should be able to use (via direct instantiation or inheritance) the really useful RestTemplate implementations of these interfaces, but they're all marked private. Can they please be marked public?

---

Affects: 5.0.4

Referenced from: commits 79e809b

3 votes, 6 watchers

[spring-projects-issues]

Shai Yallin commented

Same here. I was trying to overcome the issue in #12671 by subclassing HttpEntityRequestCallback and ended up having to actually duplicate HttpEntityRequestCallback, AcceptHeaderRequestCallback and ResponseEntityResponseExtractor.

[spring-projects-issues]

Rossen Stoyanchev commented

Craig, the AcceptHeaderRequestCallback accomplishes a very specific task that is not customizable (other than the responseType), it is internal to the RestTemplate and is already used in the places where it's needed. Could elaborate on your reasons for this request?

[spring-projects-issues]

Rossen Stoyanchev commented

Craig, Shai Yallin, as mentioned above, these callback classes perform tasks internal to the RestTemplate and are not open for extension. Providing a specific example of what you're trying to customize or do would help.

[spring-projects-issues]

Mauro Molinari commented

Could you please reconsider this?
Either by making HttpEntityRequestCallback and ResponseEntityResponseExtractor public or by making org.springframework.web.client.RestTemplate.httpEntityCallback(Object, Type) and org.springframework.web.client.RestTemplate.responseEntityExtractor(Type) public?

My use case is this: I want to supply a ResponseExtractor but I don't want to re-implement the RequestCallback for my RequestEntity. Or, vice versa, I may need to implement my own RequestCallback but I don't want to reimplement ResponseExtractor to get a ResponseEntity.

Right now, RestTemplate forces you to implement both or neither one. If I want to supply my own ResponseExtractor to support response streaming (see #12015) I am forced to implement RequestCallback as well, which is not trivial. Otherwise I must subclass RestTemplate just to make org.springframework.web.client.RestTemplate.httpEntityCallback(Object) public.

[spring-projects-issues]

Rossen Stoyanchev commented

Arjen Poutsma what do you think about exposing static factory methods on RequestCallback and ResponseExtractor? Those would invoke the protected methods on RestTemplate -- acceptHeaderRequestCallback, httpEntityCallback, httpEntityCallback, and responseEntityExtractor respectively. That would allow overriding only the RequestCallback or ResponseExtractor but not both.

[spring-projects-issues]

Arjen Poutsma commented

Sounds good to me.

[spring-projects-issues]

Rossen Stoyanchev commented

Static factory methods can't work actually, since the inner classes are not static themselves and require a surrounding instance of RestTemplate.

Changing the methods acceptHeaderRequestCallback, httpEntityCallback, etc. from protected to public would make them visible (for code completion) next to all the RestOperations methods. Adding extra variants of execute, to take a RequestCallback or ResponseExtractor only but not both, would result in a lot of new methods.

I propose to make the inner classes public. I see no strong reason to not expose them, and that doesn't have any of the downsides of the other options. Those implementations seem to be of interest, and there are legitimate reasons to use them, e.g. to provide only RequestCallback, or ResponseExtractor but not both.

So the following would become possible:

RequestCallback callback =
		template.new AcceptHeaderRequestCallback(String.class);

RequestCallback callback =
		template.new HttpEntityRequestCallback("body", String.class);

ResponseExtractor<ResponseEntity<String>> extractor =
		template.new ResponseEntityResponseExtractor<>(String.class);

ResponseExtractor<HttpHeaders> extractor =
		new RestTemplate.HeadersExtractor();

Arjen Poutsma?

[spring-projects-issues]

Arjen Poutsma commented

I see two problems with making these classes public as is.

Firstly, these inner classes have been designed for private use only. If we would make them public, we would need to refactor them for extensibility: the larger methods need to split into smaller ones with overridable template methods, proper input sanitation, etc. It's not as simple as changing private to public, we have to write new code as well, and therefore we can also expect new bug reports as a consequence.

Secondly, and this is more an aesthetic issue: I really do not care for public non-static inner classes. Instantiating them with template.new just looks very odd to me. Furthermore, we do not have a single other public non-static inner class in the Spring Framework, so this would definitely be something to discuss with Juergen Hoeller before we proceed.

Personally, I would actually prefer to use public methods to expose them, returning public interfaces like RequestCallback. This does add at least three new methods to RestTemplate}:

• acceptHeaderRequestCallback(Type)
• httpEntityRequestCallback(Object, Type), and
• responseEntityResponseExtractor(Type)
  (I do not think we need to expose HeadersExtractor, as its implementation is trivial. Especially using Java 8 lambdas.)

I do see the concern for adding even visible methods to the RestTemplate, but still believe that this is preferable to having public non-static inner classes.

[spring-projects-issues]

Rossen Stoyanchev commented

Alright, I guess the intent is to provide some way to access those instances, and not to for extension purposes. I'll go with making those methods public. That's close to the static factory methods idea but clearly they cannot be static.