Skip to content

Commit

Permalink
Document and workaround a double-encoding resulting in broken paginat…
Browse files Browse the repository at this point in the history 
…ion links on the web UI

Pagination links created by Spring MVC/HATEOAS get encoded multiple times, resulting in broken next/previous links when navigating multi-page results.
Document this behavior with a test (so that it will be noticed if it changes).

Workaround by performing multiple decoding on such links before displaying them in the Genie web UI.
  • Loading branch information
@mprimi
mprimi committed Oct 19, 2017
1 parent 8a24012 commit 9db1087
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 4 deletions.
8 changes: 4 additions & 4 deletions genie-web/src/main/resources/static/scripts/components/Pagination.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const Pagination = props => {
<li key="1">
<PageLink
pageType={props.pageType}
url={props.links.first.href}
url={decodeURI(decodeURI(props.links.first.href))} // Workaround for double encoding
text="&laquo;"
/>
</li>
Expand All @@ -27,7 +27,7 @@ const Pagination = props => {
<li key="2">
<PageLink
pageType={props.pageType}
url={props.links.prev.href}
url={decodeURI(decodeURI(props.links.prev.href))} // Workaround for double encoding
text="&larr; Previous"
/>
</li>
Expand All @@ -38,7 +38,7 @@ const Pagination = props => {
<li key="3">
<PageLink
pageType={props.pageType}
url={props.links.next.href}
url={decodeURI(decodeURI(props.links.next.href))} // Workaround for double encoding
text="Next &rarr;"
/>
</li>
Expand All @@ -49,7 +49,7 @@ const Pagination = props => {
<li key="4">
<PageLink
pageType={props.pageType}
url={props.links.last.href}
url={decodeURI(decodeURI(props.links.last.href))} // Workaround for double encoding
text="&raquo;"
/>
</li>
Expand Down
102 changes: 102 additions & 0 deletions ...rc/test/java/com/netflix/genie/web/controllers/ClusterRestControllerIntegrationTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@
*/
package com.netflix.genie.web.controllers;

import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.github.fge.jsonpatch.JsonPatch;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.netflix.genie.common.dto.Cluster;
import com.netflix.genie.common.dto.ClusterStatus;
import com.netflix.genie.common.dto.Command;
Expand All @@ -27,6 +30,8 @@
import com.netflix.genie.core.jpa.repositories.JpaCommandRepository;
import com.netflix.genie.web.aspect.DataServiceRetryAspect;
import com.netflix.genie.web.hateoas.resources.ClusterResource;
import org.apache.catalina.util.URLEncoder;
import org.apache.http.client.utils.URLEncodedUtils;
import org.hamcrest.Matchers;
import org.junit.After;
import org.junit.Assert;
Expand All @@ -44,13 +49,17 @@
import org.springframework.restdocs.request.RequestDocumentation;
import org.springframework.restdocs.snippet.Attributes;
import org.springframework.retry.RetryListener;
import org.springframework.test.web.servlet.MvcResult;
import org.springframework.test.web.servlet.request.MockMvcRequestBuilders;
import org.springframework.test.web.servlet.result.MockMvcResultMatchers;

import javax.sql.DataSource;
import java.net.URI;
import java.net.URLDecoder;
import java.nio.charset.StandardCharsets;
import java.sql.Connection;
import java.sql.Statement;
import java.util.HashMap;
import java.util.UUID;

/**
Expand Down Expand Up @@ -1174,4 +1183,97 @@ public void canRemoveCommandFromACluster() throws Exception {
.andExpect(MockMvcResultMatchers.jsonPath("$", Matchers.hasSize(1)))
.andExpect(MockMvcResultMatchers.jsonPath("$[0].id", Matchers.is(ID)));
}

/**
* This test "documents" a known bug in Spring HATEOAS links that results in doubly-encoded pagination links.
* https://github.com/spring-projects/spring-hateoas/issues/559
* Currently, we work around this bug in the UI by decoding these elements (see Pagination.js).
* If this test starts failing, it may be because the behavior has been corrected, and the workaround may be
* removed.
*
* @throws Exception on error
*/
@Test
public void testPagingDoubleEncoding() throws Exception {
Assert.assertThat(this.jpaClusterRepository.count(), Matchers.is(0L));
final String id1 = UUID.randomUUID().toString();
final String id2 = UUID.randomUUID().toString();
final String id3 = UUID.randomUUID().toString();
final String name1 = "Test " + UUID.randomUUID().toString();
final String name2 = "Test " + UUID.randomUUID().toString();
final String name3 = "Test " + UUID.randomUUID().toString();
final String user1 = UUID.randomUUID().toString();
final String user2 = UUID.randomUUID().toString();
final String user3 = UUID.randomUUID().toString();
final String version1 = UUID.randomUUID().toString();
final String version2 = UUID.randomUUID().toString();
final String version3 = UUID.randomUUID().toString();

this.createConfigResource(
new Cluster.Builder(name1, user1, version1, ClusterStatus.UP).withId(id1).build(),
null
);
Thread.sleep(1000);
this.createConfigResource(
new Cluster.Builder(name2, user2, version2, ClusterStatus.OUT_OF_SERVICE).withId(id2).build(),
null
);
Thread.sleep(1000);
this.createConfigResource(
new Cluster.Builder(name3, user3, version3, ClusterStatus.TERMINATED).withId(id3).build(),
null
);

Assert.assertThat(this.jpaClusterRepository.count(), Matchers.is(3L));

final URLEncoder urlEncoder = new URLEncoder();

final String unencodedNameQuery = "Test %";
final String singleEncodedNameQuery = urlEncoder.encode(unencodedNameQuery, StandardCharsets.UTF_8);
final String doubleEncodedNameQuery = urlEncoder.encode(singleEncodedNameQuery, StandardCharsets.UTF_8);

// Query by name with wildcard and get the second page containing a single result (out of 3)
final MvcResult response = this.mvc
.perform(MockMvcRequestBuilders.get(CLUSTERS_API)
.param("name", unencodedNameQuery)
.param("size", "1")
.param("page", "1")
)
.andExpect(MockMvcResultMatchers.status().isOk())
.andExpect(MockMvcResultMatchers.content().contentTypeCompatibleWith(MediaTypes.HAL_JSON))
.andExpect(MockMvcResultMatchers.content().encoding(StandardCharsets.UTF_8.name()))
.andExpect(MockMvcResultMatchers.jsonPath(CLUSTERS_LIST_PATH, Matchers.hasSize(1)))
.andReturn();

final JsonNode responseJsonNode = new ObjectMapper().readTree(response.getResponse().getContentAsString());

// Self link is not double-encoded
Assert.assertTrue(
responseJsonNode
.get("_links")
.get("self")
.get("href")
.asText()
.contains(singleEncodedNameQuery));

// Pagination links are double-encoded

final String[] doubleEncodedHrefs = new String[] {
"first", "next", "prev", "last",
};

for (String doubleEncodedHref : doubleEncodedHrefs) {
final String linkString = responseJsonNode.get("_links").get(doubleEncodedHref).get("href").asText();
Assert.assertNotNull(linkString);
final HashMap<String, String> params = Maps.newHashMap();
URLEncodedUtils.parse(new URI(linkString), StandardCharsets.UTF_8)
.forEach(nameValuePair -> params.put(nameValuePair.getName(), nameValuePair.getValue()));

Assert.assertTrue(params.containsKey("name"));
// Correct: singleEncodedNameQuery, actual: doubleEncodedNameQuery
Assert.assertEquals(doubleEncodedNameQuery, params.get("name"));
final String decoded = URLDecoder.decode(params.get("name"), StandardCharsets.UTF_8.name());
Assert.assertEquals(singleEncodedNameQuery, decoded);
}
}
}

0 comments on commit 9db1087

Please sign in to comment.