Skip to content

Commit

Permalink
test: asssert on actual types and their order
Browse files Browse the repository at this point in the history 
instead of strings. Add tests for passing comma separated values in one
order param. Add test for the mixed case of comma separated values in one
order param with a repeated param
  • Loading branch information
@teleivo
teleivo committed Mar 1, 2024
1 parent d245c75 commit 8d56d25
Show file tree
Hide file tree
Showing 3 changed files with 158 additions and 43 deletions.
2 changes: 1 addition & 1 deletion ...his-api/src/main/java/org/hisp/dhis/webapi/controller/event/webrequest/OrderCriteria.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public static OrderCriteria valueOf(String input) {
String field = props[0].trim();
if (StringUtils.isEmpty(field)) {
throw new IllegalArgumentException(
"Invalid order property: '"
"Missing field name in order property: '"
+ input
+ "'. Valid formats are 'field:direction' or 'field'. Valid directions are 'asc' or 'desc'. Direction defaults to 'asc'.");
}
Expand Down
39 changes: 35 additions & 4 deletions dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/CrudControllerAdvice.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@
import org.hisp.dhis.webapi.security.apikey.ApiTokenAuthenticationException;
import org.hisp.dhis.webapi.security.apikey.ApiTokenError;
import org.springframework.beans.TypeMismatchException;
import org.springframework.core.convert.ConversionFailedException;
import org.springframework.core.convert.TypeDescriptor;
import org.springframework.dao.DataAccessResourceFailureException;
import org.springframework.http.HttpStatus;
import org.springframework.http.converter.HttpMessageNotReadableException;
Expand Down Expand Up @@ -214,8 +216,10 @@ public WebMessage methodArgumentTypeMismatchException(MethodArgumentTypeMismatch
Class<?> requiredType = ex.getRequiredType();
PathVariable pathVariableAnnotation =
ex.getParameter().getParameterAnnotation(PathVariable.class);
String field = ex.getName();
Object value = ex.getValue();
String notValidValueMessage =
getNotValidValueMessage(ex.getValue(), ex.getName(), pathVariableAnnotation != null);
getNotValidValueMessage(value, field, pathVariableAnnotation != null);

String customErrorMessage;
if (requiredType == null) {
Expand All @@ -226,6 +230,9 @@ public WebMessage methodArgumentTypeMismatchException(MethodArgumentTypeMismatch
customErrorMessage = getGenericFieldErrorMessage(requiredType.getSimpleName());
} else if (ex.getCause() instanceof IllegalArgumentException) {
customErrorMessage = ex.getCause().getMessage();
} else if (ex.getCause() instanceof ConversionFailedException conversionException) {
notValidValueMessage = getConversionErrorMessage(value, field, conversionException);
customErrorMessage = "";
} else {
customErrorMessage = getGenericFieldErrorMessage(requiredType.getSimpleName());
}
Expand All @@ -237,7 +244,9 @@ public WebMessage methodArgumentTypeMismatchException(MethodArgumentTypeMismatch
@ResponseBody
public WebMessage handleTypeMismatchException(TypeMismatchException ex) {
Class<?> requiredType = ex.getRequiredType();
String notValidValueMessage = getNotValidValueMessage(ex.getValue(), ex.getPropertyName());
String field = ex.getPropertyName();
Object value = ex.getValue();
String notValidValueMessage = getNotValidValueMessage(value, field);

String customErrorMessage;
if (requiredType == null) {
Expand All @@ -248,6 +257,9 @@ public WebMessage handleTypeMismatchException(TypeMismatchException ex) {
customErrorMessage = getGenericFieldErrorMessage(requiredType.getSimpleName());
} else if (ex.getCause() instanceof IllegalArgumentException) {
customErrorMessage = ex.getCause().getMessage();
} else if (ex.getCause() instanceof ConversionFailedException conversionException) {
notValidValueMessage = getConversionErrorMessage(value, field, conversionException);
customErrorMessage = "";
} else {
customErrorMessage = getGenericFieldErrorMessage(requiredType.getSimpleName());
}
Expand All @@ -269,11 +281,12 @@ private String getGenericFieldErrorMessage(String fieldType) {
return MessageFormat.format("It should be of type {0}", fieldType);
}

private String getNotValidValueMessage(Object value, String field) {
private static String getNotValidValueMessage(Object value, String field) {
return getNotValidValueMessage(value, field, false);
}

private String getNotValidValueMessage(Object value, String field, boolean isPathVariable) {
private static String getNotValidValueMessage(
Object value, String field, boolean isPathVariable) {
if (value == null || (value instanceof String stringValue && stringValue.isEmpty())) {
return MessageFormat.format("{0} cannot be empty.", field);
}
Expand All @@ -288,9 +301,27 @@ private String getFormattedBadRequestMessage(Object value, String field, String
}

private String getFormattedBadRequestMessage(String fieldErrorMessage, String customMessage) {
if (StringUtils.isEmpty(customMessage)) {
return fieldErrorMessage;
}
return fieldErrorMessage + " " + customMessage;
}

private static String getConversionErrorMessage(
Object rootValue, String field, ConversionFailedException ex) {
Object invalidValue = ex.getValue();
if (TypeDescriptor.valueOf(String.class).equals(ex.getSourceType())
&& (invalidValue != null && ((String) invalidValue).contains(","))
&& (rootValue != null && rootValue.getClass().isArray())) {
return "You likely repeated request parameter '"
+ field
+ "' and used multiple comma-separated values within at least one of its values. Choose one of these approaches. "
+ ex.getCause().getMessage();
}

return getNotValidValueMessage(invalidValue, field) + " " + ex.getCause().getMessage();
}

/**
* A BindException wraps possible errors happened trying to bind all the request parameters to a
* binding object. Errors could be simple conversion failures (Trying to convert a 'TEXT' to an
Expand Down
160 changes: 122 additions & 38 deletions dhis-2/dhis-web-api/src/test/java/org/hisp/dhis/webapi/controller/OrderBindingTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,25 @@
*/
package org.hisp.dhis.webapi.controller;

import static org.hamcrest.core.StringContains.containsString;
import static org.hisp.dhis.dxf2.webmessage.WebMessageUtils.ok;
import static org.hisp.dhis.utils.Assertions.assertContains;
import static org.hisp.dhis.utils.Assertions.assertStartsWith;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;

import java.util.List;
import lombok.Data;
import org.hisp.dhis.common.SortDirection;
import org.hisp.dhis.dxf2.webmessage.WebMessage;
import org.hisp.dhis.jsontree.JsonMixed;
import org.hisp.dhis.webapi.controller.event.webrequest.OrderCriteria;
import org.hisp.dhis.webapi.json.domain.JsonWebMessage;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.springframework.mock.web.MockHttpServletResponse;
import org.springframework.stereotype.Controller;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.test.web.servlet.setup.MockMvcBuilders;
Expand All @@ -45,9 +54,10 @@
import org.springframework.web.bind.annotation.ResponseBody;

class OrderBindingTest {
private static final String ENDPOINT = "/ordering";

private static final String METHOD_PARAMETER_ENDPOINT = "/order/methodParameter";
private static final String CLASS_PARAMETER_ENDPOINT = "/order/classParameter";
private MockMvc mockMvc;
private List<OrderCriteria> actual;

@BeforeEach
public void setUp() {
Expand All @@ -60,60 +70,134 @@ public void setUp() {
@Test
void shouldReturnDefaultSortDirectionWhenNoSortDirectionIsPassedAsParameter() throws Exception {
mockMvc
.perform(get(ENDPOINT).param("order", "field"))
.andExpect(content().string(containsString("OK")))
.andExpect(content().string(containsString("field")))
.andExpect(content().string(containsString("ASC")));
.perform(get(METHOD_PARAMETER_ENDPOINT).param("order", "field"))
.andExpect(status().isOk());

assertEquals(List.of(OrderCriteria.of("field", SortDirection.ASC)), actual);
}

@Test
void shouldReturnAscSortDirectionWhenAscSortDirectionIsPassedAsParameter() throws Exception {
mockMvc
.perform(get(ENDPOINT).param("order", "field:asc"))
.andExpect(content().string(containsString("OK")))
.andExpect(content().string(containsString("field")))
.andExpect(content().string(containsString("ASC")));
.perform(get(METHOD_PARAMETER_ENDPOINT).param("order", "field:asc"))
.andExpect(status().isOk());

assertEquals(List.of(OrderCriteria.of("field", SortDirection.ASC)), actual);
}

@Test
void shouldReturnDescSortDirectionWhenDescSortDirectionIsPassedAsParameter() throws Exception {
mockMvc
.perform(get(ENDPOINT).param("order", "field:desc"))
.andExpect(content().string(containsString("OK")))
.andExpect(content().string(containsString("field")))
.andExpect(content().string(containsString("DESC")));
.perform(get(METHOD_PARAMETER_ENDPOINT).param("order", "field:desc"))
.andExpect(status().isOk());

assertEquals(List.of(OrderCriteria.of("field", SortDirection.DESC)), actual);
}

@Test
void shouldReturnABadRequestWhenInvalidSortDirectionIsPassedAsParameter() throws Exception {
@ParameterizedTest
@ValueSource(strings = {METHOD_PARAMETER_ENDPOINT, CLASS_PARAMETER_ENDPOINT})
void shouldHandleMultipleOrderComponentsGivenViaOneParameter(String endpoint) throws Exception {
mockMvc
.perform(get(ENDPOINT).param("order", "field:wrong"))
.andExpect(content().string(containsString("Bad Request")))
.andExpect(
content()
.string(
containsString(
"'wrong' is not a valid sort direction. Valid values are: [ASC, DESC]")));
.perform(get(endpoint).param("order", "field1:desc,field2:asc"))
.andExpect(status().isOk());

assertEquals(
List.of(
OrderCriteria.of("field1", SortDirection.DESC),
OrderCriteria.of("field2", SortDirection.ASC)),
actual);
}

@Test
void shouldReturnABadRequestWhenInvalidSortDirectionIsPassedAsParameterInAListOfOrders()
@ParameterizedTest
@ValueSource(strings = {METHOD_PARAMETER_ENDPOINT, CLASS_PARAMETER_ENDPOINT})
void shouldHandleMultipleOrderComponentsGivenViaMultipleParameters(String endpoint)
throws Exception {
mockMvc
.perform(get(ENDPOINT).param("order", "field1:wrong").param("order", "field2:asc"))
.andExpect(content().string(containsString("Bad Request")))
.andExpect(
content()
.string(
containsString(
"'wrong' is not a valid sort direction. Valid values are: [ASC, DESC]")));
.perform(get(endpoint).param("order", "field1:desc").param("order", "field2:asc"))
.andExpect(status().isOk());

assertEquals(
List.of(
OrderCriteria.of("field1", SortDirection.DESC),
OrderCriteria.of("field2", SortDirection.ASC)),
actual);
}

@ParameterizedTest
@ValueSource(strings = {METHOD_PARAMETER_ENDPOINT, CLASS_PARAMETER_ENDPOINT})
void shouldReturnABadRequestWhenMixingRepeatedParameterAndCommaSeparatedValues(String endpoint)
throws Exception {
MockHttpServletResponse response =
mockMvc
.perform(
get(endpoint)
.accept("application/json")
.param("order", "field1:desc")
.param("order", "field2:asc,field3:desc"))
.andReturn()
.getResponse();

JsonWebMessage message = JsonMixed.of(response.getContentAsString()).as(JsonWebMessage.class);
assertEquals(400, message.getHttpStatusCode());
assertStartsWith(
"You likely repeated request parameter 'order' and used", message.getMessage());
}

@ParameterizedTest
@ValueSource(strings = {METHOD_PARAMETER_ENDPOINT, CLASS_PARAMETER_ENDPOINT})
void shouldReturnABadRequestWhenInvalidSortDirectionIsPassedAsParameter(String endpoint)
throws Exception {
MockHttpServletResponse response =
mockMvc
.perform(get(endpoint).accept("application/json").param("order", "field:wrong"))
.andReturn()
.getResponse();

JsonWebMessage message = JsonMixed.of(response.getContentAsString()).as(JsonWebMessage.class);
assertEquals(400, message.getHttpStatusCode());
assertStartsWith(
"'wrong' is not a valid sort direction. Valid values are: [ASC,", message.getMessage());
}

@ParameterizedTest
@ValueSource(strings = {METHOD_PARAMETER_ENDPOINT, CLASS_PARAMETER_ENDPOINT})
void shouldReturnABadRequestWhenInvalidSortDirectionIsPassedAsParameterInAListOfOrders(
String endpoint) throws Exception {
MockHttpServletResponse response =
mockMvc
.perform(
get(endpoint)
.accept("application/json")
.param("order", "field1:wrong")
.param("order", "field2:asc"))
.andReturn()
.getResponse();

JsonWebMessage message = JsonMixed.of(response.getContentAsString()).as(JsonWebMessage.class);
assertEquals(400, message.getHttpStatusCode());
assertStartsWith("Value 'field1:wrong' is not valid for parameter order", message.getMessage());
assertContains(
"'wrong' is not a valid sort direction. Valid values are: [ASC,", message.getMessage());
}

@Controller
private static class OrderingController extends CrudControllerAdvice {
@GetMapping(value = ENDPOINT)
public @ResponseBody WebMessage getOrder(@RequestParam List<OrderCriteria> order) {
return ok(order.toString());
private class OrderingController extends CrudControllerAdvice {
@Data
static class RequestParams {
List<OrderCriteria> order;
}

@GetMapping(value = CLASS_PARAMETER_ENDPOINT)
public @ResponseBody WebMessage getOrderViaClassParameter(RequestParams params) {
actual = params.order;
return ok();
}

@GetMapping(value = METHOD_PARAMETER_ENDPOINT)
public @ResponseBody WebMessage getOrderViaMethodParameter(
@RequestParam List<OrderCriteria> order) {
actual = order;
return ok();
}
}
}

0 comments on commit 8d56d25

Please sign in to comment.