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

Update Javadoc on RestTemplate#setUriTemplateHandler [SPR-16419] #20965

Closed
spring-projects-issues opened this issue Jan 25, 2018 · 6 comments
Closed
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Jan 25, 2018

Sachin Lad opened SPR-16419 and commented

It looks like with Spring v5, DefaultUriBuilderFactory does not expand correctly if uriTemplate contains "protocol + hostname" as templated. This UriBuilder is used in RestTemplate to expand the templatedUris. Such templatedUri works well with Spring 1.5.x.

Looks like DefaultUriBuilderFactory is newly introduced in Spring v5 so may be this is not bug but the functionality has changed? But this breaks bunch of test code so I am considering this as regression.

Here is the unit test to reproduce it -

// Some comments here
@Test
    public void testUrlBuilder() throws URISyntaxException {
        String uriTemplate = "{baseUrl}/info";
        String[] vars = new String[]{"http://localhost:8085"};

        DefaultUriBuilderFactory factory = new DefaultUriBuilderFactory();

        URI uri = factory.expand(uriTemplate, vars);

        assertEquals(new URI("http://localhost:8085/info"), uri);
    }

In Spring 4.x it looks like the uriTemplate is expanded using DefaultUriTemplateHandler which works well -

    @Test
    public void testUrlBuilder() throws URISyntaxException {
        String uriTemplate = "{baseUrl}/info";
        String[] vars = new String[]{"http://localhost:8085"};

        DefaultUriTemplateHandler handler = new DefaultUriTemplateHandler();
        URI uri = handler.expand(uriTemplate, vars);

        assertEquals(new URI("http://localhost:8085/info"), uri);
    }

Affects: 5.0.3

Referenced from: commits 4f28c28

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 25, 2018

Rossen Stoyanchev commented

In Spring 4.x it looks like the uriTemplate is expanded using DefaultUriBuilderFactory which works well

DefaultUriBuilderFactory is a 5.0 class. Did you mean DefaultUriTemplateHandler?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 25, 2018

Sachin Lad commented

Sorry, I meant DefaultUriTemplateHandler. I have updated my comment above

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 26, 2018

Rossen Stoyanchev commented

Okay thanks.

The problem is that at the time of parsing, the URI template looks like a path, so there is no way to parse it correctly. DefaultUriTempateHandler appears to work but that's by accident and it wouldn't always work for example if you changed some properties on that class. You can put a breakpoint just before it creates the URI you'll see that UriComponents has no scheme, host, and port, but only a path.

The reason for the difference in behavior is because in DefaultUriBuilderFactory we proactively parse the path into path segments which then means we apply path segment encoding (not path). So if you initialized the factory with factory.setParsePath(false) you will get the exact same behavior.

But again the URI template you're providing cannot be parsed correctly so you need to change it to something that shows the structure. See similar comment provided here.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 27, 2018

Sachin Lad commented

Yeah, I was kind of expecting this to be deliberate change. Not sure if there is good place to document such behavior change but it would be helpful for developers.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 29, 2018

Rossen Stoyanchev commented

Hm good point, on RestTemplate#setUriTemplateHandler. I presume that's how you came across this?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 29, 2018

Rossen Stoyanchev commented

So the goal would be to update the Javadoc of RestTemplate#setUriTemplateHandler to discuss the deprecation of DefaultUritTemplateHandler in favor of DefaultUriBuilderFactory and the difference between the two in the default setting of parsePath. I suppose DefaultUritTemplateHandler itself should be updated as well.

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