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.

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

RestTemplate: Support default urlVariables at template level [SPR-14147] #18719

Closed
spring-issuemaster opened this issue Apr 11, 2016 · 11 comments
Closed
Assignees
Milestone

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Apr 11, 2016

Stefan Kühnel opened SPR-14147 and commented

[RestTemplate](https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/web/client/RestTemplate.java) allows for urlVariables in URIs like

https://{host}:{port}/v42/customers/{id}

The values for those urlVariables are provided at the moment the request is made.

The "problem" with this is that the lifetime/scope of the urlVariables is often different. In the above example, the scope for host and port is typically the lifetime of the RestTemplate while the scope of id is one request.

It would therefore be good to have the possibility to add the values for some urlVariables at the level of the RestTemplate. This could for example look like the following:

public class Customers
{
    private String HOST_PARAM = "host";
    private String PORT_PARAM = "port";
    private String ID_PARAM = "id";
    private String URI_TEMPLATE = "https://{host}:{port}/v42/customers/{id}";

    @value("${host:api.example.com")
    private String host;
    @value("${port:443")
    private int port;

    private RestTemplate template = new RestTemplate();

    public Customer()
    {
         // suggested new functionality
         template.addVariable(HOST_PARAM, host);
         template.addVariable(PORT_PARAM, port);
    }

    public Customer getCustomer(long id)
    {
        Map<String,Object> urlVariables = new HashMap<>();
        urlVariables.add(ID_PARAM, id);
 
	Customer customer = template.getForObject(URI_TEMPLATE, Customer.class, urlVariables);

        return customer;
    }
}

With #17627 there was a property baseUrl added to DefaultUriTemplateHandler which provides somewhat similar capabilities, but the above suggestion is more general and uniform.


Affects: 4.2.5

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 13, 2016

Rossen Stoyanchev commented

Indeed we could add a Map with default URI variables in DefaultUriTemplateHandler.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 15, 2016

Stefan Kühnel commented

Thanks for considering the feature.

One point: My feeling - seeing that the request methods at the level RestTemplate deal with the urlVariables - is that the defaults Map should be at the level RestTemplate as well. Other template handlers besides DefaultUriTemplateHandler could/would benefit from such default variables as well, IMHO.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 22, 2016

Rossen Stoyanchev commented

Alright this is in now, as soon as the snapshot build is done. Feel free to give it a try in case of any surprises.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 25, 2016

Stefan Kühnel commented

Thanks! I haven't had the time to test it but I reviewed the commit and looks good to me.

I see you've added the feature to DefaultUriTemplateHandler. With a change like the following to RestTemplate (and a similar one to AsynRestTemplate) it could work for all UriTemplateHandlers and you could get rid of the coupling to DefaultUriTemplateHandler.

	@Override
	public <T> T execute(String url, HttpMethod method, RequestCallback requestCallback,
			ResponseExtractor<T> responseExtractor, Map<String, ?> urlVariables) throws RestClientException {

+		// Create a new map
+		Map<String, Object> variablesToUse = new HashMap<String, Object>();
+		variablesToUse.putAll(getDefaultUriVariables());
+		variablesToUse.putAll(urlVariables);
+
-		URI expanded = getUriTemplateHandler().expand(url, urlVariables);
+		URI expanded = getUriTemplateHandler().expand(url, variablesToUse);
		return doExecute(expanded, method, requestCallback, responseExtractor);
	}
@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 25, 2016

Rossen Stoyanchev commented

Good point that we may want to vary separately this from how the template is expanded. Let me have another look.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 25, 2016

Stefan Kühnel commented

Thanks for considering the change. Actually I just though a little bit more of AsyncRestTemplate and if the method AsyncRestTemplate.setDefaultUriVariables() delegates to this.syncTemplate.setDefaultUriVariables() it should "just work", there should be no need to adapt AsyncRestTemplate.execute() any more, since RestTemplate would already provide the functionality.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 25, 2016

Rossen Stoyanchev commented

Right, that would work. What I don't like is also having this functionality in DefaultUriTemplateHandler where it doesn't seem needed any more.

What I thought about is an abstract base class (e.g. UriTemplateHandlerSupport) that exposes the generic features such as base URL and default URI variable values and leaves it to sub-classes to vary by the actual expand and encode mechanism. It would still mean downcasting in RestTemplate but at least the expectation that all implementations extend UriTemplateHandler becomes actually feasible (e.g. an alternative implementation based on an 3rd URI template library).

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 26, 2016

Stefan Kühnel commented

Agreed, if the change to RestTemplate would be made all changes to DefaultUriTemplateHandler could be removed completely.

Having an abstract base class would be an option as well. But independently from reduced but still existing coupling in this case I think one can argue both ways where the default URI variables belong:

  • Is it a feature of the clients of the handler (that is RestTemplate, AsyncRestTemplate) or of the handler (e.g. DefaultUriTemplateHandler) to have defaults variables?
    • The URI template handlers responsibility in my view is primarily the "mechanics" of template expansion. It understands the details of variables and expansion mechanisms (name based vs. positon based etc.)
    • The clients on the other hand "know"/"understand" the semantics of the REST-Api and the URI.

Overall in my opinion the clients are the "better" place to know which variables are default and which are individual to a request and how to best combine the two. My 0.05$ worth. :-)

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 26, 2016

Rossen Stoyanchev commented

I've gone ahead with the abstract base class idea. There is nothing wrong with having such behavior delegated to UriTemplateHandler. It's a single place where the mechanism originates and can be traced to. The setDefaultUriVariables in RestTemplate is merely a convenient shortcut. Now that the feature comes from AbtractUriTemplateHandler, it's reasonable to expect it will work with any implementation. If for some reason a UriTemplateHandler does not extend the same base class, it just means the shortcut cannot be used. You'd have to then work with whatever the UriTemplateHandler supports. That's a simple arrangement and in practice I don't expect it will ever be an issue.

Keep in mind also there are a number of other potential candidates for this including WebSocketClient, SockJsClient, MockMvcRequestBuilders, and a new WebClient for Spring 5. As that number multiplies, encapsulation becomes more attractive.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 27, 2016

Stefan Kühnel commented

Okay, with those other (potential) users of the UriTemplateHandler i can understand your preference for implementing it there.

One might think of adding the methods setDefaultUriVariables and/or setBaseUrl to the interface UriTemplateHandler add some point in the future where one has to do a non backward compatible change anyway, but until then the abstract base class is a good alternative.

Regarding the pull request:

  • Is it intentional that DefaultUriTemplateHandler broadens the access modifier from protected to public for the two expandInternal methods?
+	public URI expandInternal(String uriTemplate, Map<String, ?> uriVariables) {
  • A minor point: at DefaultUriTemplateHandler.createUri() I think the line
+			return new URI(uriComponents.toUriString());
  • could be simplified to
+			return uriComponents.toUri();
  • At AbstractUriTemplateHandler.insertBaseUrl one could think about using URI.resolve() instead of string concatenation:
-				url = new URI(getBaseUrl() + url.toString());
+				url = URI.create(getBaseUrl()).resolve(url);

Sorry for being such a nitpicker. ;-)

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 27, 2016

Rossen Stoyanchev commented

I fixed the modifier on the expandInternal methods. Thanks.

When creating the URI we can't call uriComponents.toUri() because in the case of strict encoding we have manually encoded the URI variables and calling toUri would double encoded. Just run the tests for the class and you will see the failures.

For insertBaseUrl using the resolve is not the same because we want a baseUrl that supports a partial path as well. Again you can run the tests to see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.