Provide option in RestTemplate to encode slash in URI variables [SPR-12750]

[spring-projects-issues]

Björn Voß opened SPR-12750 and commented

In a uri template like: ```
http://example.com/hotels/{hotel}/pic/{publicpath}/{size}

This is discussed in SPR-8662 with a workaround, but I just want to use UriTemplate and not always use the workaround when I'm not 100% sure that slashes in uri variables are permitted.

---

Affects: 4.0.9, 4.1.5

Attachments:

• SPR-12750_encode_slashes_in_UriTemplate.patch (2.36 kB)
• SPR-12750_respect_slash_rfc6570_in_UriTemplate_and_UriComponentsBuilder.patch (6.79 kB)

Issue Links:

• Support advanced URI Template Syntax [SPR-10505] #15137 Support advanced URI Template Syntax
• UriComponents should parse the path of a String URI into path segments [SPR-10959] #15587 UriComponents should parse the path of a String URI into path segments

[spring-projects-issues]

Björn Voß commented

Simple change with test to have the described behavior

[spring-projects-issues]

Rossen Stoyanchev commented

Here is an example of how to do this. You need to indicate explicitly the path segments vs path case and that's not possible with UriTemplate which is a full string and is essentially treated as a path.

[spring-projects-issues]

Björn Voß commented

Well I thought in terms of easy to use this behavior should be default.
Nearly every dev I talk to about usage of UriTemplate and RestTemplate wasn't aware of this problem.

In the given example it is obvious to have spacial handling for publicpath but

http://myexample.com/document/{externalId}/link/{id} might not. And yes, slashes for externalId is only a problem for springs RestTemplate.

So as a dev I can't just write

public Link loadLinkForDocument(String externalId, long linkId) {
  return restOperations.getForObject("http://myexample.com/document/{externalId}/link/{id}", Link.class, externalId, linkId);
}

So I'll stay with the first thing I do, when I start working in a new team is to change the spring application context config to use a "patched" version of RestTemplate an expose this as bean to service beans.

[spring-projects-issues]

Rossen Stoyanchev commented

What is easy or should be default behavior really depends on your use case. For example you might think of a URI template such as "/a/{b}" where {b} can represent one or more path segments. You might say, who does that, but if we changed the default behavior we'd almost certainly hear from those who do.

At the same time I agree that your case is just as valid and common and shouldn't be difficult to enable. I will re-open this ticket with the idea of providing a property on RestTemplate that would indicate whether you want to treat the path in string URI templates as a "full path" or "path segments".

I know your proposed patch is a little deeper (in UriComponentsBuilder.path) but I'm not sure we could go as far. After all we have both path and pathSegement methods in order to give a clear choice.

[spring-projects-issues]

Rossen Stoyanchev commented

Modified title (was: "Encode slashes in UriTemplate")

[spring-projects-issues]

Rossen Stoyanchev commented

Alternatively we could also do what we've already done for the UrlTag (see #16028). The URI template would have to be:

http://example.com/hotels/{/hotel}/pic/{/publicpath}/{/size}

Note the "/" operator at the start of each URI variable, which comes from the most recent UriTemplate spec, see https://tools.ietf.org/html/rfc6570#section-3.2.6. The advantage of this approach is that we can implement the change in UriComponentsBuilder and in turn it would be also supported wherever UriTemplate is used.

[spring-projects-issues]

Björn Voß commented

Hi Rossen,

Thanks for reopening this issue!

I like both approaches. I talked to some of my colleges and now I would prefer the UrlTag/RFC one you mentioned in your last comment.
If it's useful, I'll provide a patch for this during the weekend.

[spring-projects-issues]

Rossen Stoyanchev commented

Yes I think the "/" operator is cleaner and consistent with both the RFC and what we've done already. Feel free to submit something, thanks!

[spring-projects-issues]

Björn Voß commented

First suggestion for path variables like

/pathpart/{/var1}/part2

NO support for parameter variables like

/path{?id}

or something similar

[spring-projects-issues]

Arjen Poutsma commented

Björn Voß Thanks for the patch, I am reviewing it now.

In the future, could you please follow the Spring Coding guidelines when submitting a patch or pull request? That makes it a lot easier for us. For instance: we use tabs instead of spaces. Thanks!

[spring-projects-issues]

Arjen Poutsma commented

PR at #759
Largely based on the patch (thanks again!), with some minor improvements.

[spring-projects-issues]

Arjen Poutsma commented

As it turned out, the patch did not solve all (corner) cases. For instance, it didn't handle ```
{/..}

```"foo/bar{/baz}qux"
```). I've taken a different approach, and updated the PR accordingly.

[spring-projects-issues]

daniel carter commented

Is there actually a part of RFC6570 that indicates spring current behaviour with slashes is correct? From my reading slashes should be encoded making spring un-compliant.

For instance section 3.2.3, ( where base := "http://example.com/home/")

{base}index = http%3A%2F%2Fexample.com%2Fhome%2Findex
{+base}index = http://example.com/home/index

This shows that if you want the slash in a variable value to not be encoded, the path variable should be prepended with a +
If there is no plus operator, then the / should be encoded.

I understand the concern with supporting legacy behaviour, but perhaps a property somewhere could be used to enable legacy behaviour, and 4.2 onwards could default to RFC6570 behaviour?

I'll open another bug, the {/var1} encoding is working fine in 4.2

[spring-projects-issues]

daniel carter commented

Actually, i take that back about {/var1} encoding working fine in 4.2. It fails to add the preceding / I've opened a bug regarding this and other issues handling / in UriTemplates #17535

You state in your commit: "For example: {/foo} expanded with "bar/baz" with result in "bar%2F""
but it should expand to "/bar%2F"

Likewise the unit test expandMapForRFC6570() you have

new UriTemplate("/hotels/{hotel}/pic/{/publicpath}/size/{scale}");

but for the result you are expecting, the correct template is

new UriTemplate("/hotels/{hotel}/pic{/publicpath}/size/{scale}");

[spring-projects-issues]

daniel carter commented

${/var1} handling is not per RF6570. Should add a / to the variable substitution.

[spring-projects-issues]

Rossen Stoyanchev commented

The discussion related to section 3.2.3 is now under #17535 so I won't reply to that here.

The idea with the current implementation for "{/...}" was that a URI var can still be embedded anywhere, including within a path segment, and the "/" simply implies different encoding rules. So we designed explicitly with that in mind see for example. You're right that the RFC in section 3.2.6 treats the URI var as the full path segment which implies a "/" in front of it even if not present. I'll admit I overlooked this part. I think the spec interpretation is a fine idea. I don't see any reasons not to adopt it and we still have time to change our minds.

The only thing I can think of, aside from the fact it was quite painful to add this support to begin with, is whether this has any implications on @RequestMapping template patterns support backed by AntPathMatcher but that seems to be a different thing altogether. In other words URI template support for building URIs has different, more advanced needs vs URI template support for (server-side) request mapping. So probably no need to expect support for "{/...}" in the case of the latter.

/cc Arjen Poutsma

[spring-projects-issues]

daniel carter commented

You comment regarding @RequestMapping got me thinking, could supporting this on the server side do away with the need to setUrlDecode(false) on the handler mapping when using path variables? You could decode or not according to the template expression operator.

Regarding the "/" simply implies different encoding rules, it's worth nothing that this was not the intention in the RFC, as it states default encoding rules should be used. 'append "/" to the result string and then perform variable expansion, as defined in Section 3.2.1'

[spring-projects-issues]

Rossen Stoyanchev commented

My sense is that urlDecode is more of a global setting but good point that we need to think about that as we adopt more advanced RFC 6570 syntax.

Also good point that if variable expansion (as per 3.2.1) is adopted as part of #17535, then really encoding is controlled with the "+" operator and "/" simply means the URI variable represents a path segment, which mainly implies a leading slash. That said I see nothing wrong with recognizing the presence of a "/" operator and applying path segment encoding rules. The end result is the same. Probably where it becomes obvious we don't support 3.2.1 yet is if you use "/" and "+" at the same time in order to prepend a slash but keep reserved characters.

[spring-projects-issues]

Rossen Stoyanchev commented

Interesting actually that 3.2.6 is mainly concerned with making sure there is slash in front of a "{/var}". There is absolutely no mention whether to insert a slash after if necessary. For example it's clear that "{/who}" expands to "/fred" but what about "{/who}blah"? Should that become "/fred/blah" or "/fredblah"? My guess is that it should be "/fred/blah" but it would be better if it was explicitly called out.

[spring-projects-issues]

Rossen Stoyanchev commented

After some further thoughts and review of RFC 6570 we've decided to use a different approach to solve the original requirement on this ticket. See commit 3e59c2:

This change introduces a strategy for expanding a URI template into a
URI and makes it a property of the RestTemplate and AsyncRestTemplate
so that they can be pre-configured with such a strategy.

The DefaultUriTemplateHandler relies on UriComponentsBuilder internally
and provides functionality equivalent to using the UriTemplate.
A DefaultUriTemplateHandler can also be configured to parse the path
of a URI template into path segments in order to allow expanding URI
variables according to path segment encoding rules.

That means you can configure the RestTemplate as follows:

DefaultUriTemplateHandler uriTemplateHandler = new UriTemplateHandler();
uriTemplateHandler.setParsePath(true);

RestTemplate restTemplate = new RestTemplate();
restTemplate.setUriTemplateHandler(uriTemplateHandler);

[spring-projects-issues]

Rossen Stoyanchev commented

For more on RFC 6570 please follow #15137 (more comments to come shortly).