Skip to content

Commit 6043389

Browse files
enricocolasanteenricocolasante
committed
chore: Validate sort direction and remove unsupported options [TECH-1622]
1 parent 20bb935 commit 6043389

File tree

7 files changed

+125
-38
lines changed

7 files changed

+125
-38
lines changed

‎dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/mapper/SortDirection.java

+11-14
Original file line numberDiff line numberDiff line change
@@ -34,29 +34,26 @@
3434
@Getter
3535
@AllArgsConstructor
3636
public enum SortDirection {
37-
ASC("asc", false),
38-
DESC("desc", false),
39-
IASC("iasc", true),
40-
IDESC("idesc", true);
41-
42-
private static final SortDirection DEFAULT_SORTING_DIRECTION = ASC;
37+
ASC("asc"),
38+
DESC("desc");
4339

4440
private final String value;
4541

46-
private final boolean ignoreCase;
47-
4842
public static SortDirection of(String value) {
49-
return of(value, DEFAULT_SORTING_DIRECTION);
50-
}
51-
52-
public static SortDirection of(String value, SortDirection defaultSortingDirection) {
5343
return Arrays.stream(values())
5444
.filter(sortDirection -> sortDirection.getValue().equalsIgnoreCase(value))
5545
.findFirst()
56-
.orElse(defaultSortingDirection);
46+
.orElseThrow(
47+
() ->
48+
new IllegalArgumentException(
49+
"'"
50+
+ value
51+
+ "' is not a valid sort direction. Valid values are: "
52+
+ Arrays.toString(SortDirection.values())
53+
+ "."));
5754
}
5855

5956
public boolean isAscending() {
60-
return this == ASC || this == IASC;
57+
return this == ASC;
6158
}
6259
}

‎dhis-2/dhis-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteria.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -68,10 +68,10 @@ private static List<OrderCriteria> toOrderCriterias(String s) {
6868
.collect(Collectors.toList());
6969
}
7070

71-
private static OrderCriteria toOrderCriteria(String s1) {
71+
public static OrderCriteria toOrderCriteria(String s1) {
7272
String[] props = s1.split(":");
7373
if (props.length == 2) {
74-
return OrderCriteria.of(props[0], SortDirection.of(props[1]));
74+
return OrderCriteria.of(props[0], SortDirection.of(props[1].trim()));
7575
}
7676
if (props.length == 1) {
7777
return OrderCriteria.of(props[0], SortDirection.ASC);

‎dhis-2/dhis-api/src/test/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteriaTest.java

+2-9
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import static org.junit.jupiter.api.Assertions.assertEquals;
3131
import static org.junit.jupiter.api.Assertions.assertNotNull;
3232
import static org.junit.jupiter.api.Assertions.assertNull;
33+
import static org.junit.jupiter.api.Assertions.assertThrows;
3334

3435
import java.util.List;
3536
import org.hisp.dhis.webapi.controller.event.mapper.SortDirection;
@@ -44,8 +45,6 @@ void fromOrderString() {
4445

4546
List<OrderCriteria> orderCriteria =
4647
OrderCriteria.fromOrderString("one:desc,, two:asc ,three ");
47-
48-
assertNotNull(orderCriteria);
4948
assertEquals(
5049
4, orderCriteria.size(), String.format("Expected 4 item, instead got %s", orderCriteria));
5150
assertEquals(OrderCriteria.of("one", SortDirection.DESC), orderCriteria.get(0));
@@ -79,13 +78,7 @@ void fromOrderStringDefaultsToAscGivenFieldAndColon() {
7978

8079
@Test
8180
void fromOrderStringDefaultsSortDirectionToAscGivenAnUnknownSortDirection() {
82-
83-
List<OrderCriteria> orderCriteria = OrderCriteria.fromOrderString("one:wrong");
84-
85-
assertNotNull(orderCriteria);
86-
assertEquals(
87-
1, orderCriteria.size(), String.format("Expected 1 item, instead got %s", orderCriteria));
88-
assertEquals(OrderCriteria.of("one", SortDirection.ASC), orderCriteria.get(0));
81+
assertThrows(IllegalArgumentException.class, () -> OrderCriteria.fromOrderString("one:wrong"));
8982
}
9083

9184
@Test

‎dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/security/config/StringToOrderCriteriaListConverter.java ‎dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/common/OrderCriteriaParamEditor.java

+6-12
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2004-2022, University of Oslo
2+
* Copyright (c) 2004-2023, University of Oslo
33
* All rights reserved.
44
*
55
* Redistribution and use in source and binary forms, with or without
@@ -25,20 +25,14 @@
2525
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
2626
* SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
2727
*/
28-
package org.hisp.dhis.webapi.security.config;
28+
package org.hisp.dhis.webapi.common;
2929

30-
import java.util.List;
30+
import java.beans.PropertyEditorSupport;
3131
import org.hisp.dhis.webapi.controller.event.webrequest.OrderCriteria;
32-
import org.springframework.core.convert.converter.Converter;
3332

34-
/**
35-
* String to Order Criterias converter for MVC requests
36-
*
37-
* @author Giuseppe Nespolino <[email protected]>
38-
*/
39-
class StringToOrderCriteriaListConverter implements Converter<String, List<OrderCriteria>> {
33+
public class OrderCriteriaParamEditor extends PropertyEditorSupport {
4034
@Override
41-
public List<OrderCriteria> convert(String source) {
42-
return OrderCriteria.fromOrderString(source);
35+
public void setAsText(String source) {
36+
setValue(OrderCriteria.toOrderCriteria(source));
4337
}
4438
}

‎dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/CrudControllerAdvice.java

+3
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,9 @@
7777
import org.hisp.dhis.schema.SchemaPathException;
7878
import org.hisp.dhis.tracker.imports.TrackerIdSchemeParam;
7979
import org.hisp.dhis.util.DateUtils;
80+
import org.hisp.dhis.webapi.common.OrderCriteriaParamEditor;
8081
import org.hisp.dhis.webapi.common.UIDParamEditor;
82+
import org.hisp.dhis.webapi.controller.event.webrequest.OrderCriteria;
8183
import org.hisp.dhis.webapi.controller.exception.MetadataImportConflictException;
8284
import org.hisp.dhis.webapi.controller.exception.MetadataSyncException;
8385
import org.hisp.dhis.webapi.controller.exception.MetadataVersionException;
@@ -145,6 +147,7 @@ protected void initBinder(WebDataBinder binder) {
145147
IdentifiableProperty.class, new FromTextPropertyEditor(String::toUpperCase));
146148
this.enumClasses.forEach(c -> binder.registerCustomEditor(c, new ConvertEnum(c)));
147149
binder.registerCustomEditor(TrackerIdSchemeParam.class, new IdSchemeParamEditor());
150+
binder.registerCustomEditor(OrderCriteria.class, new OrderCriteriaParamEditor());
148151
binder.registerCustomEditor(UID.class, new UIDParamEditor());
149152
}
150153

‎dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/security/config/WebMvcConfig.java

-1
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,6 @@ public void configureMessageConverters(List<HttpMessageConverter<?>> converters)
219219

220220
@Override
221221
protected void addFormatters(FormatterRegistry registry) {
222-
registry.addConverter(new StringToOrderCriteriaListConverter());
223222
registry.addConverter(new FieldPathConverter());
224223
}
225224

‎dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/OrderBindingTest.java

+101
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/*
2+
* Copyright (c) 2004-2022, University of Oslo
3+
* All rights reserved.
4+
*
5+
* Redistribution and use in source and binary forms, with or without
6+
* modification, are permitted provided that the following conditions are met:
7+
* Redistributions of source code must retain the above copyright notice, this
8+
* list of conditions and the following disclaimer.
9+
*
10+
* Redistributions in binary form must reproduce the above copyright notice,
11+
* this list of conditions and the following disclaimer in the documentation
12+
* and/or other materials provided with the distribution.
13+
* Neither the name of the HISP project nor the names of its contributors may
14+
* be used to endorse or promote products derived from this software without
15+
* specific prior written permission.
16+
*
17+
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
18+
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
19+
* WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
20+
* DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
21+
* ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
22+
* (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
23+
* LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
24+
* ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
25+
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
26+
* SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
27+
*/
28+
package org.hisp.dhis.webapi.controller;
29+
30+
import static org.hamcrest.core.StringContains.containsString;
31+
import static org.hisp.dhis.dxf2.webmessage.WebMessageUtils.ok;
32+
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
33+
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content;
34+
35+
import java.util.List;
36+
import org.hisp.dhis.dxf2.webmessage.WebMessage;
37+
import org.hisp.dhis.webapi.controller.event.webrequest.OrderCriteria;
38+
import org.junit.jupiter.api.BeforeEach;
39+
import org.junit.jupiter.api.Test;
40+
import org.springframework.stereotype.Controller;
41+
import org.springframework.test.web.servlet.MockMvc;
42+
import org.springframework.test.web.servlet.setup.MockMvcBuilders;
43+
import org.springframework.web.bind.annotation.GetMapping;
44+
import org.springframework.web.bind.annotation.RequestParam;
45+
import org.springframework.web.bind.annotation.ResponseBody;
46+
47+
class OrderBindingTest {
48+
private static final String ENDPOINT = "/ordering";
49+
50+
private MockMvc mockMvc;
51+
52+
@BeforeEach
53+
public void setUp() {
54+
mockMvc =
55+
MockMvcBuilders.standaloneSetup(new OrderingController())
56+
.setControllerAdvice(new CrudControllerAdvice())
57+
.build();
58+
}
59+
60+
@Test
61+
void shouldReturnABadRequestWhenInvalidValueForEnumIsPassedAsParameter() throws Exception {
62+
mockMvc
63+
.perform(get(ENDPOINT).param("order", "field"))
64+
.andExpect(content().string(containsString("OK")))
65+
.andExpect(content().string(containsString("field")))
66+
.andExpect(content().string(containsString("ASC")));
67+
}
68+
69+
@Test
70+
void shouldReturnABadRequestWhenInvalidValueForEnumInBindingObjectIsPassedAsParameter()
71+
throws Exception {
72+
mockMvc
73+
.perform(get(ENDPOINT).param("order", "field:wrong"))
74+
.andExpect(content().string(containsString("Bad Request")))
75+
.andExpect(
76+
content()
77+
.string(
78+
containsString(
79+
"'wrong' is not a valid sort direction. Valid values are: [ASC, DESC]")));
80+
}
81+
82+
@Test
83+
void shouldNot() throws Exception {
84+
mockMvc
85+
.perform(get(ENDPOINT).param("order", "field1:wrong").param("order", "field2:asc"))
86+
.andExpect(content().string(containsString("Bad Request")))
87+
.andExpect(
88+
content()
89+
.string(
90+
containsString(
91+
"'wrong' is not a valid sort direction. Valid values are: [ASC, DESC]")));
92+
}
93+
94+
@Controller
95+
private class OrderingController extends CrudControllerAdvice {
96+
@GetMapping(value = ENDPOINT)
97+
public @ResponseBody WebMessage getOrder(@RequestParam List<OrderCriteria> order) {
98+
return ok(order.toString());
99+
}
100+
}
101+
}

0 commit comments

Comments
 (0)