From 67aa51dbead0c44609a88a6a80f70d70ae897bc1 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Wed, 23 Jul 2014 15:53:55 +0200 Subject: [PATCH] #217 - Improve UriTempalte's handling of TemplateVariables. TempalteVariables now concatenates all request parameter styles correctly. UriTemplate uses continued style of request parameters if the base URI already contains request parameters. --- .../hateoas/TemplateVariable.java | 31 +++++++++ .../hateoas/TemplateVariables.java | 33 ++++++++- .../springframework/hateoas/UriTemplate.java | 28 +++++++- .../hateoas/TemplateVariablesUnitTest.java | 67 +++++++++++++++---- .../hateoas/UriTemplateUnitTest.java | 25 +++++++ 5 files changed, 165 insertions(+), 19 deletions(-) diff --git a/src/main/java/org/springframework/hateoas/TemplateVariable.java b/src/main/java/org/springframework/hateoas/TemplateVariable.java index 2be80edc..d90463bb 100644 --- a/src/main/java/org/springframework/hateoas/TemplateVariable.java +++ b/src/main/java/org/springframework/hateoas/TemplateVariable.java @@ -15,6 +15,8 @@ */ package org.springframework.hateoas; +import static org.springframework.hateoas.TemplateVariable.VariableType.*; + import java.io.Serializable; import java.util.Arrays; import java.util.List; @@ -119,6 +121,35 @@ public final class TemplateVariable implements Serializable { return this.type.canBeCombinedWith(variable.type); } + /** + * Returns whether the given {@link TemplateVariable} is logically equivalent to the given one. This considers request + * parameter variables equivalent independently from whether they're continued or not. + * + * @param variable + * @return + */ + boolean isEquivalent(TemplateVariable variable) { + return this.name.equals(variable.name) && isCombinable(variable); + } + + /** + * Returns whether the current {@link TemplateVariable} is representing a request parameter. + * + * @return + */ + boolean isRequestParameterVariable() { + return type.equals(REQUEST_PARAM) || type.equals(REQUEST_PARAM_CONTINUED); + } + + /** + * Returns whether the variable is a fragement one. + * + * @return + */ + boolean isFragment() { + return type.equals(FRAGMENT); + } + /* * (non-Javadoc) * @see java.lang.Object#toString() diff --git a/src/main/java/org/springframework/hateoas/TemplateVariables.java b/src/main/java/org/springframework/hateoas/TemplateVariables.java index c8f0713d..205eb092 100644 --- a/src/main/java/org/springframework/hateoas/TemplateVariables.java +++ b/src/main/java/org/springframework/hateoas/TemplateVariables.java @@ -15,6 +15,8 @@ */ package org.springframework.hateoas; +import static org.springframework.hateoas.TemplateVariable.VariableType.*; + import java.io.Serializable; import java.util.ArrayList; import java.util.Arrays; @@ -23,6 +25,7 @@ import java.util.Collections; import java.util.Iterator; import java.util.List; +import org.springframework.hateoas.TemplateVariable.VariableType; import org.springframework.util.Assert; /** @@ -79,7 +82,7 @@ public final class TemplateVariables implements Iterable, Seri result.addAll(this.variables); for (TemplateVariable variable : variables) { - if (!result.contains(variable)) { + if (!containsEquivalentFor(variable)) { result.add(variable); } } @@ -106,6 +109,17 @@ public final class TemplateVariables implements Iterable, Seri return this.variables; } + private boolean containsEquivalentFor(TemplateVariable candidate) { + + for (TemplateVariable variable : this.variables) { + if (variable.isEquivalent(candidate)) { + return true; + } + } + + return false; + } + /* * (non-Javadoc) * @see java.lang.Iterable#iterator() @@ -121,6 +135,16 @@ public final class TemplateVariables implements Iterable, Seri */ @Override public String toString() { + return toString(false); + } + + /** + * Returns the string representation of the template but forcing a continued style of expressing request parameters. + * + * @param appended + * @return + */ + String toString(boolean appended) { if (variables.isEmpty()) { return ""; @@ -131,10 +155,13 @@ public final class TemplateVariables implements Iterable, Seri for (TemplateVariable variable : variables) { + VariableType type = variable.getType(); + type = appended && type.equals(REQUEST_PARAM) ? REQUEST_PARAM_CONTINUED : type; + if (previous == null) { - builder.append("{").append(variable.getType().toString()); + builder.append("{").append(type.toString()); } else if (!previous.isCombinable(variable)) { - builder.append("}{").append(variable.getType().toString()); + builder.append("}{").append(type.toString()); } else { builder.append(","); } diff --git a/src/main/java/org/springframework/hateoas/UriTemplate.java b/src/main/java/org/springframework/hateoas/UriTemplate.java index c96d4dd7..5fd93037 100644 --- a/src/main/java/org/springframework/hateoas/UriTemplate.java +++ b/src/main/java/org/springframework/hateoas/UriTemplate.java @@ -28,6 +28,7 @@ import java.util.regex.Pattern; import org.springframework.hateoas.TemplateVariable.VariableType; import org.springframework.util.Assert; import org.springframework.util.StringUtils; +import org.springframework.web.util.UriComponents; import org.springframework.web.util.UriComponentsBuilder; /** @@ -106,7 +107,26 @@ public class UriTemplate implements Iterable, Serializable { return this; } - return new UriTemplate(baseUri, this.variables.concat(variables)); + UriComponents components = UriComponentsBuilder.fromUriString(baseUri).build(); + List result = new ArrayList(); + + for (TemplateVariable variable : variables) { + + boolean isRequestParam = variable.isRequestParameterVariable(); + boolean alreadyPresent = components.getQueryParams().containsKey(variable.getName()); + + if (isRequestParam && alreadyPresent) { + continue; + } + + if (variable.isFragment() && StringUtils.hasText(components.getFragment())) { + continue; + } + + result.add(variable); + } + + return new UriTemplate(baseUri, this.variables.concat(result)); } /** @@ -212,7 +232,11 @@ public class UriTemplate implements Iterable, Serializable { */ @Override public String toString() { - return baseUri + getOptionalVariables().toString(); + + UriComponents components = UriComponentsBuilder.fromUriString(baseUri).build(); + boolean hasQueryParameters = !components.getQueryParams().isEmpty(); + + return baseUri + getOptionalVariables().toString(hasQueryParameters); } private TemplateVariables getOptionalVariables() { diff --git a/src/test/java/org/springframework/hateoas/TemplateVariablesUnitTest.java b/src/test/java/org/springframework/hateoas/TemplateVariablesUnitTest.java index eeea5da3..040324f2 100644 --- a/src/test/java/org/springframework/hateoas/TemplateVariablesUnitTest.java +++ b/src/test/java/org/springframework/hateoas/TemplateVariablesUnitTest.java @@ -17,6 +17,7 @@ package org.springframework.hateoas; import static org.hamcrest.Matchers.*; import static org.junit.Assert.*; +import static org.springframework.hateoas.TemplateVariable.VariableType.*; import java.util.List; @@ -44,7 +45,7 @@ public class TemplateVariablesUnitTest { @Test public void rendersSingleVariableCorrectly() { - TemplateVariables variables = new TemplateVariables(new TemplateVariable("foo", VariableType.SEGMENT)); + TemplateVariables variables = new TemplateVariables(new TemplateVariable("foo", SEGMENT)); assertThat(variables.toString(), is("{/foo}")); } @@ -54,8 +55,8 @@ public class TemplateVariablesUnitTest { @Test public void combinesMultipleVariablesOfTheSameType() { - TemplateVariable first = new TemplateVariable("foo", VariableType.REQUEST_PARAM); - TemplateVariable second = new TemplateVariable("bar", VariableType.REQUEST_PARAM); + TemplateVariable first = new TemplateVariable("foo", REQUEST_PARAM); + TemplateVariable second = new TemplateVariable("bar", REQUEST_PARAM); TemplateVariables variables = new TemplateVariables(first, second); @@ -68,8 +69,8 @@ public class TemplateVariablesUnitTest { @Test public void combinesMultipleVariablesOfTheDifferentType() { - TemplateVariable first = new TemplateVariable("foo", VariableType.SEGMENT); - TemplateVariable second = new TemplateVariable("bar", VariableType.REQUEST_PARAM); + TemplateVariable first = new TemplateVariable("foo", SEGMENT); + TemplateVariable second = new TemplateVariable("bar", REQUEST_PARAM); TemplateVariables variables = new TemplateVariables(first, second); @@ -82,10 +83,8 @@ public class TemplateVariablesUnitTest { @Test public void concatsVariables() { - TemplateVariables first = new TemplateVariables(new TemplateVariable("foo", VariableType.SEGMENT)); - TemplateVariables second = new TemplateVariables(new TemplateVariable("bar", VariableType.REQUEST_PARAM)); - - TemplateVariables variables = first.concat(second); + TemplateVariables variables = new TemplateVariables(new TemplateVariable("foo", SEGMENT)); + variables = variables.concat(new TemplateVariable("bar", REQUEST_PARAM)); assertThat(variables.toString(), is("{/foo}{?bar}")); } @@ -96,8 +95,8 @@ public class TemplateVariablesUnitTest { @Test public void combinesContinuedParamWithParam() { - TemplateVariable first = new TemplateVariable("foo", VariableType.REQUEST_PARAM); - TemplateVariable second = new TemplateVariable("bar", VariableType.REQUEST_PARAM_CONTINUED); + TemplateVariable first = new TemplateVariable("foo", REQUEST_PARAM); + TemplateVariable second = new TemplateVariable("bar", REQUEST_PARAM_CONTINUED); TemplateVariables variables = new TemplateVariables(first, second); @@ -110,8 +109,8 @@ public class TemplateVariablesUnitTest { @Test public void combinesContinuedParameterWithParameter() { - TemplateVariable first = new TemplateVariable("foo", VariableType.REQUEST_PARAM_CONTINUED); - TemplateVariable second = new TemplateVariable("bar", VariableType.REQUEST_PARAM); + TemplateVariable first = new TemplateVariable("foo", REQUEST_PARAM_CONTINUED); + TemplateVariable second = new TemplateVariable("bar", REQUEST_PARAM); TemplateVariables variables = new TemplateVariables(first, second); @@ -124,7 +123,7 @@ public class TemplateVariablesUnitTest { @Test public void dropsDuplicateTemplateVariable() { - TemplateVariable variable = new TemplateVariable("foo", VariableType.REQUEST_PARAM); + TemplateVariable variable = new TemplateVariable("foo", REQUEST_PARAM); TemplateVariables variables = new TemplateVariables(variable); List result = variables.concat(variable).asList(); @@ -132,4 +131,44 @@ public class TemplateVariablesUnitTest { assertThat(result, hasSize(1)); assertThat(result, hasItem(variable)); } + + /** + * @see #217 + */ + @Test + public void considersRequestParameterVariablesEquivalent() { + + TemplateVariable parameter = new TemplateVariable("foo", REQUEST_PARAM); + TemplateVariable continued = new TemplateVariable("foo", REQUEST_PARAM_CONTINUED); + TemplateVariable fragment = new TemplateVariable("foo", FRAGMENT); + + assertThat(parameter.isEquivalent(continued), is(true)); + assertThat(continued.isEquivalent(parameter), is(true)); + assertThat(fragment.isEquivalent(continued), is(false)); + } + + /** + * @see #217 + */ + @Test + public void considersFragementVariable() { + + assertThat(new TemplateVariable("foo", VariableType.FRAGMENT).isFragment(), is(true)); + assertThat(new TemplateVariable("foo", VariableType.REQUEST_PARAM).isFragment(), is(false)); + } + + /** + * @see #217 + */ + @Test + public void doesNotAddEquivalentVariable() { + + TemplateVariable parameter = new TemplateVariable("foo", VariableType.REQUEST_PARAM); + TemplateVariable parameterContinued = new TemplateVariable("foo", VariableType.REQUEST_PARAM_CONTINUED); + + List result = new TemplateVariables(parameter).concat(parameterContinued).asList(); + + assertThat(result, hasSize(1)); + assertThat(result, hasItem(parameter)); + } } diff --git a/src/test/java/org/springframework/hateoas/UriTemplateUnitTest.java b/src/test/java/org/springframework/hateoas/UriTemplateUnitTest.java index d243c2c7..4d3d809a 100644 --- a/src/test/java/org/springframework/hateoas/UriTemplateUnitTest.java +++ b/src/test/java/org/springframework/hateoas/UriTemplateUnitTest.java @@ -207,6 +207,31 @@ public class UriTemplateUnitTest { assertVariables(source.with(new TemplateVariables(toAdd)), expected); } + /** + * @see #217 + */ + @Test + public void doesNotAddVariablesForAlreadyExistingRequestParameters() { + + UriTemplate template = new UriTemplate("/?page=2"); + UriTemplate result = template.with(new TemplateVariables(new TemplateVariable("page", VariableType.REQUEST_PARAM))); + assertThat(result.getVariableNames(), is(empty())); + + result = template.with(new TemplateVariables(new TemplateVariable("page", VariableType.REQUEST_PARAM_CONTINUED))); + assertThat(result.getVariableNames(), is(empty())); + } + + /** + * @see #217 + */ + @Test + public void doesNotAddVariablesForAlreadyExistingFragment() { + + UriTemplate template = new UriTemplate("/#fragment"); + UriTemplate result = template.with(new TemplateVariables(new TemplateVariable("fragment", VariableType.FRAGMENT))); + assertThat(result.getVariableNames(), is(empty())); + } + private static void assertVariables(UriTemplate template, TemplateVariable... variables) { assertVariables(template, Arrays.asList(variables)); }