#361 - Add support for resolving values in request mappings #602

gregturn · 2017-06-15T15:20:32Z

No description provided.

odrotbohm · 2017-07-27T16:28:52Z

src/main/java/org/springframework/hateoas/mvc/ControllerLinkBuilder.java

 */
 public class ControllerLinkBuilder extends LinkBuilderSupport<ControllerLinkBuilder> {

 	private static final String REQUEST_ATTRIBUTES_MISSING = "Could not find current request via RequestContextHolder. Is this being called from a Spring MVC handler?";
-	private static final CachingAnnotationMappingDiscoverer DISCOVERER = new CachingAnnotationMappingDiscoverer(


What's the reason we move the caching of the templates into the LinkBuilder and not leave it in the MappingDiscoverer. I can see that you only want the raw templates to be cached and not necessarily the values evaluated against the Environment (as the latter might change), but I still think this could be solved at construction. Maybe a builder that you hand in an annotation type, that'd wrap the AMD with the caching one. If you then add an Environment to the builder, it could put the property resolving one in front?

The issue is all the static fields. The solution was to have a setter method such that it would fetch a default static, unless the setter had overridden a field.

Moving things around, I was able to put the caching back inside CachingANnotationMappingDiscoverer, but at that point, I ended up STILL fetching the final route after property resolution before caching. So I don't see how to do this.

gregturn · 2017-07-31T14:49:59Z

I rebased this PR but haven't resolved the issue you mentioned @olivergierke

gregturn · 2017-08-28T17:35:53Z

@olivergierke I found a solution. Working on updating this PR.

gregturn · 2017-08-28T18:17:31Z

@olivergierke I rewrote this so it doesn't move around the CachingAnnotationMappingDiscoverer as before. Instead, CachingAnnotationMappingDiscoverer is able to spot if a custom MappingDiscoverer has been injected and then use that instead.

Original pull-request: #391

gregturn · 2019-02-06T19:46:05Z

I have cleaned up this PR and rebased against latest changes. The idea is to offer a static method through which you can register a Spring PropertyResolver so that Spring can resolve variables. Static is the requirement to hook into link creation.

schauder

Just to minor comments, really about the same issue.
Otherwise I'm fine with this change.

schauder · 2019-02-08T09:52:10Z

src/main/java/org/springframework/hateoas/mvc/ControllerLinkBuilder.java

 		this.addAffordances(findAffordances(invocation, uriComponents));
 	}

+	public static void setDelegateDiscoverer(MappingDiscoverer delegateDiscovererOverride) {


Some JavaDoc here would be helpful, to point out that this disables caching of UriTemplates

schauder · 2019-02-08T09:55:00Z

src/main/java/org/springframework/hateoas/mvc/ControllerLinkBuilder.java

@@ -368,14 +380,72 @@ private static HttpServletRequest getCurrentRequest() {
 	@RequiredArgsConstructor
 	private static class CachingAnnotationMappingDiscoverer implements MappingDiscoverer {


I find the name kind of misleading since the class seems to do two things:

determine the correct delegate to use.

caching UriTemplates

I see various ways to improve that: 2 classes, lengthy name or some JavaDoc.

odrotbohm · 2019-02-11T08:44:07Z

I'm still not convinced we should go down that route. Reconfiguring static properties on calling a setter that is. The main reason is that this is totally unexpected to Spring users, as configuration is always per instance.

I'm in favor of documenting the lack of support for this for static usage of ControllerLinkBuilder and rather apply Greg's suggested changes (finding some means to configure a PropertyResolver) on ControllerLinkBuilderFactory and encourage developers to rather inject that into controllers for non-static usage.

This commit updates URLs to prefer the https protocol. Redirects are not followed to avoid accidentally expanding intentionally shortened URLs (i.e. if using a URL shortener). # Fixed URLs ## Fixed Success These URLs were switched to an https URL with a 2xx status. While the status was successful, your review is still recommended. * http://maven.apache.org/xsd/maven-4.0.0.xsd with 1 occurrences migrated to: https://maven.apache.org/xsd/maven-4.0.0.xsd ([https](https://maven.apache.org/xsd/maven-4.0.0.xsd) result 200). * http://www.apache.org/licenses/LICENSE-2.0 with 4 occurrences migrated to: https://www.apache.org/licenses/LICENSE-2.0 ([https](https://www.apache.org/licenses/LICENSE-2.0) result 200). * http://static.springframework.org/spring/docs/4.1.x/javadoc-api (301) with 1 occurrences migrated to: https://docs.spring.io/spring/docs/4.1.x/javadoc-api ([https](https://static.springframework.org/spring/docs/4.1.x/javadoc-api) result 301). * http://github.com/SpringSource/spring-hateoas with 1 occurrences migrated to: https://github.com/SpringSource/spring-hateoas ([https](https://github.com/SpringSource/spring-hateoas) result 301). * http://www.spring.io with 1 occurrences migrated to: https://www.spring.io ([https](https://www.spring.io) result 301). * http://docs.oracle.com/javase/6/docs/api with 1 occurrences migrated to: https://docs.oracle.com/javase/6/docs/api ([https](https://docs.oracle.com/javase/6/docs/api) result 302). * http://repo.spring.io/libs-snapshot with 2 occurrences migrated to: https://repo.spring.io/libs-snapshot ([https](https://repo.spring.io/libs-snapshot) result 302). # Ignored These URLs were intentionally ignored. * http://maven.apache.org/POM/4.0.0 with 2 occurrences * http://www.w3.org/2001/XMLSchema-instance with 1 occurrences

This commit updates URLs to prefer the https protocol. Redirects are not followed to avoid accidentally expanding intentionally shortened URLs (i.e. if using a URL shortener). # HTTP URLs that Could Not Be Fixed These URLs were unable to be fixed. Please review them to see if they can be manually resolved. * [ ] http://alps.io (200) with 1 occurrences could not be migrated: ([https](https://alps.io) result AnnotatedConnectException). * [ ] http://alps.io/spec/ (200) with 6 occurrences could not be migrated: ([https](https://alps.io/spec/) result AnnotatedConnectException). * [ ] http://amundsen.com/media-types/collection/examples/ (200) with 7 occurrences could not be migrated: ([https](https://amundsen.com/media-types/collection/examples/) result AnnotatedConnectException). * [ ] http://amundsen.com/media-types/collection/format/ (200) with 1 occurrences could not be migrated: ([https](https://amundsen.com/media-types/collection/format/) result AnnotatedConnectException). * [ ] http://stateless.co/hal_specification.html (200) with 2 occurrences could not be migrated: ([https](https://stateless.co/hal_specification.html) result SSLHandshakeException). * [ ] http://www.opensearch.org/Specifications/OpenSearch/1.1 (200) with 1 occurrences could not be migrated: ([https](https://www.opensearch.org/Specifications/OpenSearch/1.1) result SSLHandshakeException). * [ ] http://foo.com/bar (301) with 8 occurrences could not be migrated: ([https](https://foo.com/bar) result SSLHandshakeException). * [ ] http://www.csse.monash.edu.au/~damian/papers/HTML/Plurals.html (302) with 1 occurrences could not be migrated: ([https](https://www.csse.monash.edu.au/~damian/papers/HTML/Plurals.html) result SSLHandshakeException). * [ ] http://alps.io/ext/range (404) with 2 occurrences could not be migrated: ([https](https://alps.io/ext/range) result AnnotatedConnectException). # Fixed URLs ## Fixed But Review Recommended These URLs were fixed, but the https status was not OK. However, the https status was the same as the http request or http redirected to an https URL, so they were migrated. Your review is recommended. * [ ] http://tools.ietf.org/html/draft-kelly-json-hal (301) with 2 occurrences migrated to: https://tools.ietf.org/html/draft-kelly-json-hal ([https](https://tools.ietf.org/html/draft-kelly-json-hal) result ReadTimeoutException). * [ ] http://examples.org/blogs/jdoe (UnknownHostException) with 5 occurrences migrated to: https://examples.org/blogs/jdoe ([https](https://examples.org/blogs/jdoe) result UnknownHostException). * [ ] http://examples.org/blogs/msmith (UnknownHostException) with 3 occurrences migrated to: https://examples.org/blogs/msmith ([https](https://examples.org/blogs/msmith) result UnknownHostException). * [ ] http://examples.org/blogs/rwilliams (UnknownHostException) with 3 occurrences migrated to: https://examples.org/blogs/rwilliams ([https](https://examples.org/blogs/rwilliams) result UnknownHostException). * [ ] http://examples.org/images/jdoe (UnknownHostException) with 5 occurrences migrated to: https://examples.org/images/jdoe ([https](https://examples.org/images/jdoe) result UnknownHostException). * [ ] http://examples.org/images/msmith (UnknownHostException) with 3 occurrences migrated to: https://examples.org/images/msmith ([https](https://examples.org/images/msmith) result UnknownHostException). * [ ] http://examples.org/images/rwilliams (UnknownHostException) with 3 occurrences migrated to: https://examples.org/images/rwilliams ([https](https://examples.org/images/rwilliams) result UnknownHostException). * [ ] http://acme.com/rels/foo-bar (404) with 2 occurrences migrated to: https://acme.com/rels/foo-bar ([https](https://acme.com/rels/foo-bar) result 404). * [ ] http://example.com/custom/deprecated (404) with 1 occurrences migrated to: https://example.com/custom/deprecated ([https](https://example.com/custom/deprecated) result 404). * [ ] http://example.com/customers/deprecated (404) with 2 occurrences migrated to: https://example.com/customers/deprecated ([https](https://example.com/customers/deprecated) result 404). * [ ] http://example.com/rels/ (404) with 2 occurrences migrated to: https://example.com/rels/ ([https](https://example.com/rels/) result 404). * [ ] http://example.com/rels/persons (404) with 1 occurrences migrated to: https://example.com/rels/persons ([https](https://example.com/rels/persons) result 404). * [ ] http://example.org/blogs/wchandry (404) with 2 occurrences migrated to: https://example.org/blogs/wchandry ([https](https://example.org/blogs/wchandry) result 404). * [ ] http://example.org/friends/ (404) with 13 occurrences migrated to: https://example.org/friends/ ([https](https://example.org/friends/) result 404). * [ ] http://example.org/friends/?queries (404) with 2 occurrences migrated to: https://example.org/friends/?queries ([https](https://example.org/friends/?queries) result 404). * [ ] http://example.org/friends/?template (404) with 2 occurrences migrated to: https://example.org/friends/?template ([https](https://example.org/friends/?template) result 404). * [ ] http://example.org/friends/jdoe (404) with 4 occurrences migrated to: https://example.org/friends/jdoe ([https](https://example.org/friends/jdoe) result 404). * [ ] http://example.org/friends/msmith (404) with 2 occurrences migrated to: https://example.org/friends/msmith ([https](https://example.org/friends/msmith) result 404). * [ ] http://example.org/friends/rss (404) with 5 occurrences migrated to: https://example.org/friends/rss ([https](https://example.org/friends/rss) result 404). * [ ] http://example.org/friends/rwilliams (404) with 2 occurrences migrated to: https://example.org/friends/rwilliams ([https](https://example.org/friends/rwilliams) result 404). * [ ] http://example.org/friends/search (404) with 2 occurrences migrated to: https://example.org/friends/search ([https](https://example.org/friends/search) result 404). * [ ] http://example.org/images/wchandry (404) with 2 occurrences migrated to: https://example.org/images/wchandry ([https](https://example.org/images/wchandry) result 404). * [ ] http://example.org/rels/todo (404) with 3 occurrences migrated to: https://example.org/rels/todo ([https](https://example.org/rels/todo) result 404). * [ ] http://example.org/samples/full/doc.html (404) with 3 occurrences migrated to: https://example.org/samples/full/doc.html ([https](https://example.org/samples/full/doc.html) result 404). * [ ] http://pubsubhubbub.googlecode.com (404) with 1 occurrences migrated to: https://pubsubhubbub.googlecode.com ([https](https://pubsubhubbub.googlecode.com) result 404). * [ ] http://www.example.com/rels/ (404) with 1 occurrences migrated to: https://www.example.com/rels/ ([https](https://www.example.com/rels/) result 404). ## Fixed Success These URLs were switched to an https URL with a 2xx status. While the status was successful, your review is still recommended. * [ ] http://docs.spring.io/spring-hateoas/docs/current/reference/html/ with 1 occurrences migrated to: https://docs.spring.io/spring-hateoas/docs/current/reference/html/ ([https](https://docs.spring.io/spring-hateoas/docs/current/reference/html/) result 200). * [ ] http://docs.spring.io/spring-hateoas/docs/current/reference/pdf/spring-hateoas-reference.pdf with 1 occurrences migrated to: https://docs.spring.io/spring-hateoas/docs/current/reference/pdf/spring-hateoas-reference.pdf ([https](https://docs.spring.io/spring-hateoas/docs/current/reference/pdf/spring-hateoas-reference.pdf) result 200). * [ ] http://en.wikipedia.org/wiki/HATEOAS with 2 occurrences migrated to: https://en.wikipedia.org/wiki/HATEOAS ([https](https://en.wikipedia.org/wiki/HATEOAS) result 200). * [ ] http://example.com?name=foo with 1 occurrences migrated to: https://example.com?name=foo ([https](https://example.com?name=foo) result 200). * [ ] http://mamund.site44.com/misc/hal-forms/ with 1 occurrences migrated to: https://mamund.site44.com/misc/hal-forms/ ([https](https://mamund.site44.com/misc/hal-forms/) result 200). * [ ] http://projects.spring.io/spring-hateoas/ with 2 occurrences migrated to: https://projects.spring.io/spring-hateoas/ ([https](https://projects.spring.io/spring-hateoas/) result 200). * [ ] http://uberhypermedia.org/ (302) with 1 occurrences migrated to: https://rawgit.com/uber-hypermedia/specification/master/uber-hypermedia.html ([https](https://uberhypermedia.org/) result 200). * [ ] http://tools.ietf.org/html/draft-kelly-json-hal-05 with 1 occurrences migrated to: https://tools.ietf.org/html/draft-kelly-json-hal-05 ([https](https://tools.ietf.org/html/draft-kelly-json-hal-05) result 200). * [ ] http://tools.ietf.org/html/rfc6570 with 1 occurrences migrated to: https://tools.ietf.org/html/rfc6570 ([https](https://tools.ietf.org/html/rfc6570) result 200). * [ ] http://tools.ietf.org/html/rfc7239 with 1 occurrences migrated to: https://tools.ietf.org/html/rfc7239 ([https](https://tools.ietf.org/html/rfc7239) result 200). * [ ] http://www.example.com with 2 occurrences migrated to: https://www.example.com ([https](https://www.example.com) result 200). * [ ] http://www.hixie.ch/specs/pingback/pingback with 1 occurrences migrated to: https://www.hixie.ch/specs/pingback/pingback ([https](https://www.hixie.ch/specs/pingback/pingback) result 200). * [ ] http://www.iana.org/assignments/link-relations/link-relations.xhtml with 2 occurrences migrated to: https://www.iana.org/assignments/link-relations/link-relations.xhtml ([https](https://www.iana.org/assignments/link-relations/link-relations.xhtml) result 200). * [ ] http://www.w3.org/TR/2011/WD-html5-20110113/links.html with 1 occurrences migrated to: https://www.w3.org/TR/2011/WD-html5-20110113/links.html ([https](https://www.w3.org/TR/2011/WD-html5-20110113/links.html) result 200). * [ ] http://www.w3.org/TR/html5/links.html with 11 occurrences migrated to: https://www.w3.org/TR/html5/links.html ([https](https://www.w3.org/TR/html5/links.html) result 200). * [ ] http://www.w3.org/TR/powder-dr/ with 1 occurrences migrated to: https://www.w3.org/TR/powder-dr/ ([https](https://www.w3.org/TR/powder-dr/) result 200). * [ ] http://www.w3.org/TR/preload/ with 1 occurrences migrated to: https://www.w3.org/TR/preload/ ([https](https://www.w3.org/TR/preload/) result 200). * [ ] http://www.w3.org/TR/resource-hints/ with 1 occurrences migrated to: https://www.w3.org/TR/resource-hints/ ([https](https://www.w3.org/TR/resource-hints/) result 200). * [ ] http://www.w3.org/TR/webmention/ with 1 occurrences migrated to: https://www.w3.org/TR/webmention/ ([https](https://www.w3.org/TR/webmention/) result 200). * [ ] http://amazon.com with 4 occurrences migrated to: https://amazon.com ([https](https://amazon.com) result 301). * [ ] http://contributor-covenant.org with 1 occurrences migrated to: https://contributor-covenant.org ([https](https://contributor-covenant.org) result 301). * [ ] http://contributor-covenant.org/version/1/3/0/ with 1 occurrences migrated to: https://contributor-covenant.org/version/1/3/0/ ([https](https://contributor-covenant.org/version/1/3/0/) result 301). * [ ] http://www.w3.org/TR/1999/REC-html401-19991224 with 10 occurrences migrated to: https://www.w3.org/TR/1999/REC-html401-19991224 ([https](https://www.w3.org/TR/1999/REC-html401-19991224) result 301). * [ ] http://www.w3.org/TR/curie with 1 occurrences migrated to: https://www.w3.org/TR/curie ([https](https://www.w3.org/TR/curie) result 301). * [ ] http://tools.ietf.org/html/rfc5988=section-4 with 1 occurrences migrated to: https://tools.ietf.org/html/rfc5988=section-4 ([https](https://tools.ietf.org/html/rfc5988=section-4) result 302). * [ ] http://www.springsource.org/download with 1 occurrences migrated to: https://www.springsource.org/download ([https](https://www.springsource.org/download) result 302). # Ignored These URLs were intentionally ignored. * http://barfoo:8888 with 1 occurrences * http://foobar with 1 occurrences * http://foobar:8088 with 1 occurrences * http://foobarhost/ with 1 occurrences * http://foobarhost:9090/ with 1 occurrences * http://localhost with 7 occurrences * http://localhost/ with 1 occurrences * http://localhost/customers/15 with 1 occurrences * http://localhost/employees with 33 occurrences * http://localhost/employees/0 with 20 occurrences * http://localhost/employees/1 with 8 occurrences * http://localhost/employees/2 with 15 occurrences * http://localhost/sample/1/foo with 1 occurrences * http://localhost/sample/2/bar with 1 occurrences * http://localhost/something/bar/foo with 1 occurrences * http://localhost:8080/api/ with 1 occurrences * http://localhost:8080/foo with 2 occurrences * http://localhost:8080/my/custom/location with 2 occurrences * http://localhost:8080/rels with 1 occurrences * http://localhost:8080/rels/ with 6 occurrences * http://localhost:8080/something with 4 occurrences * http://localhost:8080/test?page=0&filter=foo,bar with 2 occurrences * http://localhost:8080/your-app with 1 occurrences * http://localhost:8080/your-app/people with 1 occurrences * http://myhost/people with 3 occurrences * http://myhost/person/1 with 1 occurrences * http://myhost/person/1/orders with 1 occurrences * http://proxy1:1443 with 1 occurrences * http://somethingDifferent with 1 occurrences * http://www.w3.org/2005/Atom with 1 occurrences

This commit updates URLs to prefer the https protocol. Redirects are not followed to avoid accidentally expanding intentionally shortened URLs (i.e. if using a URL shortener). # Fixed URLs ## Fixed Success These URLs were switched to an https URL with a 2xx status. While the status was successful, your review is still recommended. * [ ] http://www.springframework.org/schema/beans/spring-beans.xsd with 1 occurrences migrated to: https://www.springframework.org/schema/beans/spring-beans.xsd ([https](https://www.springframework.org/schema/beans/spring-beans.xsd) result 200). * [ ] http://www.springframework.org/schema/context/spring-context.xsd with 1 occurrences migrated to: https://www.springframework.org/schema/context/spring-context.xsd ([https](https://www.springframework.org/schema/context/spring-context.xsd) result 200). # Ignored These URLs were intentionally ignored. * http://maven.apache.org/POM/4.0.0 with 2 occurrences * http://www.springframework.org/schema/beans with 2 occurrences * http://www.springframework.org/schema/context with 2 occurrences * http://www.w3.org/2001/XMLSchema-instance with 2 occurrences * http://www.w3.org/2005/Atom with 1 occurrences

This commit updates URLs to prefer the https protocol. Redirects are not followed to avoid accidentally expanding intentionally shortened URLs (i.e. if using a URL shortener). # Fixed URLs ## Fixed Success These URLs were switched to an https URL with a 2xx status. While the status was successful, your review is still recommended. * [ ] http://www.apache.org/licenses/ with 2 occurrences migrated to: https://www.apache.org/licenses/ ([https](https://www.apache.org/licenses/) result 200). * [ ] http://www.apache.org/licenses/LICENSE-2.0 with 221 occurrences migrated to: https://www.apache.org/licenses/LICENSE-2.0 ([https](https://www.apache.org/licenses/LICENSE-2.0) result 200).

gregturn · 2020-07-27T16:06:04Z

Resolved via c5f4bfa and #1328.

gregturn added the process: in progress label Jun 15, 2017

gregturn force-pushed the feature/resolve-routes branch 6 times, most recently from 6bd2d74 to 1c6b6ee Compare June 16, 2017 19:24

nealeu mentioned this pull request Jun 26, 2017

Cannot convert PathVariable Map<String,String> when building link #605

Open

odrotbohm requested changes Jul 27, 2017

View reviewed changes

gregturn force-pushed the feature/resolve-routes branch from 1c6b6ee to 498d009 Compare July 31, 2017 14:49

gregturn force-pushed the feature/resolve-routes branch from 498d009 to 518c1d4 Compare August 28, 2017 18:09

gregturn mentioned this pull request Feb 6, 2018

Spring property resolved in @RequestMapping but not in Hateos link #696

Closed

gregturn force-pushed the feature/resolve-routes branch 3 times, most recently from 5798278 to 5543a06 Compare February 6, 2019 17:45

#361 - Add support for resolving values in request mappings.

0852752

Original pull-request: #391

gregturn force-pushed the feature/resolve-routes branch from 5543a06 to 0852752 Compare February 6, 2019 17:47

gregturn added type: enhancement in: infrastructure Build infrastructure and dependency upgrades labels Feb 6, 2019

gregturn requested review from odrotbohm and schauder February 6, 2019 19:43

schauder approved these changes Feb 8, 2019

View reviewed changes

Spring Operator added 3 commits March 18, 2019 17:57

gregturn closed this Jul 27, 2020

gregturn deleted the feature/resolve-routes branch July 27, 2020 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

#361 - Add support for resolving values in request mappings #602

#361 - Add support for resolving values in request mappings #602

Uh oh!

gregturn commented Jun 15, 2017

Uh oh!

odrotbohm Jul 27, 2017

Uh oh!

gregturn Jul 28, 2017

Uh oh!

gregturn commented Jul 31, 2017

Uh oh!

gregturn commented Aug 28, 2017

Uh oh!

gregturn commented Aug 28, 2017

Uh oh!

gregturn commented Feb 6, 2019

Uh oh!

schauder left a comment

Uh oh!

schauder Feb 8, 2019

Uh oh!

schauder Feb 8, 2019

Uh oh!

odrotbohm commented Feb 11, 2019

Uh oh!

gregturn commented Jul 27, 2020

Uh oh!

Uh oh!

		@@ -368,14 +380,72 @@ private static HttpServletRequest getCurrentRequest() {
		@RequiredArgsConstructor
		private static class CachingAnnotationMappingDiscoverer implements MappingDiscoverer {

#361 - Add support for resolving values in request mappings #602

#361 - Add support for resolving values in request mappings #602

Uh oh!

Conversation

gregturn commented Jun 15, 2017

Uh oh!

odrotbohm Jul 27, 2017

Choose a reason for hiding this comment

Uh oh!

gregturn Jul 28, 2017

Choose a reason for hiding this comment

Uh oh!

gregturn commented Jul 31, 2017

Uh oh!

gregturn commented Aug 28, 2017

Uh oh!

gregturn commented Aug 28, 2017

Uh oh!

gregturn commented Feb 6, 2019

Uh oh!

schauder left a comment

Choose a reason for hiding this comment

Uh oh!

schauder Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

schauder Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

odrotbohm commented Feb 11, 2019

Uh oh!

gregturn commented Jul 27, 2020

Uh oh!

Uh oh!