Skip to content

Commit

Permalink
CHE-2022; fixed validation behavior when field value matches default …
Browse files Browse the repository at this point in the history 
…value for its type; Fixes #2022
  • Loading branch information
@mshaposhnik
mshaposhnik committed Aug 2, 2016
1 parent 0be2173 commit 60f4657
Show file tree
Hide file tree
Showing 5 changed files with 44 additions and 20 deletions.
2 changes: 2 additions & 0 deletions ...che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/FactoryService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ public Factory saveFactory(Factory factory)
throw new BadRequestException("Not null factory required");
}
processDefaults(factory);
factoryBuilder.checkValid(factory);
createValidator.validateOnCreate(factory);
final Factory storedFactory = factoryStore.getFactory(factoryStore.saveFactory(factory, null));
return storedFactory.withLinks(createLinks(storedFactory, null, uriInfo));
Expand Down Expand Up @@ -345,6 +346,7 @@ public Factory updateFactory(@ApiParam(value = "Factory id")
newFactory.setId(existingFactory.getId());

// validate the new content
factoryBuilder.checkValid(newFactory, true);
createValidator.validateOnCreate(newFactory);

// access granted, user can update the factory
Expand Down
14 changes: 5 additions & 9 deletions ...er/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/ValueHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
import java.util.Collection;
import java.util.Map;

import static com.google.common.base.Strings.isNullOrEmpty;

/**
* @author Sergii Kabashniuk
*/
Expand All @@ -22,19 +24,13 @@ public class ValueHelper {
*
* @param value
* - value to check
* @return - true if value is useless for factory (0 for primitives or empty collection or map), false otherwise
* @return - true if value is useless for factory (empty string, collection or map), false otherwise
*/
public static boolean isEmpty(Object value) {
return (null == value) ||
(value.getClass().equals(Boolean.class) && !((Boolean)value)) ||
(value.getClass().equals(Integer.class) && (Integer)value == 0) ||
(value.getClass().equals(Long.class) && (Long)value == 0) ||
(value.getClass().equals(String.class) && isNullOrEmpty((String)value) ||
(Collection.class.isAssignableFrom(value.getClass()) && ((Collection)value).isEmpty()) ||
(Map.class.isAssignableFrom(value.getClass()) && ((Map)value).isEmpty()) ||
(value.getClass().equals(Byte.class) && (Byte)value == 0) ||
(value.getClass().equals(Short.class) && (Short)value == 0) ||
(value.getClass().equals(Double.class) && (Double)value == 0) ||
(value.getClass().equals(Float.class) && (Float)value == 0);
(Map.class.isAssignableFrom(value.getClass()) && ((Map)value).isEmpty()));
}

}
40 changes: 29 additions & 11 deletions ...-api-factory/src/main/java/org/eclipse/che/api/factory/server/builder/FactoryBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,13 +116,27 @@ public Factory build(InputStream json) throws IOException, ConflictException {
}

/**
* Validate factory compatibility.
* Validate factory compatibility at creation time.
*
* @param factory
* - factory object to validate
* @throws ConflictException
*/
public void checkValid(Factory factory) throws ConflictException {
checkValid(factory, false);
}

/**
* Validate factory compatibility.
*
* @param factory
* - factory object to validate
* @param isUpdate
* - indicates is validation performed on update time.
* Set-by-server variables are allowed during update.
* @throws ConflictException
*/
public void checkValid(Factory factory, boolean isUpdate) throws ConflictException {
if (null == factory) {
throw new ConflictException(FactoryConstants.UNPARSABLE_FACTORY_MESSAGE);
}
Expand All @@ -145,7 +159,7 @@ public void checkValid(Factory factory) throws ConflictException {
default:
throw new ConflictException(FactoryConstants.INVALID_VERSION_MESSAGE);
}
validateCompatibility(factory, null, Factory.class, usedFactoryVersionMethodProvider, v, "");
validateCompatibility(factory, null, Factory.class, usedFactoryVersionMethodProvider, v, "", isUpdate);
}

/**
Expand All @@ -170,7 +184,7 @@ public Factory convertToLatest(Factory factory) throws ApiException {
*
* @param object
* - object to validate factory parameters
* @param object
* @param parent
* - parent object
* @param methodsProvider
* - class that provides methods with {@link org.eclipse.che.api.core.factory.FactoryParameter}
Expand All @@ -188,7 +202,8 @@ void validateCompatibility(Object object,
Class methodsProvider,
Class allowedMethodsProvider,
Version version,
String parentName) throws ConflictException {
String parentName,
boolean isUpdate) throws ConflictException {
// validate source
if (SourceStorageDto.class.equals(methodsProvider) && !hasSubprojectInPath(parent)) {
sourceStorageParametersValidator.validate((SourceStorage)object, version);
Expand All @@ -203,9 +218,12 @@ void validateCompatibility(Object object,
continue;
}
String fullName = (parentName.isEmpty() ? "" : (parentName + ".")) + CaseFormat.UPPER_CAMEL.to(CaseFormat.LOWER_CAMEL,
method.getName()
.substring(3)
.toLowerCase());
method.getName().startsWith("is")
? method.getName()
.substring(2)
: method.getName()
.substring(3)
.toLowerCase());
// check that field is set
Object parameterValue;
try {
Expand Down Expand Up @@ -235,15 +253,15 @@ void validateCompatibility(Object object,
throw new ConflictException(String.format(FactoryConstants.PARAMETRIZED_INVALID_PARAMETER_MESSAGE, fullName, version));
} else {
// is parameter deprecated
if (factoryParameter.deprecatedSince().compareTo(version) <= 0 || factoryParameter.setByServer()) {
if (factoryParameter.deprecatedSince().compareTo(version) <= 0 || (!isUpdate && factoryParameter.setByServer())) {
throw new ConflictException(
String.format(FactoryConstants.PARAMETRIZED_INVALID_PARAMETER_MESSAGE, fullName, version));
}

// use recursion if parameter is DTO object
if (method.getReturnType().isAnnotationPresent(DTO.class)) {
// validate inner objects such Git ot ProjectAttributes
validateCompatibility(parameterValue, object, method.getReturnType(), method.getReturnType(), version, fullName);
validateCompatibility(parameterValue, object, method.getReturnType(), method.getReturnType(), version, fullName, isUpdate);
} else if (Map.class.isAssignableFrom(method.getReturnType())) {
Type tp = ((ParameterizedType)method.getGenericReturnType()).getActualTypeArguments()[1];

Expand All @@ -253,7 +271,7 @@ void validateCompatibility(Object object,
Map<Object, Object> map = (Map)parameterValue;
for (Map.Entry<Object, Object> entry : map.entrySet()) {
validateCompatibility(entry.getValue(), object, secMapParamClass, secMapParamClass, version,
fullName + "." + entry.getKey());
fullName + "." + entry.getKey(), isUpdate);
}
} else {
throw new RuntimeException("This type of fields is not supported by factory.");
Expand All @@ -267,7 +285,7 @@ void validateCompatibility(Object object,
if (secListParamClass.isAnnotationPresent(DTO.class)) {
List<Object> list = (List)parameterValue;
for (Object entry : list) {
validateCompatibility(entry, object, secListParamClass, secListParamClass, version, fullName);
validateCompatibility(entry, object, secListParamClass, secListParamClass, version, fullName, isUpdate);
}
} else {
throw new RuntimeException("This type of fields is not supported by factory.");
Expand Down
2 changes: 2 additions & 0 deletions ...core-api-factory/src/test/java/org/eclipse/che/api/factory/server/FactoryServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
import static org.eclipse.che.api.workspace.server.DtoConverter.asDto;
import static org.hamcrest.Matchers.equalTo;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyBoolean;
import static org.mockito.Matchers.anyInt;
import static org.mockito.Matchers.anyMap;
import static org.mockito.Matchers.anySetOf;
Expand Down Expand Up @@ -160,6 +161,7 @@ public void setUp() throws Exception {
dto = DtoFactory.getInstance();
factoryBuilder = spy(new FactoryBuilder(new SourceStorageParametersValidator()));
doNothing().when(factoryBuilder).checkValid(any(Factory.class));
doNothing().when(factoryBuilder).checkValid(any(Factory.class), anyBoolean());
when(factoryParametersResolverHolder.getFactoryParametersResolvers()).thenReturn(factoryParametersResolvers);
when(userDao.getById(anyString())).thenReturn(new UserImpl(null,
null,
Expand Down
6 changes: 6 additions & 0 deletions ...-factory/src/test/java/org/eclipse/che/api/factory/server/builder/FactoryBuilderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,12 @@ public void shouldNotAllowUsingParamsThatCanBeSetOnlyByServer(Factory factory)
factoryBuilder.checkValid(factory);
}

@Test(dataProvider = "setByServerParamsProvider")
public void shouldAllowUsingParamsThatCanBeSetOnlyByServerDuringUpdate(Factory factory)
throws InvocationTargetException, IllegalAccessException, ApiException, NoSuchMethodException {
factoryBuilder.checkValid(factory, true);
}

@DataProvider(name = "setByServerParamsProvider")
public static Object[][] setByServerParamsProvider() throws URISyntaxException, IOException, NoSuchMethodException {
Factory factory = prepareFactory();
Expand Down

0 comments on commit 60f4657

Please sign in to comment.