From 270e21a57c34672ee59f39dae964a59d6127c536 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 4 Jun 2019 12:17:01 +0200 Subject: [PATCH 1/5] /workspace/devfile now validates the devfile against schema and workspace manager validates the integrity of the devfile before creating the workspace. Signed-off-by: Lukas Krejci --- .../LimitsCheckingWorkspaceManager.java | 12 ++++-- .../WorkspacePermissionsFilterTest.java | 2 +- .../server/urlfactory/URLFactoryBuilder.java | 2 +- .../urlfactory/URLFactoryBuilderTest.java | 2 +- .../workspace/server/WorkspaceManager.java | 20 ++++++++- .../workspace/server/WorkspaceService.java | 42 +++++++++++++++---- .../server/devfile/DevfileManager.java | 29 +++++++++++-- .../server/devfile/DevfileService.java | 2 +- .../devfile/URLFileContentProvider.java | 3 +- .../validator/DevfileSchemaValidator.java | 40 ++++++++++++------ .../server/WorkspaceManagerTest.java | 10 ++++- .../server/WorkspaceServiceTest.java | 10 +++-- .../server/devfile/DevfileManagerTest.java | 14 +++---- .../server/devfile/DevfileServiceTest.java | 2 +- .../validator/DevfileSchemaValidatorTest.java | 4 +- 15 files changed, 147 insertions(+), 47 deletions(-) diff --git a/multiuser/api/che-multiuser-api-resource/src/main/java/org/eclipse/che/multiuser/resource/api/workspace/LimitsCheckingWorkspaceManager.java b/multiuser/api/che-multiuser-api-resource/src/main/java/org/eclipse/che/multiuser/resource/api/workspace/LimitsCheckingWorkspaceManager.java index 06d9a1e7d9b..031b75d623b 100644 --- a/multiuser/api/che-multiuser-api-resource/src/main/java/org/eclipse/che/multiuser/resource/api/workspace/LimitsCheckingWorkspaceManager.java +++ b/multiuser/api/che-multiuser-api-resource/src/main/java/org/eclipse/che/multiuser/resource/api/workspace/LimitsCheckingWorkspaceManager.java @@ -35,7 +35,7 @@ import org.eclipse.che.api.workspace.server.WorkspaceManager; import org.eclipse.che.api.workspace.server.WorkspaceRuntimes; import org.eclipse.che.api.workspace.server.WorkspaceValidator; -import org.eclipse.che.api.workspace.server.devfile.convert.DevfileConverter; +import org.eclipse.che.api.workspace.server.devfile.validator.DevfileIntegrityValidator; import org.eclipse.che.api.workspace.server.model.impl.WorkspaceConfigImpl; import org.eclipse.che.api.workspace.server.model.impl.WorkspaceImpl; import org.eclipse.che.api.workspace.server.spi.WorkspaceDao; @@ -84,8 +84,14 @@ public LimitsCheckingWorkspaceManager( EnvironmentRamCalculator environmentRamCalculator, ResourceManager resourceManager, ResourcesLocks resourcesLocks, - DevfileConverter devfileConverter) { - super(workspaceDao, runtimes, eventService, accountManager, workspaceValidator); + DevfileIntegrityValidator devfileIntegrityValidator) { + super( + workspaceDao, + runtimes, + eventService, + accountManager, + workspaceValidator, + devfileIntegrityValidator); this.environmentRamCalculator = environmentRamCalculator; this.maxRamPerEnvMB = "-1".equals(maxRamPerEnv) ? -1 : Size.parseSizeToMegabytes(maxRamPerEnv); this.resourceManager = resourceManager; diff --git a/multiuser/permission/che-multiuser-permission-workspace/src/test/java/org/eclipse/che/multiuser/permission/workspace/server/filters/WorkspacePermissionsFilterTest.java b/multiuser/permission/che-multiuser-permission-workspace/src/test/java/org/eclipse/che/multiuser/permission/workspace/server/filters/WorkspacePermissionsFilterTest.java index 3e4cc98c080..726a8aa5d81 100644 --- a/multiuser/permission/che-multiuser-permission-workspace/src/test/java/org/eclipse/che/multiuser/permission/workspace/server/filters/WorkspacePermissionsFilterTest.java +++ b/multiuser/permission/che-multiuser-permission-workspace/src/test/java/org/eclipse/che/multiuser/permission/workspace/server/filters/WorkspacePermissionsFilterTest.java @@ -158,7 +158,7 @@ public void shouldCheckAccountPermissionsAccessOnWorkspaceCreationFromDevfile() .post(SECURE_PATH + "/workspace/devfile?namespace=userok"); assertEquals(response.getStatusCode(), 204); - verify(workspaceService).create(any(DevfileDto.class), any(), any(), eq("userok")); + verify(workspaceService).create(anyString(), any(), any(), eq("userok"), any()); verify(permissionsFilter).checkAccountPermissions("userok", AccountOperation.CREATE_WORKSPACE); verifyZeroInteractions(subject); } diff --git a/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/urlfactory/URLFactoryBuilder.java b/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/urlfactory/URLFactoryBuilder.java index dae832fa6cf..1cd93be15a3 100644 --- a/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/urlfactory/URLFactoryBuilder.java +++ b/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/urlfactory/URLFactoryBuilder.java @@ -104,7 +104,7 @@ public Optional createFactoryFromDevfile( return Optional.empty(); } try { - DevfileImpl devfile = devfileManager.parse(devfileYamlContent); + DevfileImpl devfile = devfileManager.parseYaml(devfileYamlContent); WorkspaceConfigImpl wsConfig = devfileManager.createWorkspaceConfig(devfile, fileContentProvider); FactoryDto factoryDto = diff --git a/wsmaster/che-core-api-factory/src/test/java/org/eclipse/che/api/factory/server/urlfactory/URLFactoryBuilderTest.java b/wsmaster/che-core-api-factory/src/test/java/org/eclipse/che/api/factory/server/urlfactory/URLFactoryBuilderTest.java index 8dabceb1101..303a245f61f 100644 --- a/wsmaster/che-core-api-factory/src/test/java/org/eclipse/che/api/factory/server/urlfactory/URLFactoryBuilderTest.java +++ b/wsmaster/che-core-api-factory/src/test/java/org/eclipse/che/api/factory/server/urlfactory/URLFactoryBuilderTest.java @@ -119,7 +119,7 @@ public void checkWithCustomDevfileAndRecipe() throws Exception { workspaceConfigImpl.setDefaultEnv("name"); when(urlFetcher.fetchSafely(anyString())).thenReturn("random_content"); - when(devfileManager.parse(anyString())).thenReturn(devfile); + when(devfileManager.parseYaml(anyString())).thenReturn(devfile); when(devfileManager.createWorkspaceConfig(any(DevfileImpl.class), any())) .thenReturn(workspaceConfigImpl); diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java index 336ecc4db22..0816086b99e 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java @@ -42,6 +42,9 @@ import org.eclipse.che.api.core.model.workspace.WorkspaceStatus; import org.eclipse.che.api.core.model.workspace.devfile.Devfile; import org.eclipse.che.api.core.notification.EventService; +import org.eclipse.che.api.workspace.server.devfile.FileContentProvider; +import org.eclipse.che.api.workspace.server.devfile.exception.DevfileFormatException; +import org.eclipse.che.api.workspace.server.devfile.validator.DevfileIntegrityValidator; import org.eclipse.che.api.workspace.server.model.impl.WorkspaceConfigImpl; import org.eclipse.che.api.workspace.server.model.impl.WorkspaceImpl; import org.eclipse.che.api.workspace.server.model.impl.devfile.DevfileImpl; @@ -73,6 +76,7 @@ public class WorkspaceManager { private final AccountManager accountManager; private final EventService eventService; private final WorkspaceValidator validator; + private final DevfileIntegrityValidator devfileIntegrityValidator; @Inject public WorkspaceManager( @@ -80,12 +84,14 @@ public WorkspaceManager( WorkspaceRuntimes runtimes, EventService eventService, AccountManager accountManager, - WorkspaceValidator validator) { + WorkspaceValidator validator, + DevfileIntegrityValidator devfileIntegrityValidator) { this.workspaceDao = workspaceDao; this.runtimes = runtimes; this.accountManager = accountManager; this.eventService = eventService; this.validator = validator; + this.devfileIntegrityValidator = devfileIntegrityValidator; } /** @@ -122,7 +128,10 @@ public WorkspaceImpl createWorkspace( @Traced public WorkspaceImpl createWorkspace( - Devfile devfile, String namespace, Map attributes) + Devfile devfile, + String namespace, + Map attributes, + FileContentProvider contentProvider) throws ServerException, NotFoundException, ConflictException, ValidationException { TracingTags.STACK_ID.set(() -> attributes.getOrDefault("stackId", "no stack")); @@ -130,6 +139,13 @@ public WorkspaceImpl createWorkspace( requireNonNull(namespace, "Required non-null namespace"); validator.validateAttributes(attributes); + try { + devfileIntegrityValidator.validateDevfile(devfile); + devfileIntegrityValidator.validateContentReferences(devfile, contentProvider); + } catch (DevfileFormatException e) { + throw new ValidationException(e.getMessage(), e); + } + WorkspaceImpl workspace = doCreateWorkspace(devfile, accountManager.getByName(namespace), attributes, false); TracingTags.WORKSPACE_ID.set(workspace.getId()); diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceService.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceService.java index 2cb73de3968..1b8aab83e24 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceService.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceService.java @@ -50,6 +50,8 @@ import javax.ws.rs.PathParam; import javax.ws.rs.Produces; import javax.ws.rs.QueryParam; +import javax.ws.rs.core.Context; +import javax.ws.rs.core.HttpHeaders; import javax.ws.rs.core.Response; import org.eclipse.che.api.core.BadRequestException; import org.eclipse.che.api.core.ConflictException; @@ -62,10 +64,16 @@ import org.eclipse.che.api.core.model.workspace.Workspace; import org.eclipse.che.api.core.model.workspace.config.ServerConfig; import org.eclipse.che.api.core.rest.Service; +import org.eclipse.che.api.workspace.server.devfile.DevfileManager; +import org.eclipse.che.api.workspace.server.devfile.FileContentProvider; +import org.eclipse.che.api.workspace.server.devfile.URLFetcher; +import org.eclipse.che.api.workspace.server.devfile.URLFileContentProvider; +import org.eclipse.che.api.workspace.server.devfile.exception.DevfileException; import org.eclipse.che.api.workspace.server.model.impl.CommandImpl; import org.eclipse.che.api.workspace.server.model.impl.EnvironmentImpl; import org.eclipse.che.api.workspace.server.model.impl.ProjectConfigImpl; import org.eclipse.che.api.workspace.server.model.impl.WorkspaceImpl; +import org.eclipse.che.api.workspace.server.model.impl.devfile.DevfileImpl; import org.eclipse.che.api.workspace.server.token.MachineAccessForbidden; import org.eclipse.che.api.workspace.server.token.MachineTokenException; import org.eclipse.che.api.workspace.server.token.MachineTokenProvider; @@ -79,7 +87,6 @@ import org.eclipse.che.api.workspace.shared.dto.ServerDto; import org.eclipse.che.api.workspace.shared.dto.WorkspaceConfigDto; import org.eclipse.che.api.workspace.shared.dto.WorkspaceDto; -import org.eclipse.che.api.workspace.shared.dto.devfile.DevfileDto; import org.eclipse.che.commons.annotation.Nullable; import org.eclipse.che.commons.env.EnvironmentContext; @@ -100,6 +107,8 @@ public class WorkspaceService extends Service { private final String devfileRegistryUrl; private final String apiEndpoint; private final boolean cheWorkspaceAutoStart; + private final FileContentProvider devfileContentProvider; + private final DevfileManager devfileManager; @Inject public WorkspaceService( @@ -109,7 +118,9 @@ public WorkspaceService( MachineTokenProvider machineTokenProvider, WorkspaceLinksGenerator linksGenerator, @Named(CHE_WORKSPACE_PLUGIN_REGISTRY_URL_PROPERTY) @Nullable String pluginRegistryUrl, - @Named(CHE_WORKSPACE_DEVFILE_REGISTRY_URL_PROPERTY) @Nullable String devfileRegistryUrl) { + @Named(CHE_WORKSPACE_DEVFILE_REGISTRY_URL_PROPERTY) @Nullable String devfileRegistryUrl, + URLFetcher urlFetcher, + DevfileManager devfileManager) { this.apiEndpoint = apiEndpoint; this.cheWorkspaceAutoStart = cheWorkspaceAutoStart; this.workspaceManager = workspaceManager; @@ -117,6 +128,8 @@ public WorkspaceService( this.linksGenerator = linksGenerator; this.pluginRegistryUrl = pluginRegistryUrl; this.devfileRegistryUrl = devfileRegistryUrl; + this.devfileContentProvider = new URLFileContentProvider(null, urlFetcher); + this.devfileManager = devfileManager; } @POST @@ -188,14 +201,14 @@ public Response create( @Beta @Path("/devfile") @POST - @Consumes(APPLICATION_JSON) + @Consumes({APPLICATION_JSON, "text/yaml", "text/x-yaml"}) @Produces(APPLICATION_JSON) @ApiOperation( value = "Creates a new workspace based on the Devfile.", notes = "This method is in beta phase. It's strongly recommended to use `POST /devfile` instead" + " to get a workspace from Devfile. Workspaces created with this method are not stable yet.", - consumes = APPLICATION_JSON, + consumes = "application/json, text/yaml, text/x-yaml", produces = APPLICATION_JSON, nickname = "createFromDevfile", response = WorkspaceConfigDto.class) @@ -211,8 +224,7 @@ public Response create( @ApiResponse(code = 500, message = "Internal server error occurred") }) public Response create( - @ApiParam(value = "The configuration to create the new workspace", required = true) - DevfileDto devfile, + @ApiParam(value = "The devfile of the workspace to create", required = true) String devfile, @ApiParam( value = "Workspace attribute defined in 'attrName:attrValue' format. " @@ -231,11 +243,23 @@ public Response create( @DefaultValue("false") Boolean startAfterCreate, @ApiParam("Namespace where workspace should be created") @QueryParam("namespace") - String namespace) + String namespace, + @Context HttpHeaders headers) throws ConflictException, BadRequestException, ForbiddenException, NotFoundException, ServerException { requiredNotNull(devfile, "Devfile"); + DevfileImpl devfileModel; + try { + if (headers.getRequestHeader(HttpHeaders.CONTENT_TYPE).contains(APPLICATION_JSON)) { + devfileModel = devfileManager.parseJson(devfile); + } else { + devfileModel = devfileManager.parseYaml(devfile); + } + } catch (DevfileException e) { + throw new BadRequestException(e.getMessage()); + } + final Map attributes = parseAttrs(attrsList); if (namespace == null) { @@ -244,7 +268,9 @@ public Response create( WorkspaceImpl workspace; try { - workspace = workspaceManager.createWorkspace(devfile, namespace, attributes); + workspace = + workspaceManager.createWorkspace( + devfileModel, namespace, attributes, devfileContentProvider); } catch (ValidationException x) { throw new BadRequestException(x.getMessage()); } diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/DevfileManager.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/DevfileManager.java index 2fc4f27ba5a..d04883556d9 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/DevfileManager.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/DevfileManager.java @@ -82,7 +82,7 @@ public DevfileManager( } /** - * Creates {@link DevfileImpl} from given devfile content. Performs schema and integrity + * Creates {@link DevfileImpl} from given devfile content in YAML. Performs schema and integrity * validation of input data. * * @param devfileContent raw content of devfile @@ -90,8 +90,26 @@ public DevfileManager( * @throws DevfileFormatException when any of schema or integrity validations fail * @throws DevfileFormatException when any yaml parsing error occurs */ - public DevfileImpl parse(String devfileContent) throws DevfileFormatException { - JsonNode parsed = schemaValidator.validateBySchema(devfileContent); + public DevfileImpl parseYaml(String devfileContent) throws DevfileFormatException { + return parse(devfileContent, schemaValidator::validateYaml); + } + + /** + * Creates {@link DevfileImpl} from given devfile content in JSON. Performs schema and integrity + * validation of input data. + * + * @param devfileContent raw content of devfile + * @return Devfile object created from the source content + * @throws DevfileFormatException when any of schema or integrity validations fail + * @throws DevfileFormatException when any yaml parsing error occurs + */ + public DevfileImpl parseJson(String devfileContent) throws DevfileFormatException { + return parse(devfileContent, schemaValidator::validateJson); + } + + private DevfileImpl parse(String content, ValidationFunction validationFunction) + throws DevfileFormatException { + JsonNode parsed = validationFunction.validate(content); DevfileImpl devfile; try { @@ -188,4 +206,9 @@ private WorkspaceConfigImpl findAvailableName(WorkspaceConfigImpl config) throws } return config; } + + @FunctionalInterface + private interface ValidationFunction { + JsonNode validate(String content) throws DevfileFormatException; + } } diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/DevfileService.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/DevfileService.java index 4f3b1968ba2..7bddbb1fa1b 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/DevfileService.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/DevfileService.java @@ -120,7 +120,7 @@ public Response createFromYaml(String data) WorkspaceImpl workspace; try { - DevfileImpl devfile = devfileManager.parse(data); + DevfileImpl devfile = devfileManager.parseYaml(data); workspace = devfileManager.createWorkspace(devfile, urlFileContentProvider); } catch (DevfileException e) { throw new BadRequestException(e.getMessage()); diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/URLFileContentProvider.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/URLFileContentProvider.java index 20d96d5f829..a5588dd84b2 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/URLFileContentProvider.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/URLFileContentProvider.java @@ -17,6 +17,7 @@ import java.net.URI; import java.net.URISyntaxException; import org.eclipse.che.api.workspace.server.devfile.exception.DevfileException; +import org.eclipse.che.commons.annotation.Nullable; /** * A simple implementation of the FileContentProvider that merely uses the function resolve relative @@ -27,7 +28,7 @@ public class URLFileContentProvider implements FileContentProvider { private final URI devfileLocation; private final URLFetcher urlFetcher; - public URLFileContentProvider(URI devfileLocation, URLFetcher urlFetcher) { + public URLFileContentProvider(@Nullable URI devfileLocation, URLFetcher urlFetcher) { this.devfileLocation = devfileLocation; this.urlFetcher = urlFetcher; } diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/validator/DevfileSchemaValidator.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/validator/DevfileSchemaValidator.java index eb8ab9b920d..dbea4a20b13 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/validator/DevfileSchemaValidator.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/validator/DevfileSchemaValidator.java @@ -35,15 +35,15 @@ public class DevfileSchemaValidator { private final JsonValidationService service = JsonValidationService.newInstance(); - private ObjectMapper yamlReader; - private ObjectMapper jsonWriter; + private ObjectMapper yamlMapper; + private ObjectMapper jsonMapper; private JsonSchema schema; private ErrorMessageComposer errorMessageComposer; @Inject public DevfileSchemaValidator(DevfileSchemaProvider schemaProvider) { - this.yamlReader = new ObjectMapper(new YAMLFactory()); - this.jsonWriter = new ObjectMapper(); + this.yamlMapper = new ObjectMapper(new YAMLFactory()); + this.jsonMapper = new ObjectMapper(); this.errorMessageComposer = new ErrorMessageComposer(); try { this.schema = service.readSchema(schemaProvider.getAsReader()); @@ -52,23 +52,39 @@ public DevfileSchemaValidator(DevfileSchemaProvider schemaProvider) { } } - public JsonNode validateBySchema(String yamlContent) throws DevfileFormatException { + public JsonNode validateYaml(String yamlContent) throws DevfileFormatException { + return validate(yamlContent, yamlMapper); + } + + public JsonNode validateJson(String jsonContent) throws DevfileFormatException { + return validate(jsonContent, jsonMapper); + } + + private JsonNode validate(String content, ObjectMapper mapper) throws DevfileFormatException { JsonNode contentNode; try { - contentNode = yamlReader.readTree(yamlContent); + contentNode = mapper.readTree(content); + validate(contentNode); + return contentNode; + } catch (IOException e) { + throw new DevfileFormatException("Unable to validate Devfile. Error: " + e.getMessage()); + } + } + + private void validate(JsonNode contentNode) throws DevfileFormatException { + try { List validationErrors = new ArrayList<>(); ProblemHandler handler = ProblemHandler.collectingTo(validationErrors); try (JsonReader reader = service.createReader( - new StringReader(jsonWriter.writeValueAsString(contentNode)), schema, handler)) { + new StringReader(jsonMapper.writeValueAsString(contentNode)), schema, handler)) { reader.read(); } - if (validationErrors.isEmpty()) { - return contentNode; + if (!validationErrors.isEmpty()) { + String error = errorMessageComposer.extractMessages(validationErrors, new StringBuilder()); + throw new DevfileFormatException( + format("Devfile schema validation failed. Error: %s", error)); } - String error = errorMessageComposer.extractMessages(validationErrors, new StringBuilder()); - throw new DevfileFormatException( - format("Devfile schema validation failed. Error: %s", error)); } catch (IOException e) { throw new DevfileFormatException("Unable to validate Devfile. Error: " + e.getMessage()); } diff --git a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java index 00a9a976d50..81f3c898c35 100644 --- a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java +++ b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java @@ -73,6 +73,7 @@ import org.eclipse.che.api.core.model.workspace.runtime.MachineStatus; import org.eclipse.che.api.core.notification.EventService; import org.eclipse.che.api.workspace.server.devfile.convert.DevfileConverter; +import org.eclipse.che.api.workspace.server.devfile.validator.DevfileIntegrityValidator; import org.eclipse.che.api.workspace.server.model.impl.CommandImpl; import org.eclipse.che.api.workspace.server.model.impl.EnvironmentImpl; import org.eclipse.che.api.workspace.server.model.impl.MachineConfigImpl; @@ -120,6 +121,7 @@ public class WorkspaceManagerTest { @Mock private EventService eventService; @Mock private WorkspaceValidator validator; @Mock private DevfileConverter devfileConverter; + @Mock private DevfileIntegrityValidator devfileIntegrityValidator; @Captor private ArgumentCaptor workspaceCaptor; @@ -128,7 +130,13 @@ public class WorkspaceManagerTest { @BeforeMethod public void setUp() throws Exception { workspaceManager = - new WorkspaceManager(workspaceDao, runtimes, eventService, accountManager, validator); + new WorkspaceManager( + workspaceDao, + runtimes, + eventService, + accountManager, + validator, + devfileIntegrityValidator); lenient() .when(accountManager.getByName(NAMESPACE_1)) .thenReturn(new AccountImpl("accountId", NAMESPACE_1, "test")); diff --git a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceServiceTest.java b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceServiceTest.java index a383b0f7a20..f5a9856fb46 100644 --- a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceServiceTest.java +++ b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceServiceTest.java @@ -137,7 +137,9 @@ public void setup() { machineTokenProvider, linksGenerator, CHE_WORKSPACE_PLUGIN_REGISTRY_ULR, - CHE_WORKSPACE_DEVFILE_REGISTRY_ULR); + CHE_WORKSPACE_DEVFILE_REGISTRY_ULR, + null, + Tnull); } @Test @@ -180,7 +182,8 @@ public void shouldCreateWorkspaceFromConfig() throws Exception { public void shouldCreateWorkspaceFromDevfile() throws Exception { final DevfileDto devfileDto = createDevfileDto(); final WorkspaceImpl workspace = createWorkspace(devfileDto); - when(wsManager.createWorkspace(any(Devfile.class), anyString(), any())).thenReturn(workspace); + when(wsManager.createWorkspace(any(Devfile.class), anyString(), any(), any())) + .thenReturn(workspace); final Response response = given() @@ -208,7 +211,8 @@ public void shouldCreateWorkspaceFromDevfile() throws Exception { ImmutableMap.of( "stackId", "stack123", "factoryId", "factory123", - "custom", "custom:value"))); + "custom", "custom:value")), + any()); } @Test diff --git a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/devfile/DevfileManagerTest.java b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/devfile/DevfileManagerTest.java index 1a499efd0e3..76393e8d58e 100644 --- a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/devfile/DevfileManagerTest.java +++ b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/devfile/DevfileManagerTest.java @@ -78,18 +78,18 @@ public class DevfileManagerTest { public void setUp() throws Exception { devfile = new DevfileImpl(); - lenient().when(schemaValidator.validateBySchema(any())).thenReturn(devfileJsonNode); + lenient().when(schemaValidator.validateYaml(any())).thenReturn(devfileJsonNode); lenient().when(objectMapper.treeToValue(any(), eq(DevfileImpl.class))).thenReturn(devfile); } @Test public void testValidateAndParse() throws Exception { // when - DevfileImpl parsed = devfileManager.parse(DEVFILE_YAML_CONTENT); + DevfileImpl parsed = devfileManager.parseYaml(DEVFILE_YAML_CONTENT); // then assertEquals(parsed, devfile); - verify(schemaValidator).validateBySchema(DEVFILE_YAML_CONTENT); + verify(schemaValidator).validateYaml(DEVFILE_YAML_CONTENT); verify(objectMapper).treeToValue(devfileJsonNode, DevfileImpl.class); verify(integrityValidator).validateDevfile(devfile); } @@ -106,7 +106,7 @@ public void testInitializingDevfileMapsAfterParsing() throws Exception { devfile.getComponents().add(component); // when - DevfileImpl parsed = devfileManager.parse(DEVFILE_YAML_CONTENT); + DevfileImpl parsed = devfileManager.parseYaml(DEVFILE_YAML_CONTENT); // then assertNotNull(parsed.getCommands().get(0).getAttributes()); @@ -119,10 +119,10 @@ public void testInitializingDevfileMapsAfterParsing() throws Exception { expectedExceptionsMessageRegExp = "non valid") public void shouldThrowExceptionWhenExceptionOccurredDuringSchemaValidation() throws Exception { // given - doThrow(new DevfileFormatException("non valid")).when(schemaValidator).validateBySchema(any()); + doThrow(new DevfileFormatException("non valid")).when(schemaValidator).validateYaml(any()); // when - devfileManager.parse(DEVFILE_YAML_CONTENT); + devfileManager.parseYaml(DEVFILE_YAML_CONTENT); } @Test( @@ -135,7 +135,7 @@ public void shouldThrowExceptionWhenErrorOccurredDuringDevfileParsing() throws E doThrow(jsonException).when(objectMapper).treeToValue(any(), any()); // when - devfileManager.parse(DEVFILE_YAML_CONTENT); + devfileManager.parseYaml(DEVFILE_YAML_CONTENT); } @Test diff --git a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/devfile/DevfileServiceTest.java b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/devfile/DevfileServiceTest.java index 001a83669e8..d1f771706a2 100644 --- a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/devfile/DevfileServiceTest.java +++ b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/devfile/DevfileServiceTest.java @@ -89,7 +89,7 @@ public void shouldAcceptDevFileContentAndCreateWorkspace() throws Exception { Files.readFile(getClass().getClassLoader().getResourceAsStream("devfile/devfile.yaml")); DevfileImpl devfile = createDevfile(yamlContent); WorkspaceImpl ws = createWorkspace(WorkspaceStatus.STOPPED); - when(devfileManager.parse(anyString())).thenReturn(devfile); + when(devfileManager.parseYaml(anyString())).thenReturn(devfile); when(devfileManager.createWorkspace(any(DevfileImpl.class), any())).thenReturn(ws); final Response response = given() diff --git a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/devfile/validator/DevfileSchemaValidatorTest.java b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/devfile/validator/DevfileSchemaValidatorTest.java index 055d85726e0..a54db770741 100644 --- a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/devfile/validator/DevfileSchemaValidatorTest.java +++ b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/devfile/validator/DevfileSchemaValidatorTest.java @@ -35,7 +35,7 @@ public void setUp() { @Test(dataProvider = "validDevfiles") public void shouldNotThrowExceptionOnValidationOfValidDevfile(String resourceFilePath) throws Exception { - schemaValidator.validateBySchema(getResource(resourceFilePath)); + schemaValidator.validateYaml(getResource(resourceFilePath)); } @DataProvider @@ -69,7 +69,7 @@ public Object[][] validDevfiles() { public void shouldThrowExceptionOnValidationOfNonValidDevfile( String resourceFilePath, String expectedMessage) throws Exception { try { - schemaValidator.validateBySchema(getResource(resourceFilePath)); + schemaValidator.validateYaml(getResource(resourceFilePath)); } catch (DevfileFormatException e) { assertEquals( e.getMessage(), From 7c5994d239ccd82b5785dac516bfdf71ff30595f Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 4 Jun 2019 15:38:53 +0200 Subject: [PATCH 2/5] Whoops, a typo.. Signed-off-by: Lukas Krejci --- .../eclipse/che/api/workspace/server/WorkspaceServiceTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceServiceTest.java b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceServiceTest.java index f5a9856fb46..68aad4fc4c4 100644 --- a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceServiceTest.java +++ b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceServiceTest.java @@ -139,7 +139,7 @@ public void setup() { CHE_WORKSPACE_PLUGIN_REGISTRY_ULR, CHE_WORKSPACE_DEVFILE_REGISTRY_ULR, null, - Tnull); + null); } @Test From 0dfe0feaa191ca70c5cbcd60f414a59ff34d1f97 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 4 Jun 2019 18:23:41 +0200 Subject: [PATCH 3/5] Fix content type detection and implement a couple of tests for the devfile validation during workspace creation. Signed-off-by: Lukas Krejci --- .../workspace/server/WorkspaceService.java | 3 +- .../server/WorkspaceManagerTest.java | 32 +++++ .../server/WorkspaceServiceTest.java | 112 +++++++++++++++--- .../che/core/db/jpa/CascadeRemovalTest.java | 6 + 4 files changed, 137 insertions(+), 16 deletions(-) diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceService.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceService.java index 1b8aab83e24..e92072b964b 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceService.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceService.java @@ -251,7 +251,8 @@ public Response create( DevfileImpl devfileModel; try { - if (headers.getRequestHeader(HttpHeaders.CONTENT_TYPE).contains(APPLICATION_JSON)) { + if (headers.getMediaType().getType().equals("application") + && headers.getMediaType().getSubtype().equals("json")) { devfileModel = devfileManager.parseJson(devfile); } else { devfileModel = devfileManager.parseYaml(devfile); diff --git a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java index 81f3c898c35..a7e30f84fb6 100644 --- a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java +++ b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java @@ -61,6 +61,7 @@ import org.eclipse.che.api.core.NotFoundException; import org.eclipse.che.api.core.Page; import org.eclipse.che.api.core.ServerException; +import org.eclipse.che.api.core.ValidationException; import org.eclipse.che.api.core.model.workspace.Runtime; import org.eclipse.che.api.core.model.workspace.Warning; import org.eclipse.che.api.core.model.workspace.Workspace; @@ -73,6 +74,7 @@ import org.eclipse.che.api.core.model.workspace.runtime.MachineStatus; import org.eclipse.che.api.core.notification.EventService; import org.eclipse.che.api.workspace.server.devfile.convert.DevfileConverter; +import org.eclipse.che.api.workspace.server.devfile.exception.DevfileFormatException; import org.eclipse.che.api.workspace.server.devfile.validator.DevfileIntegrityValidator; import org.eclipse.che.api.workspace.server.model.impl.CommandImpl; import org.eclipse.che.api.workspace.server.model.impl.EnvironmentImpl; @@ -587,6 +589,36 @@ public void removesTemporaryWorkspaceAfterStartFailed() throws Exception { verify(workspaceDao, times(1)).remove(anyString()); } + @Test(expectedExceptions = ValidationException.class, expectedExceptionsMessageRegExp = "boom") + public void shouldFailToCreateWorkspaceUsingInvalidDevfile() throws Exception { + // given + doThrow(new DevfileFormatException("boom")) + .when(devfileIntegrityValidator) + .validateDevfile(any()); + + Devfile devfile = mock(Devfile.class); + + // when + workspaceManager.createWorkspace(devfile, "ns", emptyMap(), null); + + // then exception is thrown + } + + @Test(expectedExceptions = ValidationException.class, expectedExceptionsMessageRegExp = "boom") + public void shouldFailTocreateWorkspaceUsingInconsistentDevfile() throws Exception { + // given + doThrow(new DevfileFormatException("boom")) + .when(devfileIntegrityValidator) + .validateContentReferences(any(), any()); + + Devfile devfile = mock(Devfile.class); + + // when + workspaceManager.createWorkspace(devfile, "ns", emptyMap(), null); + + // then exception is thrown + } + private void mockRuntimeStatus(WorkspaceImpl workspace, WorkspaceStatus status) { lenient().when(runtimes.getStatus(workspace.getId())).thenReturn(status); } diff --git a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceServiceTest.java b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceServiceTest.java index 68aad4fc4c4..4b9902cd499 100644 --- a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceServiceTest.java +++ b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceServiceTest.java @@ -60,6 +60,10 @@ import org.eclipse.che.api.core.model.workspace.runtime.ServerStatus; import org.eclipse.che.api.core.rest.ApiExceptionMapper; import org.eclipse.che.api.core.rest.shared.dto.ServiceError; +import org.eclipse.che.api.workspace.server.devfile.DevfileManager; +import org.eclipse.che.api.workspace.server.devfile.URLFetcher; +import org.eclipse.che.api.workspace.server.devfile.exception.DevfileFormatException; +import org.eclipse.che.api.workspace.server.dto.DtoServerImpls.DevfileDtoImpl; import org.eclipse.che.api.workspace.server.model.impl.CommandImpl; import org.eclipse.che.api.workspace.server.model.impl.EnvironmentImpl; import org.eclipse.che.api.workspace.server.model.impl.MachineConfigImpl; @@ -69,6 +73,7 @@ import org.eclipse.che.api.workspace.server.model.impl.ServerImpl; import org.eclipse.che.api.workspace.server.model.impl.WorkspaceConfigImpl; import org.eclipse.che.api.workspace.server.model.impl.WorkspaceImpl; +import org.eclipse.che.api.workspace.server.model.impl.devfile.DevfileImpl; import org.eclipse.che.api.workspace.server.token.MachineTokenProvider; import org.eclipse.che.api.workspace.shared.Constants; import org.eclipse.che.api.workspace.shared.dto.CommandDto; @@ -124,6 +129,8 @@ public class WorkspaceServiceTest { @Mock private WorkspaceManager wsManager; @Mock private MachineTokenProvider machineTokenProvider; @Mock private WorkspaceLinksGenerator linksGenerator; + @Mock private DevfileManager devfileManager; + @Mock private URLFetcher urlFetcher; private WorkspaceService service; @@ -138,8 +145,8 @@ public void setup() { linksGenerator, CHE_WORKSPACE_PLUGIN_REGISTRY_ULR, CHE_WORKSPACE_DEVFILE_REGISTRY_ULR, - null, - null); + urlFetcher, + devfileManager); } @Test @@ -180,8 +187,11 @@ public void shouldCreateWorkspaceFromConfig() throws Exception { @Test public void shouldCreateWorkspaceFromDevfile() throws Exception { - final DevfileDto devfileDto = createDevfileDto(); + final DevfileDtoImpl devfileDto = createDevfileDto(); final WorkspaceImpl workspace = createWorkspace(devfileDto); + + when(devfileManager.parseJson(any())).thenReturn(new DevfileImpl()); + when(wsManager.createWorkspace(any(Devfile.class), anyString(), any(), any())) .thenReturn(workspace); @@ -190,7 +200,7 @@ public void shouldCreateWorkspaceFromDevfile() throws Exception { .auth() .basic(ADMIN_USER_NAME, ADMIN_USER_PASSWORD) .contentType("application/json") - .body(devfileDto) + .body(devfileDto.toJson()) .when() .post( SECURE_PATH @@ -215,6 +225,77 @@ public void shouldCreateWorkspaceFromDevfile() throws Exception { any()); } + @Test + public void shouldAcceptYamlDevfileWhenCreatingWorkspace() throws Exception { + final DevfileDtoImpl devfileDto = createDevfileDto(); + final WorkspaceImpl workspace = createWorkspace(devfileDto); + + when(devfileManager.parseYaml(any())).thenReturn(new DevfileImpl()); + + when(wsManager.createWorkspace(any(Devfile.class), anyString(), any(), any())) + .thenReturn(workspace); + + final Response response = + given() + .auth() + .basic(ADMIN_USER_NAME, ADMIN_USER_PASSWORD) + .contentType("text/yaml") + .when() + .post( + SECURE_PATH + + "/workspace/devfile" + + "?namespace=test" + + "&attribute=stackId:stack123" + + "&attribute=factoryId:factory123" + + "&attribute=custom:custom:value"); + + assertEquals(response.getStatusCode(), 201); + assertEquals( + new WorkspaceImpl(unwrapDto(response, WorkspaceDto.class), TEST_ACCOUNT), workspace); + verify(wsManager) + .createWorkspace( + any(Devfile.class), + eq("test"), + eq( + ImmutableMap.of( + "stackId", "stack123", + "factoryId", "factory123", + "custom", "custom:value")), + any()); + } + + @Test + public void shouldReturnBadRequestOnInvalidDevfile() throws Exception { + final DevfileDtoImpl devfileDto = createDevfileDto(); + final WorkspaceImpl workspace = createWorkspace(devfileDto); + + when(devfileManager.parseJson(any())).thenThrow(new DevfileFormatException("boom")); + + when(wsManager.createWorkspace(any(Devfile.class), anyString(), any(), any())) + .thenReturn(workspace); + + final Response response = + given() + .auth() + .basic(ADMIN_USER_NAME, ADMIN_USER_PASSWORD) + .contentType("application/json") + .body(devfileDto.toJson()) + .when() + .post( + SECURE_PATH + + "/workspace/devfile" + + "?namespace=test" + + "&attribute=stackId:stack123" + + "&attribute=factoryId:factory123" + + "&attribute=custom:custom:value"); + + assertEquals(response.getStatusCode(), 400); + String error = unwrapError(response); + assertEquals(error, "boom"); + + verify(wsManager, never()).createWorkspace(any(Devfile.class), any(), any(), any()); + } + @Test public void shouldUseUsernameAsNamespaceWhenCreatingWorkspaceFromConfigWithoutSpecifiedNamespace() throws Exception { @@ -1270,17 +1351,18 @@ private WorkspaceImpl createWorkspace(DevfileDto devfileDto) { .build(); } - private DevfileDto createDevfileDto() { - return newDto(DevfileDto.class) - .withSpecVersion("0.0.1") - .withName("ws") - .withProjects( - singletonList( - newDto(ProjectDto.class) - .withName("project") - .withSource( - newDto(SourceDto.class) - .withLocation("https://github.com/eclipse/che.git")))); + private DevfileDtoImpl createDevfileDto() { + return (DevfileDtoImpl) + newDto(DevfileDto.class) + .withSpecVersion("0.0.1") + .withName("ws") + .withProjects( + singletonList( + newDto(ProjectDto.class) + .withName("project") + .withSource( + newDto(SourceDto.class) + .withLocation("https://github.com/eclipse/che.git")))); } private static WorkspaceImpl createWorkspace(WorkspaceConfig configDto) { diff --git a/wsmaster/integration-tests/cascade-removal/src/test/java/org/eclipse/che/core/db/jpa/CascadeRemovalTest.java b/wsmaster/integration-tests/cascade-removal/src/test/java/org/eclipse/che/core/db/jpa/CascadeRemovalTest.java index 4c8168a1178..cc592085d94 100644 --- a/wsmaster/integration-tests/cascade-removal/src/test/java/org/eclipse/che/core/db/jpa/CascadeRemovalTest.java +++ b/wsmaster/integration-tests/cascade-removal/src/test/java/org/eclipse/che/core/db/jpa/CascadeRemovalTest.java @@ -36,6 +36,7 @@ import com.google.inject.Guice; import com.google.inject.Injector; import com.google.inject.Stage; +import com.google.inject.multibindings.MapBinder; import com.google.inject.name.Names; import java.util.Map; import java.util.concurrent.Callable; @@ -71,6 +72,7 @@ import org.eclipse.che.api.workspace.server.WorkspaceRuntimes; import org.eclipse.che.api.workspace.server.WorkspaceSharedPool; import org.eclipse.che.api.workspace.server.devfile.convert.DevfileConverter; +import org.eclipse.che.api.workspace.server.devfile.validator.ComponentIntegrityValidator; import org.eclipse.che.api.workspace.server.hc.probe.ProbeScheduler; import org.eclipse.che.api.workspace.server.jpa.JpaWorkspaceDao.RemoveWorkspaceBeforeAccountRemovedEventSubscriber; import org.eclipse.che.api.workspace.server.jpa.WorkspaceJpaModule; @@ -263,6 +265,10 @@ protected void configure() { bind(AccountManager.class); bind(WorkspaceSharedPool.class) .toInstance(new WorkspaceSharedPool("cached", null, null, null)); + + MapBinder.newMapBinder(binder(), String.class, ComponentIntegrityValidator.class) + .addBinding("kubernetes") + .toInstance(mock(ComponentIntegrityValidator.class)); } }); From c0dcb0ab4497c46724f7371020f645d3620b2d39 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Wed, 5 Jun 2019 08:08:46 +0200 Subject: [PATCH 4/5] Add javadoc, remove the superfluous validation step. Signed-off-by: Lukas Krejci --- .../workspace/server/WorkspaceManager.java | 19 ++++++++++++++++++- .../server/WorkspaceManagerTest.java | 15 --------------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java index 0816086b99e..039e55a4394 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java @@ -126,6 +126,24 @@ public WorkspaceImpl createWorkspace( return workspace; } + /** + * Creates a workspace out of a devfile. + * + *

The devfile should have been validated using the {@link + * DevfileIntegrityValidator#validateDevfile(Devfile)}. This method does rest of the validation + * and actually creates the workspace. + * + * @param devfile the devfile describing the workspace + * @param namespace workspace name is unique in this namespace + * @param attributes workspace instance attributes + * @return new workspace instance + * @throws NullPointerException when either {@code config} or {@code namespace} is null + * @throws NotFoundException when account with given id was not found + * @throws ConflictException when any conflict occurs (e.g Workspace with such name already exists + * for {@code owner}) + * @throws ServerException when any other error occurs + * @throws ValidationException when incoming configuration or attributes are not valid + */ @Traced public WorkspaceImpl createWorkspace( Devfile devfile, @@ -140,7 +158,6 @@ public WorkspaceImpl createWorkspace( validator.validateAttributes(attributes); try { - devfileIntegrityValidator.validateDevfile(devfile); devfileIntegrityValidator.validateContentReferences(devfile, contentProvider); } catch (DevfileFormatException e) { throw new ValidationException(e.getMessage(), e); diff --git a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java index a7e30f84fb6..a4f65213dc8 100644 --- a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java +++ b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java @@ -589,21 +589,6 @@ public void removesTemporaryWorkspaceAfterStartFailed() throws Exception { verify(workspaceDao, times(1)).remove(anyString()); } - @Test(expectedExceptions = ValidationException.class, expectedExceptionsMessageRegExp = "boom") - public void shouldFailToCreateWorkspaceUsingInvalidDevfile() throws Exception { - // given - doThrow(new DevfileFormatException("boom")) - .when(devfileIntegrityValidator) - .validateDevfile(any()); - - Devfile devfile = mock(Devfile.class); - - // when - workspaceManager.createWorkspace(devfile, "ns", emptyMap(), null); - - // then exception is thrown - } - @Test(expectedExceptions = ValidationException.class, expectedExceptionsMessageRegExp = "boom") public void shouldFailTocreateWorkspaceUsingInconsistentDevfile() throws Exception { // given From 55d095b72457b9c99b27eb798ea0e9c3165c569d Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Wed, 5 Jun 2019 12:44:44 +0200 Subject: [PATCH 5/5] Simplify the content type checking. Signed-off-by: Lukas Krejci --- .../workspace/server/WorkspaceManager.java | 2 ++ .../workspace/server/WorkspaceService.java | 20 +++++++++++++------ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java index 039e55a4394..13cb3666b9f 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java @@ -136,6 +136,8 @@ public WorkspaceImpl createWorkspace( * @param devfile the devfile describing the workspace * @param namespace workspace name is unique in this namespace * @param attributes workspace instance attributes + * @param contentProvider the content provider to use for resolving content references in the + * devfile * @return new workspace instance * @throws NullPointerException when either {@code config} or {@code namespace} is null * @throws NotFoundException when account with given id was not found diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceService.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceService.java index e92072b964b..f749250c1ca 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceService.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceService.java @@ -15,7 +15,9 @@ import static java.lang.String.format; import static java.util.Collections.emptyMap; import static java.util.stream.Collectors.toList; +import static javax.ws.rs.core.HttpHeaders.CONTENT_TYPE; import static javax.ws.rs.core.MediaType.APPLICATION_JSON; +import static javax.ws.rs.core.MediaType.APPLICATION_JSON_TYPE; import static org.eclipse.che.api.workspace.server.DtoConverter.asDto; import static org.eclipse.che.api.workspace.server.WorkspaceKeyValidator.validateKey; import static org.eclipse.che.api.workspace.shared.Constants.CHE_WORKSPACE_AUTO_START; @@ -44,14 +46,14 @@ import javax.ws.rs.DELETE; import javax.ws.rs.DefaultValue; import javax.ws.rs.GET; +import javax.ws.rs.HeaderParam; import javax.ws.rs.POST; import javax.ws.rs.PUT; import javax.ws.rs.Path; import javax.ws.rs.PathParam; import javax.ws.rs.Produces; import javax.ws.rs.QueryParam; -import javax.ws.rs.core.Context; -import javax.ws.rs.core.HttpHeaders; +import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import org.eclipse.che.api.core.BadRequestException; import org.eclipse.che.api.core.ConflictException; @@ -244,15 +246,14 @@ public Response create( Boolean startAfterCreate, @ApiParam("Namespace where workspace should be created") @QueryParam("namespace") String namespace, - @Context HttpHeaders headers) + @HeaderParam(CONTENT_TYPE) MediaType contentType) throws ConflictException, BadRequestException, ForbiddenException, NotFoundException, ServerException { requiredNotNull(devfile, "Devfile"); DevfileImpl devfileModel; try { - if (headers.getMediaType().getType().equals("application") - && headers.getMediaType().getSubtype().equals("json")) { + if (APPLICATION_JSON_TYPE.isCompatible(contentType)) { devfileModel = devfileManager.parseJson(devfile); } else { devfileModel = devfileManager.parseYaml(devfile); @@ -271,7 +272,14 @@ public Response create( try { workspace = workspaceManager.createWorkspace( - devfileModel, namespace, attributes, devfileContentProvider); + devfileModel, + namespace, + attributes, + // create a new cache for each request so that we don't have to care about lifetime + // of the cache, etc. The content is cached only for the duration of this call + // (i.e. all the validation and provisioning of the devfile will download each + // referenced file only once per request) + FileContentProvider.cached(devfileContentProvider)); } catch (ValidationException x) { throw new BadRequestException(x.getMessage()); }