From 3c58472e7f7cd45cfbb43841648fba27364fa93a Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Sun, 24 Feb 2019 15:52:05 +0900 Subject: [PATCH 1/5] digdag push validate on the server side --- .../io/digdag/server/rs/ProjectResource.java | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java b/digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java index b0263ce7b1..848119a2e2 100644 --- a/digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java +++ b/digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java @@ -1,6 +1,8 @@ package io.digdag.server.rs; +import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.stream.Collectors; import java.net.URI; import java.time.Instant; @@ -74,6 +76,8 @@ import io.digdag.core.repository.StoredProject; import io.digdag.core.repository.StoredRevision; import io.digdag.core.repository.StoredWorkflowDefinition; +import io.digdag.core.repository.WorkflowDefinition; +import io.digdag.core.repository.WorkflowDefinitionList; import io.digdag.core.schedule.ScheduleStore; import io.digdag.core.schedule.ScheduleStoreManager; import io.digdag.core.schedule.SchedulerManager; @@ -82,9 +86,12 @@ import io.digdag.core.session.SessionStoreManager; import io.digdag.core.session.StoredSessionWithLastAttempt; import io.digdag.core.storage.ArchiveManager; +import io.digdag.core.workflow.Workflow; import io.digdag.core.workflow.WorkflowCompiler; +import io.digdag.core.workflow.WorkflowTask; import io.digdag.server.GenericJsonExceptionHandler; import io.digdag.spi.DirectDownloadHandle; +import io.digdag.spi.Scheduler; import io.digdag.spi.SecretControlStore; import io.digdag.spi.SecretControlStoreManager; import io.digdag.spi.SecretScopes; @@ -549,6 +556,28 @@ public RestProject putProject(@QueryParam("project") String name, @QueryParam("r if (md5Count.getCount() != contentLength) { throw new IllegalArgumentException("Content-Length header doesn't match with uploaded data size"); } + WorkflowDefinitionList defs = meta.getWorkflowList(); + for (WorkflowDefinition def : defs.get()) { + Workflow wf = compiler.compile(def.getName(), def.getConfig()); + + // validate workflow and schedule +// Set required = new HashSet<>(); + for (WorkflowTask task : wf.getTasks()) { + // raise an exception if task doesn't valid. + task.getConfig(); +// String require = config.getOptional("require>", String.class).orNull(); +// if (require != null && required.add(require)) { +// f.ln(" -> %s", require); +// } + } + Revision rev = Revision.builderFromArchive("check", meta, getUserInfo()) + .archiveType(ArchiveType.NONE) + .build(); + // raise an exception if "schedule:" is invalid. + srm.tryGetScheduler(rev, def); + + } + } ArchiveManager.Location location = From 3a2c3e97a0875a490b188fd041a427163f204f83 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Tue, 26 Feb 2019 22:02:55 +0900 Subject: [PATCH 2/5] Add integration tests --- .../java/acceptance/ValidateProjectIT.java | 87 +++++++++++++++++++ .../acceptance/schedule/invalid_schedule.dig | 8 ++ 2 files changed, 95 insertions(+) create mode 100644 digdag-tests/src/test/java/acceptance/ValidateProjectIT.java create mode 100644 digdag-tests/src/test/resources/acceptance/schedule/invalid_schedule.dig diff --git a/digdag-tests/src/test/java/acceptance/ValidateProjectIT.java b/digdag-tests/src/test/java/acceptance/ValidateProjectIT.java new file mode 100644 index 0000000000..5c4d3dedb7 --- /dev/null +++ b/digdag-tests/src/test/java/acceptance/ValidateProjectIT.java @@ -0,0 +1,87 @@ +package acceptance; + +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import utils.CommandStatus; +import utils.TemporaryDigdagServer; + +import java.nio.file.Path; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.is; +import static utils.TestUtils.copyResource; +import static utils.TestUtils.main; + +// +// This file doesn't contain normal case. +// It defined in another test. +// +public class ValidateProjectIT +{ + @Rule + public TemporaryFolder folder = new TemporaryFolder(); + + @Rule + public TemporaryDigdagServer server = TemporaryDigdagServer.builder() + .build(); + + private Path config; + private Path projectDir; + + @Before + public void setUp() + throws Exception + { + projectDir = folder.getRoot().toPath().resolve("foobar"); + config = folder.newFile().toPath(); + } + + @Test + public void uploadInvalidTaskProject() + throws Exception + { + // Create new project + CommandStatus initStatus = main("init", + "-c", config.toString(), + projectDir.toString()); + assertThat(initStatus.code(), is(0)); + + copyResource("acceptance/error_task/invalid_at_group.dig", projectDir.resolve("invalid_at_group.dig")); + + // Push the project + CommandStatus pushStatus = main( + "push", + "--project", projectDir.toString(), + "foobar", + "-c", config.toString(), + "-e", server.endpoint()); + assertThat(pushStatus.code(), is(1)); + assertThat(pushStatus.errUtf8(), containsString("A task can't have more than one operator")); + } + + @Test + public void uploadInvalidScheduleProject() + throws Exception + { + // Create new project + CommandStatus initStatus = main("init", + "-c", config.toString(), + projectDir.toString()); + assertThat(initStatus.code(), is(0)); + + copyResource("acceptance/schedule/invalid_schedule.dig", projectDir.resolve("invalid_schedule.dig")); + + // Push the project + CommandStatus pushStatus = main( + "push", + "--project", projectDir.toString(), + "foobar", + "-c", config.toString(), + "-e", server.endpoint()); + assertThat(pushStatus.code(), is(1)); + assertThat(pushStatus.errUtf8(), containsString("scheduler requires mm:ss format")); + } +} diff --git a/digdag-tests/src/test/resources/acceptance/schedule/invalid_schedule.dig b/digdag-tests/src/test/resources/acceptance/schedule/invalid_schedule.dig new file mode 100644 index 0000000000..b076f2140c --- /dev/null +++ b/digdag-tests/src/test/resources/acceptance/schedule/invalid_schedule.dig @@ -0,0 +1,8 @@ +timezone: UTC + +schedule: + hourly>: "10:11:12" # correct format is "MM:SS" + ++foo: + sh>: "touch foo.out" + From a50f2c85e86c3cc6638987b449973c092da094ad Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Tue, 12 Mar 2019 13:23:49 +0900 Subject: [PATCH 3/5] Remove unnesessary comments --- .../src/main/java/io/digdag/server/rs/ProjectResource.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java b/digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java index 848119a2e2..90a29c0ed8 100644 --- a/digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java +++ b/digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java @@ -561,14 +561,9 @@ public RestProject putProject(@QueryParam("project") String name, @QueryParam("r Workflow wf = compiler.compile(def.getName(), def.getConfig()); // validate workflow and schedule -// Set required = new HashSet<>(); for (WorkflowTask task : wf.getTasks()) { // raise an exception if task doesn't valid. task.getConfig(); -// String require = config.getOptional("require>", String.class).orNull(); -// if (require != null && required.add(require)) { -// f.ln(" -> %s", require); -// } } Revision rev = Revision.builderFromArchive("check", meta, getUserInfo()) .archiveType(ArchiveType.NONE) From b696beabb2b5045196b935b3ce6cb2db115c80e6 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Tue, 12 Mar 2019 13:33:12 +0900 Subject: [PATCH 4/5] Move validation into a separate method --- .../io/digdag/server/rs/ProjectResource.java | 37 +++++++++++-------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java b/digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java index 90a29c0ed8..04eb65534e 100644 --- a/digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java +++ b/digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java @@ -556,23 +556,8 @@ public RestProject putProject(@QueryParam("project") String name, @QueryParam("r if (md5Count.getCount() != contentLength) { throw new IllegalArgumentException("Content-Length header doesn't match with uploaded data size"); } - WorkflowDefinitionList defs = meta.getWorkflowList(); - for (WorkflowDefinition def : defs.get()) { - Workflow wf = compiler.compile(def.getName(), def.getConfig()); - - // validate workflow and schedule - for (WorkflowTask task : wf.getTasks()) { - // raise an exception if task doesn't valid. - task.getConfig(); - } - Revision rev = Revision.builderFromArchive("check", meta, getUserInfo()) - .archiveType(ArchiveType.NONE) - .build(); - // raise an exception if "schedule:" is invalid. - srm.tryGetScheduler(rev, def); - - } + validateWorkflowAndSchedule(meta); } ArchiveManager.Location location = @@ -705,6 +690,26 @@ private void validateTarEntry(TarArchiveEntry entry) } } + private void validateWorkflowAndSchedule(ArchiveMetadata meta) + { + WorkflowDefinitionList defs = meta.getWorkflowList(); + for (WorkflowDefinition def : defs.get()) { + Workflow wf = compiler.compile(def.getName(), def.getConfig()); + + // validate workflow and schedule + for (WorkflowTask task : wf.getTasks()) { + // raise an exception if task doesn't valid. + task.getConfig(); + } + Revision rev = Revision.builderFromArchive("check", meta, getUserInfo()) + .archiveType(ArchiveType.NONE) + .build(); + // raise an exception if "schedule:" is invalid. + srm.tryGetScheduler(rev, def); + + } + } + @PUT @Consumes("application/json") @Path("/api/projects/{id}/secrets/{key}") From ff9783ad72b7a2e7c33667fa24fe0415779128d7 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Tue, 12 Mar 2019 16:10:18 +0900 Subject: [PATCH 5/5] Remove unused imports --- .../src/main/java/io/digdag/server/rs/ProjectResource.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java b/digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java index 04eb65534e..8e2e47e24c 100644 --- a/digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java +++ b/digdag-server/src/main/java/io/digdag/server/rs/ProjectResource.java @@ -1,8 +1,6 @@ package io.digdag.server.rs; -import java.util.HashSet; import java.util.List; -import java.util.Set; import java.util.stream.Collectors; import java.net.URI; import java.time.Instant; @@ -91,7 +89,6 @@ import io.digdag.core.workflow.WorkflowTask; import io.digdag.server.GenericJsonExceptionHandler; import io.digdag.spi.DirectDownloadHandle; -import io.digdag.spi.Scheduler; import io.digdag.spi.SecretControlStore; import io.digdag.spi.SecretControlStoreManager; import io.digdag.spi.SecretScopes;