diff --git a/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/FactoryService.java b/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/FactoryService.java index 5ea0fd2a25..ae20c544a1 100644 --- a/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/FactoryService.java +++ b/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/FactoryService.java @@ -245,6 +245,7 @@ public class FactoryService extends Service { if (factory == null) { throw new BadRequestException("Not null factory required"); } + factoryBuilder.checkValid(factory); processDefaults(factory); createValidator.validateOnCreate(factory); final Factory storedFactory = factoryStore.getFactory(factoryStore.saveFactory(factory, null)); @@ -345,6 +346,7 @@ public class FactoryService extends Service { newFactory.setId(existingFactory.getId()); // validate the new content + factoryBuilder.checkValid(newFactory, true); createValidator.validateOnCreate(newFactory); // access granted, user can update the factory diff --git a/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/ValueHelper.java b/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/ValueHelper.java index 0e2c01840c..99ee0445dd 100644 --- a/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/ValueHelper.java +++ b/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/ValueHelper.java @@ -13,6 +13,8 @@ package org.eclipse.che.api.factory.server; import java.util.Collection; import java.util.Map; +import static com.google.common.base.Strings.isNullOrEmpty; + /** * @author Sergii Kabashniuk */ @@ -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())); } } diff --git a/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/builder/FactoryBuilder.java b/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/builder/FactoryBuilder.java index 3806438f29..127d656c24 100644 --- a/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/builder/FactoryBuilder.java +++ b/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/builder/FactoryBuilder.java @@ -116,13 +116,27 @@ public class FactoryBuilder { } /** - * 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); } @@ -145,7 +159,7 @@ public class FactoryBuilder { default: throw new ConflictException(FactoryConstants.INVALID_VERSION_MESSAGE); } - validateCompatibility(factory, null, Factory.class, usedFactoryVersionMethodProvider, v, ""); + validateCompatibility(factory, null, Factory.class, usedFactoryVersionMethodProvider, v, "", isUpdate); } /** @@ -170,7 +184,7 @@ public class FactoryBuilder { * * @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} @@ -188,7 +202,8 @@ public class FactoryBuilder { 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); @@ -203,9 +218,12 @@ public class FactoryBuilder { 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 { @@ -235,7 +253,7 @@ public class FactoryBuilder { 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)); } @@ -243,7 +261,7 @@ public class FactoryBuilder { // 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]; @@ -253,7 +271,7 @@ public class FactoryBuilder { Map map = (Map)parameterValue; for (Map.Entry 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."); @@ -267,7 +285,7 @@ public class FactoryBuilder { if (secListParamClass.isAnnotationPresent(DTO.class)) { List 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."); diff --git a/wsmaster/che-core-api-factory/src/test/java/org/eclipse/che/api/factory/server/FactoryServiceTest.java b/wsmaster/che-core-api-factory/src/test/java/org/eclipse/che/api/factory/server/FactoryServiceTest.java index 27d8acfd64..32c1e05561 100644 --- a/wsmaster/che-core-api-factory/src/test/java/org/eclipse/che/api/factory/server/FactoryServiceTest.java +++ b/wsmaster/che-core-api-factory/src/test/java/org/eclipse/che/api/factory/server/FactoryServiceTest.java @@ -89,6 +89,7 @@ import static org.eclipse.che.api.factory.server.FactoryService.VALIDATE_QUERY_P 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; @@ -160,6 +161,7 @@ public class FactoryServiceTest { 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, diff --git a/wsmaster/che-core-api-factory/src/test/java/org/eclipse/che/api/factory/server/builder/FactoryBuilderTest.java b/wsmaster/che-core-api-factory/src/test/java/org/eclipse/che/api/factory/server/builder/FactoryBuilderTest.java index 7229265b09..13a06a7fe6 100644 --- a/wsmaster/che-core-api-factory/src/test/java/org/eclipse/che/api/factory/server/builder/FactoryBuilderTest.java +++ b/wsmaster/che-core-api-factory/src/test/java/org/eclipse/che/api/factory/server/builder/FactoryBuilderTest.java @@ -14,6 +14,8 @@ import com.google.common.collect.ImmutableMap; import org.eclipse.che.api.core.ApiException; import org.eclipse.che.api.core.factory.FactoryParameter; +import org.eclipse.che.api.core.model.machine.Limits; +import org.eclipse.che.api.core.model.workspace.Environment; import org.eclipse.che.api.factory.server.impl.SourceStorageParametersValidator; import org.eclipse.che.api.factory.shared.dto.Action; import org.eclipse.che.api.factory.shared.dto.Author; @@ -26,6 +28,7 @@ import org.eclipse.che.api.factory.shared.dto.OnAppLoaded; import org.eclipse.che.api.factory.shared.dto.OnProjectsLoaded; import org.eclipse.che.api.factory.shared.dto.Policies; import org.eclipse.che.api.machine.shared.dto.CommandDto; +import org.eclipse.che.api.machine.shared.dto.LimitsDto; import org.eclipse.che.api.machine.shared.dto.MachineConfigDto; import org.eclipse.che.api.machine.shared.dto.MachineSourceDto; import org.eclipse.che.api.machine.shared.dto.ServerConfDto; @@ -45,6 +48,7 @@ import org.testng.annotations.Test; import java.io.IOException; import java.lang.reflect.InvocationTargetException; import java.net.URISyntaxException; +import java.util.Collections; import static java.util.Arrays.asList; import static java.util.Collections.singletonList; @@ -102,6 +106,12 @@ public class FactoryBuilderTest { 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(); @@ -122,7 +132,30 @@ public class FactoryBuilderTest { @DataProvider(name = "notValidParamsProvider") public static Object[][] notValidParamsProvider() throws URISyntaxException, IOException, NoSuchMethodException { - return new Object[][] {}; + Factory factory = prepareFactory(); + EnvironmentDto environmentDto = factory.getWorkspace().getEnvironments().get(0); + environmentDto.getMachineConfigs().add(dto.createDto(MachineConfigDto.class).withType(null)); + return new Object[][] { + {dto.clone(factory).withWorkspace(factory.getWorkspace().withDefaultEnv(null)) }, + {dto.clone(factory).withWorkspace(factory.getWorkspace().withEnvironments(Collections.singletonList(environmentDto))) } + }; + } + + @Test(dataProvider = "sameAsDefaultParamsProvider") + public void shouldAllowUsingParamsWIthValueLikeDefaultForType(Factory factory) + throws InvocationTargetException, IllegalAccessException, ApiException, NoSuchMethodException { + factoryBuilder.checkValid(factory); + } + + @DataProvider(name = "sameAsDefaultParamsProvider") + public static Object[][] sameAsDefaultParamsProvider() throws URISyntaxException, IOException, NoSuchMethodException { + Factory factory = prepareFactory(); + EnvironmentDto environmentDto = factory.getWorkspace().getEnvironments().get(0); + environmentDto.getMachineConfigs().get(0).withDev(false) + .withLimits(dto.newDto(LimitsDto.class).withRam(0)); + return new Object[][] { + {dto.clone(factory).withWorkspace(factory.getWorkspace().withEnvironments(Collections.singletonList(environmentDto))) } + }; }