From d98e678c591cdfd56896f7e95b07d471a341a733 Mon Sep 17 00:00:00 2001 From: Oliver Drotbohm Date: Thu, 28 Nov 2019 15:00:37 +0100 Subject: [PATCH] #593 - Make UriTemplate smarter to avoid reencoding of given URI strings. UriTemplate now uses UriBuilderFactory (DefaultUriBuilderFactory in particular) to expand templates. We inspect the given source URI string, try to decode it and configure the factory to only encode values if the decoded String is shorter than the source one as that indicates it already contains encoded characters. The UriBuilderFactory is held as transient value as its implementations are usually not serializable in the first place. Added the necessary logic to recreate the factory instance on deserialization. Added all expansion tests given in the original ticket as unit tests. --- .../springframework/hateoas/UriTemplate.java | 60 ++++++++- .../hateoas/UriTemplateUnitTest.java | 122 +++++++++++++++++- 2 files changed, 174 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/springframework/hateoas/UriTemplate.java b/src/main/java/org/springframework/hateoas/UriTemplate.java index e899a345..e01aaf50 100644 --- a/src/main/java/org/springframework/hateoas/UriTemplate.java +++ b/src/main/java/org/springframework/hateoas/UriTemplate.java @@ -15,8 +15,11 @@ */ package org.springframework.hateoas; +import java.io.IOException; +import java.io.ObjectInputStream; import java.io.Serializable; import java.net.URI; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Arrays; import java.util.Iterator; @@ -31,8 +34,13 @@ import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.ConcurrentReferenceHashMap; import org.springframework.util.StringUtils; +import org.springframework.web.util.DefaultUriBuilderFactory; +import org.springframework.web.util.DefaultUriBuilderFactory.EncodingMode; +import org.springframework.web.util.UriBuilder; +import org.springframework.web.util.UriBuilderFactory; import org.springframework.web.util.UriComponents; import org.springframework.web.util.UriComponentsBuilder; +import org.springframework.web.util.UriUtils; /** * Custom URI template to support qualified URI template variables. @@ -52,6 +60,7 @@ public class UriTemplate implements Iterable, Serializable { private final TemplateVariables variables; private String toString; private String baseUri; + private transient UriBuilderFactory factory; /** * Creates a new {@link UriTemplate} using the given template string. @@ -93,6 +102,7 @@ public class UriTemplate implements Iterable, Serializable { this.variables = variables.isEmpty() ? TemplateVariables.NONE : new TemplateVariables(variables); this.baseUri = template.substring(0, baseUriEndIndex); + this.factory = createFactory(baseUri); } /** @@ -244,16 +254,21 @@ public class UriTemplate implements Iterable, Serializable { return URI.create(baseUri); } - UriComponentsBuilder builder = UriComponentsBuilder.fromUriString(baseUri); + UriBuilder builder = factory.uriString(baseUri); Iterator iterator = Arrays.asList(parameters).iterator(); + variables.asList().stream() // + .filter(TemplateVariable::isRequired)// + .filter(__ -> iterator.hasNext()) // + .forEach(__ -> iterator.next()); + for (TemplateVariable variable : getOptionalVariables()) { Object value = iterator.hasNext() ? iterator.next() : null; appendToBuilder(builder, variable, value); } - return builder.buildAndExpand(parameters).toUri(); + return builder.build(parameters); } /** @@ -270,13 +285,13 @@ public class UriTemplate implements Iterable, Serializable { return URI.create(baseUri); } - UriComponentsBuilder builder = UriComponentsBuilder.fromUriString(baseUri); + UriBuilder builder = factory.uriString(baseUri); for (TemplateVariable variable : getOptionalVariables()) { appendToBuilder(builder, variable, parameters.get(variable.getName())); } - return builder.buildAndExpand(parameters).toUri(); + return builder.build(parameters); } /* @@ -313,6 +328,25 @@ public class UriTemplate implements Iterable, Serializable { .collect(Collectors.collectingAndThen(Collectors.toList(), TemplateVariables::new)); } + /** + * Creates a {@link UriBuilderFactory} that might optionally encode the given base URI if it still needs to be + * encoded. + * + * @param baseUri must not be {@literal null} or empty. + * @return + */ + private static UriBuilderFactory createFactory(String baseUri) { + + EncodingMode mode = UriUtils.decode(baseUri, StandardCharsets.UTF_8).length() < baseUri.length() // + ? EncodingMode.VALUES_ONLY // + : EncodingMode.TEMPLATE_AND_VALUES; + + DefaultUriBuilderFactory factory = new DefaultUriBuilderFactory(); + factory.setEncodingMode(mode); + + return factory; + } + /** * Appends the value for the given {@link TemplateVariable} to the given {@link UriComponentsBuilder}. * @@ -320,7 +354,7 @@ public class UriTemplate implements Iterable, Serializable { * @param variable must not be {@literal null}. * @param value can be {@literal null}. */ - private static void appendToBuilder(UriComponentsBuilder builder, TemplateVariable variable, @Nullable Object value) { + private static void appendToBuilder(UriBuilder builder, TemplateVariable variable, @Nullable Object value) { if (value == null) { @@ -359,7 +393,7 @@ public class UriTemplate implements Iterable, Serializable { * @see https://tools.ietf.org/html/rfc6570#section-2.4.2 */ @SuppressWarnings("unchecked") - private static void appendComposite(UriComponentsBuilder builder, String name, Object value) { + private static void appendComposite(UriBuilder builder, String name, Object value) { if (value instanceof Iterable) { @@ -374,4 +408,18 @@ public class UriTemplate implements Iterable, Serializable { builder.queryParam(name, value); } } + + /** + * Recreate {@link UriBuilderFactory} on deserialization. + * + * @param in + * @throws IOException + * @throws ClassNotFoundException + */ + private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException { + + in.defaultReadObject(); + + this.factory = createFactory(baseUri); + } } diff --git a/src/test/java/org/springframework/hateoas/UriTemplateUnitTest.java b/src/test/java/org/springframework/hateoas/UriTemplateUnitTest.java index 441003bd..15cd1b90 100755 --- a/src/test/java/org/springframework/hateoas/UriTemplateUnitTest.java +++ b/src/test/java/org/springframework/hateoas/UriTemplateUnitTest.java @@ -16,17 +16,28 @@ package org.springframework.hateoas; import static org.assertj.core.api.Assertions.*; +import static org.springframework.hateoas.UriTemplateUnitTest.EncodingFixture.*; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.ObjectInputStream; +import java.io.ObjectOutputStream; import java.net.URI; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.stream.Stream; +import org.apache.commons.io.output.ByteArrayOutputStream; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; import org.springframework.hateoas.TemplateVariable.VariableType; /** @@ -344,9 +355,37 @@ class UriTemplateUnitTest { @Test // #1127 void escapesBaseUriProperly() { + of("https://example.org/foo and bar/{baz}", "https://example.org/foo%20and%20bar/xyzzy") // + .param("baz", "xyzzy") // + .verify(); + } - assertThat(UriTemplate.of("https://example.org/foo and bar/{baz}").expand("xyzzy")) - .hasToString("https://example.org/foo%20and%20bar/xyzzy"); + @ParameterizedTest // #593 + @MethodSource("getEncodingFixtures") + public void uriTemplateExpansionsShouldWork(EncodingFixture fixture) { + fixture.verify(); + } + + @Test // #593 + void deserializesProperly() throws IOException, ClassNotFoundException { + + UriTemplate template = UriTemplate.of("/{foo}"); + + try (ByteArrayOutputStream output = new ByteArrayOutputStream(); + ObjectOutputStream stream = new ObjectOutputStream(output)) { + + stream.writeObject(template); + + try (InputStream input = new ByteArrayInputStream(output.toByteArray()); + ObjectInputStream object = new ObjectInputStream(input)) { + + Object result = object.readObject(); + + assertThat(result).isInstanceOfSatisfying(UriTemplate.class, it -> { + assertThat(it.expand("bar")).hasToString("/bar"); + }); + } + } } private static void assertVariables(UriTemplate template, TemplateVariable... variables) { @@ -364,4 +403,83 @@ class UriTemplateUnitTest { assertThat(template.getVariableNames()).contains(variable.getName()); } } + + private static Stream getEncodingFixtures() { + + return Stream.of(// + of("/foo/bar/{?x}", "/foo/bar/?x=1").param("x", 1), // + of("/foo/bar/{?x,y}", "/foo/bar/?x=1&y=2").param("x", 1).param("y", 2), + of("/foo/bar{?x}{&y}", "/foo/bar?x=1&y=2").param("x", 1).param("y", 2), + of("/foo/bar?x=1{&y}", "/foo/bar?x=1&y=2").param("y", 2), // + of("/foo/bar?x=1{&y,z}", "/foo/bar?x=1&y=2&z=3").param("y", 2).param("z", 3L), + of("/foo{/x}", "/foo/1").param("x", 1), // + of("/foo{/x,y}", "/foo/1/2").param("x", 1).param("y", "2"), + of("/foo{/x}{/y}", "/foo/1/2").param("x", 1).param("y", "2"), + of("/foo{/x}{/y}{?z}", "/foo/1/2?z=3").param("x", 1).param("y", "2").param("z", 3L), + of("/foo/{x}", "/foo/1").param("x", 1), // + of("/foo/{x}/bar", "/foo/1/bar").param("x", 1), // + of("/services/foo/{x}/bar/{y}/gaz", "/services/foo/1/bar/2/gaz").param("x", 1).param("y", "2"), + of("/foo/{x}/bar/{y}/bar{?z}", "/foo/1/bar/2/bar?z=3").param("x", 1).param("y", "2").param("z", 3), + of("/foo/{x}/bar/{y}/bar{?z}", "/foo/1/bar/2/bar").param("x", 1).param("y", "2"), + of("/foo/{x}/bar/{y}/bar{?z}", "/foo/1/bar/2/bar").param("x", 1).param("y", "2"), + of("/foo/bar{?x,y,z}", "/foo/bar?x=1").param("x", 1), // + of("/foo/bar{?x,y,z}", "/foo/bar?x=1&y=2").param("x", 1).param("y", "2"), + of("/foo/bar{?x,y,z}", "/foo/bar?x=1&z=3").param("x", 1).param("z", 3L).skipVarArgsVerification(), + of("/foo/{x}/bar{/y}{?z}", "/foo/1/bar/2?z=3").param("x", 1).param("y", "2").param("z", 3L), + of("/foo/{x}/bar{/y}{?z}", "/foo/1/bar?z=3").param("x", 1).param("z", 3L).skipVarArgsVerification(), + of("/foo/{x}/bar{?y}{#z}", "/foo/1/bar?y=2").param("x", 1).param("y", "2"), + of("/foo/{x}/bar{?y}{#z}", "/foo/1/bar?y=2#3").param("x", 1).param("y", "2").param("z", 3L), + of("/foo/{x}/bar{?y}{#z}", "/foo/1/bar#3").param("x", 1).param("z", 3L).skipVarArgsVerification(), + of("/foo/b%20ar{?x}", "/foo/b%20ar?x=1").param("x", 1), // + of("/foo/b\"ar{?x}", "/foo/b%22ar?x=1").param("x", 1), // + of("/foo/b%22ar{?x}", "/foo/b%22ar?x=1").param("x", 1)); + } + + static class EncodingFixture { + + private final String template; + private final URI uri; + private final Map parameters; + private final boolean varArgsVerification; + + private EncodingFixture(String template, URI uri, Map parameters, boolean varArgsVerification) { + + this.template = template; + this.uri = uri; + this.parameters = parameters; + this.varArgsVerification = varArgsVerification; + } + + public static EncodingFixture of(String template, String uri) { + return new EncodingFixture(template, URI.create(uri), new LinkedHashMap<>(), true); + } + + public EncodingFixture param(String key, Object value) { + + Map newParameters = new LinkedHashMap<>(parameters); + newParameters.put(key, value); + + return new EncodingFixture(template, uri, newParameters, varArgsVerification); + } + + public EncodingFixture skipVarArgsVerification() { + return new EncodingFixture(template, uri, parameters, false); + } + + public void verify() { + + UriTemplate uriTemplate = UriTemplate.of(template); + + assertThat(uriTemplate.expand(parameters)).isEqualTo(uri); + + if (varArgsVerification) { + assertThat(uriTemplate.expand(parameters.values().toArray())).isEqualTo(uri); + } + } + + @Override + public String toString() { + return String.format("Expanding %s using parameters %s results in %s.", template, parameters, uri); + } + } }