diff --git a/core/platform-api/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/DefaultWorkspaceConfigValidator.java b/core/platform-api/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/DefaultWorkspaceConfigValidator.java index 9e83450bd1b4..49a826d96f2d 100644 --- a/core/platform-api/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/DefaultWorkspaceConfigValidator.java +++ b/core/platform-api/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/DefaultWorkspaceConfigValidator.java @@ -30,7 +30,7 @@ @Singleton public class DefaultWorkspaceConfigValidator implements WorkspaceConfigValidator { /* should contain [3, 20] characters, first and last character is letter or digit, available characters {A-Za-z0-9.-_}*/ - private static final Pattern WS_NAME = Pattern.compile("[\\w][\\w\\.\\-]{1,18}[\\w]"); + private static final Pattern WS_NAME = Pattern.compile("[a-zA-Z0-9][-_.a-zA-Z0-9]{1,18}[a-zA-Z0-9]"); /** * Checks that workspace configuration is valid. @@ -49,58 +49,67 @@ public class DefaultWorkspaceConfigValidator implements WorkspaceConfigValidator @Override public void validate(WorkspaceConfig config) throws BadRequestException { // configuration object itself - requiredNotNull(config.getName(), "Workspace name required"); - if (!WS_NAME.matcher(config.getName()).matches()) { - throw new BadRequestException("Incorrect workspace name, it must be between 3 and 20 characters and may contain digits, " + - "latin letters, underscores, dots, dashes and should start and end only with digits, " + - "latin letters or underscores"); - } + checkNotNull(config.getName(), "Workspace name required"); + checkArgument(WS_NAME.matcher(config.getName()).matches(), + "Incorrect workspace name, it must be between 3 and 20 characters and may contain digits, " + + "latin letters, underscores, dots, dashes and should start and end only with digits, " + + "latin letters or underscores"); //attributes for (String attributeName : config.getAttributes().keySet()) { //attribute name should not be empty and should not start with codenvy - if (attributeName.trim().isEmpty() || attributeName.toLowerCase().startsWith("codenvy")) { - throw new BadRequestException(format("Attribute name '%s' is not valid", attributeName)); - } + checkArgument(attributeName != null && !attributeName.trim().isEmpty() && !attributeName.toLowerCase().startsWith("codenvy"), + "Attribute name '%s' is not valid", + attributeName); } //environments - requiredNotNull(config.getDefaultEnv(), "Workspace default environment name required"); - if (!config.getEnvironments().stream().anyMatch(env -> env.getName().equals(config.getDefaultEnv()))) { - throw new BadRequestException("Workspace default environment configuration required"); - } + checkArgument(!isNullOrEmpty(config.getDefaultEnv()), "Workspace default environment name required"); + checkArgument(config.getEnvironments() + .stream() + .anyMatch(env -> config.getDefaultEnv().equals(env.getName())), + "Workspace default environment configuration required"); + for (Environment environment : config.getEnvironments()) { final String envName = environment.getName(); - requiredNotNull(envName, "Environment name should not be null"); + checkArgument(!isNullOrEmpty(envName), "Environment name should be neither null nor empty"); + + checkArgument(environment.getRecipe() == null || "docker".equals(environment.getRecipe().getType()), + "Couldn't start workspace '%s' from environment '%s', environment recipe has unsupported type '%s'", + config.getName(), + envName, + environment.getRecipe() != null ? environment.getRecipe().getType() : null); //machine configs - if (environment.getMachineConfigs().isEmpty()) { - throw new BadRequestException("Environment '" + envName + "' should contain at least 1 machine"); - } + checkArgument(!environment.getMachineConfigs().isEmpty(), "Environment '%s' should contain at least 1 machine", envName); + final long devCount = environment.getMachineConfigs() .stream() .filter(MachineConfig::isDev) .count(); - if (devCount != 1) { - throw new BadRequestException(format("Environment should contain exactly 1 dev machine, but '%s' contains '%d'", - envName, - devCount)); - } + checkArgument(devCount == 1, + "Environment should contain exactly 1 dev machine, but '%s' contains '%d'", + envName, + devCount); for (MachineConfig machineCfg : environment.getMachineConfigs()) { - if (isNullOrEmpty(machineCfg.getName())) { - throw new BadRequestException("Environment " + envName + " contains machine without of name"); - } - requiredNotNull(machineCfg.getSource(), "Environment " + envName + " contains machine without of source"); - //TODO require type? + checkArgument(!isNullOrEmpty(machineCfg.getName()), "Environment %s contains machine with null or empty name", envName); + checkNotNull(machineCfg.getSource(), "Environment " + envName + " contains machine without source"); + checkArgument("docker".equals(machineCfg.getType()), + "Type of machine %s in environment %s is not supported. Supported value is 'docker'.", + machineCfg.getName(), + envName); } } //commands for (Command command : config.getCommands()) { - requiredNotNull(command.getName(), "Workspace " + config.getName() + " contains command without of name"); - requiredNotNull(command.getCommandLine(), format("Command line required for command '%s' in workspace '%s'", - command.getName(), - config.getName())); + checkArgument(!isNullOrEmpty(command.getName()), + "Workspace %s contains command with null or empty name", + config.getName()); + checkArgument(!isNullOrEmpty(command.getCommandLine()), + "Command line required for command '%s' in workspace '%s'", + command.getName(), + config.getName()); } //projects @@ -111,9 +120,31 @@ public void validate(WorkspaceConfig config) throws BadRequestException { * Checks that object reference is not null, throws {@link BadRequestException} * in the case of null {@code object} with given {@code message}. */ - private void requiredNotNull(Object object, String message) throws BadRequestException { + private void checkNotNull(Object object, String message) throws BadRequestException { if (object == null) { throw new BadRequestException(message); } } + + /** + * Checks that expression is true, throws {@link BadRequestException} otherwise. + * + *
Exception uses error message built from error message template and error message parameters. + */ + private void checkArgument(boolean expression, String errorMessageTemplate, Object... errorMessageParams) throws BadRequestException { + if (!expression) { + throw new BadRequestException(format(errorMessageTemplate, errorMessageParams)); + } + } + + /** + * Checks that expression is true, throws {@link BadRequestException} otherwise. + * + *
Exception uses error message built from error message template and error message parameters. + */ + private void checkArgument(boolean expression, String errorMessage) throws BadRequestException { + if (!expression) { + throw new BadRequestException(errorMessage); + } + } } diff --git a/core/platform-api/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/RuntimeWorkspaceRegistry.java b/core/platform-api/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/RuntimeWorkspaceRegistry.java index e23d21243e8e..29a72ff4de15 100644 --- a/core/platform-api/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/RuntimeWorkspaceRegistry.java +++ b/core/platform-api/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/RuntimeWorkspaceRegistry.java @@ -38,7 +38,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; @@ -65,6 +64,9 @@ * second for owner -> list of workspaces mapping(which speeds up fetching workspaces by owner). * Maps are guarded by {@link ReentrantReadWriteLock}. * + *
The implementation doesn't validate parameters.
+ * They should be validated by caller of methods of this class.
+ *
* @author Yevhenii Voevodin
* @author Alexander Garagatyi
*/
@@ -120,7 +122,6 @@ public RuntimeWorkspaceImpl start(UsersWorkspace usersWorkspace, String envName,
BadRequestException,
NotFoundException {
checkRegistryIsNotStopped();
- checkWorkspaceIsValidForStart(usersWorkspace, envName);
// Prepare runtime workspace for start
final RuntimeWorkspaceImpl newRuntime = RuntimeWorkspaceImpl.builder()
.fromWorkspace(usersWorkspace)
@@ -367,43 +368,6 @@ private void startEnvironment(Environment environment, String workspaceId, boole
}
}
- /**
- * Checks if workspace is valid for start.
- */
- private void checkWorkspaceIsValidForStart(UsersWorkspace workspace, String envName) throws BadRequestException {
- if (workspace == null) {
- throw new BadRequestException("Required non-null workspace");
- }
- if (envName == null) {
- throw new BadRequestException(format("Couldn't start workspace '%s', environment name is null",
- workspace.getConfig().getName()));
- }
- final Optional extends Environment> environmentOptional = workspace.getConfig()
- .getEnvironments()
- .stream()
- .filter(env -> env.getName().equals(envName))
- .findFirst();
- if (!environmentOptional.isPresent()) {
- throw new BadRequestException(format("Couldn't start workspace '%s', workspace doesn't have environment '%s'",
- workspace.getConfig().getName(),
- envName));
- }
- Environment environment = environmentOptional.get();
- if (environment.getRecipe() != null && !"docker".equals(environment.getRecipe().getType())) {
- throw new BadRequestException(format("Couldn't start workspace '%s' from environment '%s', " +
- "environment recipe has unsupported type '%s'",
- workspace.getConfig().getName(),
- envName,
- environment.getRecipe().getType()));
- }
- if (findDev(environment.getMachineConfigs()) == null) {
- throw new BadRequestException(format("Couldn't start workspace '%s' from environment '%s', " +
- "environment doesn't contain dev-machine",
- workspace.getConfig().getName(),
- envName));
- }
- }
-
/**
* Adds given machine to the running workspace, if the workspace exists.
* Sets up this machine as dev-machine if it is dev.
diff --git a/core/platform-api/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java b/core/platform-api/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java
index 0f735311c20e..49b556db7647 100644
--- a/core/platform-api/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java
+++ b/core/platform-api/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java
@@ -399,11 +399,13 @@ public UsersWorkspaceImpl startWorkspaceByName(String workspaceName,
* @see WorkspaceHooks#beforeCreate(UsersWorkspace, String)
* @see RuntimeWorkspaceRegistry#start(UsersWorkspace, String)
*/
- public RuntimeWorkspaceImpl startTemporaryWorkspace(WorkspaceConfig workspaceConfig, @Nullable String accountId) throws ServerException,
- BadRequestException,
- ForbiddenException,
- NotFoundException,
- ConflictException {
+ public RuntimeWorkspaceImpl startTemporaryWorkspace(WorkspaceConfig workspaceConfig,
+ @Nullable String accountId) throws ServerException,
+ BadRequestException,
+ ForbiddenException,
+ NotFoundException,
+ ConflictException {
+
final UsersWorkspaceImpl workspace = fromConfig(workspaceConfig, getCurrentUserId());
workspace.setTemporary(true);
// Temporary workspace is not persistent one, which means
@@ -573,7 +575,9 @@ private UsersWorkspaceImpl fromConfig(WorkspaceConfig config, String owner) thro
UsersWorkspaceImpl performAsyncStart(UsersWorkspaceImpl workspace,
String envName,
boolean recover,
- @Nullable String accountId) throws ConflictException {
+ @Nullable String accountId) throws ConflictException,
+ BadRequestException {
+
// Runtime workspace registry performs this check as well
// but this check needed here because permanent workspace start performed asynchronously
// which means that even if registry won't start workspace client receives workspace object
@@ -588,6 +592,19 @@ UsersWorkspaceImpl performAsyncStart(UsersWorkspaceImpl workspace,
}
workspace.setTemporary(false);
workspace.setStatus(WorkspaceStatus.STARTING);
+
+ if (envName != null && !workspace.getConfig()
+ .getEnvironments()
+ .stream()
+ .filter(env -> env.getName().equals(envName))
+ .findFirst()
+ .isPresent()) {
+
+ throw new BadRequestException(format("Couldn't start workspace '%s', workspace doesn't have environment '%s'",
+ workspace.getConfig().getName(),
+ envName));
+ }
+
executor.execute(ThreadLocalPropagateContext.wrap(() -> {
try {
performSyncStart(workspace, firstNonNull(envName, workspace.getConfig().getDefaultEnv()), recover, accountId);
diff --git a/core/platform-api/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/DefaultWorkspaceConfigValidatorTest.java b/core/platform-api/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/DefaultWorkspaceConfigValidatorTest.java
index 7b6f530729af..388ff108bdf2 100644
--- a/core/platform-api/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/DefaultWorkspaceConfigValidatorTest.java
+++ b/core/platform-api/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/DefaultWorkspaceConfigValidatorTest.java
@@ -11,20 +11,24 @@
package org.eclipse.che.api.workspace.server;
import org.eclipse.che.api.core.BadRequestException;
-import org.eclipse.che.api.core.model.workspace.WorkspaceConfig;
+import org.eclipse.che.api.machine.shared.dto.CommandDto;
import org.eclipse.che.api.machine.shared.dto.MachineConfigDto;
import org.eclipse.che.api.machine.shared.dto.MachineSourceDto;
import org.eclipse.che.api.workspace.shared.dto.EnvironmentDto;
+import org.eclipse.che.api.workspace.shared.dto.RecipeDto;
import org.eclipse.che.api.workspace.shared.dto.WorkspaceConfigDto;
import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;
import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Optional;
import static java.util.Collections.singletonList;
+import static java.util.Collections.singletonMap;
import static org.eclipse.che.dto.server.DtoFactory.newDto;
-import static org.testng.Assert.assertNull;
-import static org.testng.AssertJUnit.assertNotNull;
/**
* Tests for {@link WorkspaceConfigValidator} and {@link DefaultWorkspaceConfigValidator}
@@ -41,52 +45,328 @@ public void prepare() throws Exception {
}
@Test
- public void shouldBeNotValidationHaveNotDevMachineInConfig() throws Exception {
- BadRequestException exResult = null;
- try {
- wsValidator.validate(createConfig("dev-workspace", false));
- } catch (BadRequestException e) {
- exResult = e;
- }
- assertNotNull(exResult);
+ public void shouldValidateCorrectWorkspace() throws Exception {
+ final WorkspaceConfigDto config = createConfig();
+
+
+ wsValidator.validate(config);
}
- @Test
- public void shouldBeValidationHaveDevMachineInConfig() throws Exception {
- Exception exResult = null;
- try {
- wsValidator.validate(createConfig("dev-workspace", true));
- } catch (BadRequestException e) {
- exResult = e;
- }
- assertNull(exResult);
+ @Test(expectedExceptions = BadRequestException.class,
+ expectedExceptionsMessageRegExp = "Workspace name required")
+ public void shouldFailValidationIfNameIsNull() throws Exception {
+ final WorkspaceConfigDto config = createConfig();
+ config.withName(null);
+
+
+ wsValidator.validate(config);
}
- @Test
- public void shouldNotBeValidationHaveDevMachineInConfigButWsNameNull() throws Exception {
- Exception exResult = null;
- try {
- wsValidator.validate(createConfig(null, true));
- } catch (BadRequestException e) {
- exResult = e;
- }
- assertNotNull(exResult);
+ @Test(dataProvider = "invalidNameProvider",
+ expectedExceptions = BadRequestException.class,
+ expectedExceptionsMessageRegExp = "Incorrect workspace name, it must be between 3 and 20 characters and may contain digits, " +
+ "latin letters, underscores, dots, dashes and should start and end only with digits, " +
+ "latin letters or underscores")
+ public void shouldFailValidationIfNameIsInvalid(String name) throws Exception {
+ final WorkspaceConfigDto config = createConfig();
+ config.withName(name);
+
+
+ wsValidator.validate(config);
+ }
+
+ @DataProvider(name = "invalidNameProvider")
+ public static Object[][] invalidNameProvider() {
+ return new Object[][] {
+ {".name"},
+ {"name."},
+ {"-name"},
+ {"name-"},
+ {"long-name12345678901234567890"},
+ {"_name"},
+ {"name_"}
+ };
+ }
+
+ @Test(dataProvider = "validNameProvider")
+ public void shouldValidateCorrectWorkspaceName(String name) throws Exception {
+ final WorkspaceConfigDto config = createConfig();
+ config.withName(name);
+
+
+ wsValidator.validate(config);
+ }
+
+ @DataProvider(name = "validNameProvider")
+ public static Object[][] validNameProvider() {
+ return new Object[][] {
+ {"name"},
+ {"quiteLongName1234567"},
+ {"name-with-dashes"},
+ {"name.with.dots"},
+ {"name0with1digits"},
+ {"mixed-symbols.name12"},
+ {"123456"},
+ {"name_name"},
+ {"123-456.78"}
+ };
+ }
+
+ @Test(expectedExceptions = BadRequestException.class,
+ expectedExceptionsMessageRegExp = "Attribute name 'null' is not valid")
+ public void shouldFailValidationIfAttributeNameIsNull() throws Exception {
+ final WorkspaceConfigDto config = createConfig();
+ config.getAttributes()
+ .put(null, "value1");
+
+
+ wsValidator.validate(config);
+ }
+
+ @Test(expectedExceptions = BadRequestException.class,
+ expectedExceptionsMessageRegExp = "Attribute name '' is not valid")
+ public void shouldFailValidationIfAttributeNameIsEmpty() throws Exception {
+ final WorkspaceConfigDto config = createConfig();
+ config.getAttributes()
+ .put("", "value1");
+
+
+ wsValidator.validate(config);
+ }
+
+ @Test(expectedExceptions = BadRequestException.class,
+ expectedExceptionsMessageRegExp = "Attribute name '.*' is not valid")
+ public void shouldFailValidationIfAttributeNameStartsWithWordCodenvy() throws Exception {
+ final WorkspaceConfigDto config = createConfig();
+ config.getAttributes()
+ .put("codenvy_key", "value1");
+
+
+ wsValidator.validate(config);
+ }
+
+ @Test(expectedExceptions = BadRequestException.class,
+ expectedExceptionsMessageRegExp = "Workspace default environment name required")
+ public void shouldFailValidationIfDefaultEnvNameIsNull() throws Exception {
+ final WorkspaceConfigDto config = createConfig();
+ config.setDefaultEnv(null);
+
+
+ wsValidator.validate(config);
+ }
+
+ @Test(expectedExceptions = BadRequestException.class,
+ expectedExceptionsMessageRegExp = "Workspace default environment name required")
+ public void shouldFailValidationIfDefaultEnvNameIsEmpty() throws Exception {
+ final WorkspaceConfigDto config = createConfig();
+ config.setDefaultEnv("");
+
+
+ wsValidator.validate(config);
+ }
+
+ @Test(expectedExceptions = BadRequestException.class,
+ expectedExceptionsMessageRegExp = "Workspace default environment configuration required")
+ public void shouldFailValidationIfEnvWithDefaultEnvNameIsNull() throws Exception {
+ final WorkspaceConfigDto config = createConfig();
+ config.setEnvironments(null);
+
+
+ wsValidator.validate(config);
+ }
+
+ @Test(expectedExceptions = BadRequestException.class,
+ expectedExceptionsMessageRegExp = "Environment name should be neither null nor empty")
+ public void shouldFailValidationIfEnvNameIsNull() throws Exception {
+ final WorkspaceConfigDto config = createConfig();
+ config.getEnvironments()
+ .add(newDto(EnvironmentDto.class).withName(null)
+ .withMachineConfigs(config.getEnvironments()
+ .get(0)
+ .getMachineConfigs()));
+
+
+ wsValidator.validate(config);
+ }
+
+ @Test(expectedExceptions = BadRequestException.class,
+ expectedExceptionsMessageRegExp = "Environment name should be neither null nor empty")
+ public void shouldFailValidationIfEnvNameIsEmpty() throws Exception {
+ final WorkspaceConfigDto config = createConfig();
+ config.getEnvironments()
+ .add(newDto(EnvironmentDto.class).withName("")
+ .withMachineConfigs(config.getEnvironments()
+ .get(0)
+ .getMachineConfigs()));
+
+
+ wsValidator.validate(config);
+ }
+
+ @Test(expectedExceptions = BadRequestException.class,
+ expectedExceptionsMessageRegExp = "Couldn't start workspace '.*' from environment '.*', environment recipe has unsupported type '.*'")
+ public void shouldFailValidationIfEnvironmentRecipeTypeIsNotDocker() throws Exception {
+ final WorkspaceConfigDto config = createConfig();
+ config.getEnvironments()
+ .get(0)
+ .withRecipe(newDto(RecipeDto.class).withType("kubernetes"));
+
+
+ wsValidator.validate(config);
}
@Test
- public void shouldNotBeValidationHaveNotDevMachineInConfigButWsNameNull() throws Exception {
- Exception exResult = null;
- try {
- wsValidator.validate(createConfig(null, false));
- } catch (BadRequestException e) {
- exResult = e;
- }
- assertNotNull(exResult);
+ public void shouldNotFailValidationIfEnvironmentRecipeTypeIsDocker() throws Exception {
+ final WorkspaceConfigDto config = createConfig();
+ config.getEnvironments()
+ .get(0)
+ .withRecipe(newDto(RecipeDto.class).withType("docker"));
+
+
+ wsValidator.validate(config);
+ }
+
+ @Test(expectedExceptions = BadRequestException.class,
+ expectedExceptionsMessageRegExp = "Environment '.*' should contain at least 1 machine")
+ public void shouldFailValidationIfMachinesListIsEmpty() throws Exception {
+ final WorkspaceConfigDto config = createConfig();
+ config.getEnvironments()
+ .get(0)
+ .withMachineConfigs(null);
+
+
+ wsValidator.validate(config);
+ }
+
+ @Test(expectedExceptions = BadRequestException.class,
+ expectedExceptionsMessageRegExp = "Environment should contain exactly 1 dev machine, but '.*' contains '0'")
+ public void shouldFailValidationIfNoDevMachineFound() throws Exception {
+ final WorkspaceConfigDto config = createConfig();
+ config.getEnvironments()
+ .get(0)
+ .getMachineConfigs()
+ .stream()
+ .filter(MachineConfigDto::isDev)
+ .forEach(machine -> machine.withDev(false));
+
+
+ wsValidator.validate(config);
+ }
+
+ @Test(expectedExceptions = BadRequestException.class,
+ expectedExceptionsMessageRegExp = "Environment should contain exactly 1 dev machine, but '.*' contains '2'")
+ public void shouldFailValidationIf2DevMachinesFound() throws Exception {
+ final WorkspaceConfigDto config = createConfig();
+ final Optional