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

Add String based URI template variants to RequestEntity [SPR-17551] #22083

Closed
spring-issuemaster opened this issue Nov 29, 2018 · 10 comments
Closed

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Nov 29, 2018

Ryon Day opened SPR-17551 and commented

The RestTemplateBuilder documentation for the rootUri() method states:

"Set a root URL that should be applied to each request that starts with '/'."

However, the following code does not result in RestTemplate expanding the URI:

UriTemplate template = new UriTemplate("/foo?bar={bar}");

this.restTemplate = restTemplateBuilder
 .rootUri(URI.create("http://foo.bar"))
 .build();

URI uri = template.expand("baz");
RequestEntity<Void> req = RequestEntity.get(uri)
 .accept(MediaType.APPLICATION_JSON)
 .header(accountLinkHeader, accountLinksHeader)
 .build();
String response = restTemplate.exchange(req, String.class);

 

Moreover, the exception received is EXTREMELY ambiguous when this doesn't work. In fact, the Spring exception thrown occludes the initial Apache exception (which states there's no host).


Affects: 5.1.3

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 30, 2018

Rossen Stoyanchev commented

RestTemplateBuilder is a Spring Boot class, so we can't change anything here. As far as the underlying mechanism, it's using the uriTemplateHandler which is documented as "Configure a strategy for expanding URI templates". In other words only the methods that accept a String URI template to be expanded have this handler applied. All the methods that accept a java.net.URI, the URI is considered fully built and used as is. So you might want to create a ticket in Boot to make the Javadoc on rootUri more clear about that and/or to link to the JavaDoc of the underlying mechanism it's using.

I think what could be a reasonable request here is to provide a way to supply a URI template via RequestEntity so that you don't have to expand it externally, and lose the advantage of the applying a baseUrl.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 30, 2018

Rossen Stoyanchev commented

Resolving for now but feel free to comment or reopen if needed.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 30, 2018

Ryon Day commented

Thanks for the quick attention to this and the explanation. It does indeed feel like this is a mismatch between RestTemplateBuilder and the actual RestTemplate

Supplying a UriTemplate in RequestEntity would be super nice here; it would let us use this very nice pattern:

UriTemplate template = new UriTemplate("/foo?bar={bar}");

this.restTemplate = restTemplateBuilder
 .rootUri(URI.create("http://foo.bar"))
 .build();

URI uri = template.expand("baz");
RequestEntity<Void> req = RequestEntity.get(uri)
 .accept(MediaType.APPLICATION_JSON)
 .header(accountLinkHeader, accountLinksHeader)
 .build();
String response = restTemplate.exchange(req, String.class); 

 

which is simply so, so much nicer and clearer than this pattern:

String template = "/foo?bar={bar}";

HttpHeaders headers = new HttpHeaders();
headers.set(HttpHeaders.ACCEPT, MediaType.APPLICATION_JSON_VALUE);
headers.set("Foo", "Bar");

HttpEntity entity = new HttpEntity(headers);

ResponseEntity<String> resp = restTemplate.exchange(template,
    HttpMethod.GET,
    entity,
    String.class,
    baz);

(Not being able to chain .set (or any of the other methods) when using HttpHeaders causes me almost physical pain :D)

 

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 30, 2018

Rossen Stoyanchev commented

Okay I'm re-purposing the ticket to be add String-based URI template method variants to RequestEntity.

@aberkecz
Copy link

@aberkecz aberkecz commented Jan 2, 2020

Hi,

I would love to help out with this issue if it's up for grabs. I spent some time investigating it but got stuck a bit when trying to figure out a nice solution.

So far, I've created an example project to see if the issue is still valid and found out that it is. However, the Javadoc for RestTemplateBuilder#rootUri has been clarified to describe the above behaviour.

Now, I’m a bit confused because I’m not entirely sure what would be a nice way to do the expansion when a RequestEntity is used. (Do I extend the RequestEntity with new public methods expecting a String templateUri? But how do I expand it there when I only have the rootUri set in the RestTemplate instance?)

Would it be helpful if I tried and submitted a PR for this? If so, could you please help me with some guiding on how to get started?

Thanks!

@rstoyanchev
Copy link
Contributor

@rstoyanchev rstoyanchev commented Jan 7, 2020

@aberkecz thanks for considering this and yes this is marked "ideal-for-contribution".

For the implementation, I can suggest something like this:

  1. Add extra static RequestEntity methods (per HTTP method) that accept a String-based URI template.
  2. Add methods in the builder (RequestEntity.HeadersBuilder probably) that accept URI variables as either vararg or a Map.
  3. Add private RequestEntity constructor that accepts java.net.URI and String-based URI template + varargs and store those are nullable fields (it's either-or).
  4. Add overloaded public URI getUrl(UriTemplateHandler) method to RequestEntity which expands the URI template if it has one, or otherwise returns the URI it has.
  5. Update existing getUrl() to expand if necessary using a default instance of DefaultUriBuilderFactory.
  6. Make use of this new method in RestTemplate.
@parviz-93
Copy link
Contributor

@parviz-93 parviz-93 commented Jan 19, 2020

HI.
Are work underway on this task?
Last comment was 12 days ago ... If activity does not go, I would like to take this task

@parviz-93
Copy link
Contributor

@parviz-93 parviz-93 commented Jan 20, 2020

opened Pull Request #24406

@rstoyanchev
Copy link
Contributor

@rstoyanchev rstoyanchev commented Jan 22, 2020

Superseded by #24406.

@rstoyanchev rstoyanchev modified the milestone: 5.3 M1 Jan 22, 2020
@aberkecz
Copy link

@aberkecz aberkecz commented Jan 27, 2020

@rstoyanchev : got my notifications muted and totally missed your reply. Sorry about that. :(

As I can see, there is already a PR open for that so I will look for a new issue and be careful with the notifications next time...

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
4 participants
You can’t perform that action at this time.