diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java b/zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java index dea55ac7c60..a0c8908d60e 100644 --- a/zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java +++ b/zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java @@ -898,27 +898,29 @@ public void updateNote(String noteId, return null; } } else { - AuthenticationInfo requestingAuth = new AuthenticationInfo((String)config.get("cronExecutingUser"),(String) config.get("cronExecutingRoles"), null); + if (config.get("cron") != null) { + AuthenticationInfo requestingAuth = new AuthenticationInfo((String) config.get("cronExecutingUser"), (String) config.get("cronExecutingRoles"), null); - String requestCronUser = requestingAuth.getUser(); - Set requestCronRoles = requestingAuth.getRoles(); + String requestCronUser = requestingAuth.getUser(); + Set requestCronRoles = requestingAuth.getRoles(); - if (!authorizationService.hasRunPermission(Collections.singleton(requestCronUser), note.getId())) { - LOGGER.error("Wrong cronExecutingUser: {}", requestCronUser); - callback.onFailure(new IllegalArgumentException(requestCronUser), context); - return null; - } else { - // This part should be restarted but we need to prepare to notice who can be a cron user in advance - if (!context.getUserAndRoles().contains(requestCronUser)) { + if (!authorizationService.hasRunPermission(Collections.singleton(requestCronUser), note.getId())) { LOGGER.error("Wrong cronExecutingUser: {}", requestCronUser); callback.onFailure(new IllegalArgumentException(requestCronUser), context); return null; - } + } else { + // This part should be restarted but we need to prepare to notice who can be a cron user in advance + if (!context.getUserAndRoles().contains(requestCronUser)) { + LOGGER.error("Wrong cronExecutingUser: {}", requestCronUser); + callback.onFailure(new IllegalArgumentException(requestCronUser), context); + return null; + } - if (!context.getUserAndRoles().containsAll(requestCronRoles)) { - LOGGER.error("Wrong cronExecutingRoles: {}", requestCronRoles); - callback.onFailure(new IllegalArgumentException(requestCronRoles.toString()), context); - return null; + if (!context.getUserAndRoles().containsAll(requestCronRoles)) { + LOGGER.error("Wrong cronExecutingRoles: {}", requestCronRoles); + callback.onFailure(new IllegalArgumentException(requestCronRoles.toString()), context); + return null; + } } } @@ -927,17 +929,18 @@ public void updateNote(String noteId, config.remove("cron"); } } - boolean cronUpdated = isCronUpdated(config, note.getConfig()); - if (cronUpdated) { - schedulerService.refreshCron(note.getId()); - } - } + } + boolean cronUpdated = isCronUpdated(config, note.getConfig()); note.setName(name); note.setConfig(config); - notebook.updateNote(note, context.getAutheInfo()); callback.onSuccess(note, context); + // refresh cron scheduler after note update + if (cronUpdated) { + schedulerService.refreshCron(note.getId()); + } + return null; }); } diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java b/zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java index 8ba1087a9f5..be30a3cda89 100644 --- a/zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java +++ b/zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java @@ -39,6 +39,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import java.util.stream.IntStream; @@ -74,6 +75,8 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; +import org.junit.jupiter.api.TestInfo; + import com.google.gson.Gson; @@ -82,6 +85,7 @@ class NotebookServiceTest { private static NotebookService notebookService; private File notebookDir; + private File confDir; private SearchService searchService; private Notebook notebook; private ServiceContext context = @@ -93,11 +97,21 @@ class NotebookServiceTest { @BeforeEach - void setUp() throws Exception { + void setUp(TestInfo testInfo) throws Exception { notebookDir = Files.createTempDirectory("notebookDir").toAbsolutePath().toFile(); ZeppelinConfiguration zConf = ZeppelinConfiguration.load(); zConf.setProperty(ZeppelinConfiguration.ConfVars.ZEPPELIN_NOTEBOOK_DIR.getVarName(), notebookDir.getAbsolutePath()); + // enable cron for testNoteUpdate method + if ("testNoteUpdate()".equals(testInfo.getDisplayName())){ + confDir = Files.createTempDirectory("confDir").toAbsolutePath().toFile(); + zConf.setProperty(ZeppelinConfiguration.ConfVars.ZEPPELIN_CONF_DIR.getVarName(), + confDir.getAbsolutePath()); + zConf.setProperty(ZeppelinConfiguration.ConfVars.ZEPPELIN_NOTEBOOK_CRON_ENABLE.getVarName(), "true"); + String shiroPath = zConf.getAbsoluteDir(String.format("%s/shiro.ini", zConf.getConfDir())); + Files.createFile(new File(shiroPath).toPath()); + context.getUserAndRoles().add("test"); + } noteParser = new GsonNoteParser(zConf); ConfigStorage storage = ConfigStorage.createConfigStorage(zConf); NotebookRepo notebookRepo = new VFSNotebookRepo(); @@ -151,6 +165,9 @@ void setUp() throws Exception { @AfterEach void tearDown() { notebookDir.delete(); + if (confDir != null){ + confDir.delete(); + } searchService.close(); } @@ -393,6 +410,64 @@ void testNoteOperations() throws IOException { assertEquals(0, notesInfo.size()); } + @Test + void testNoteUpdate() throws IOException { + // create note + String note1Id = notebookService.createNote("/folder_update/note_test_update", "test", true, context, callback); + // update note with cron for every 5 minutes + reset(callback); + Map updatedConfig = new HashMap<>(); + updatedConfig.put("isZeppelinNotebookCronEnable", true); + updatedConfig.put("looknfeel", "looknfeel"); + updatedConfig.put("personalizedMode", "false"); + updatedConfig.put("cron", "0 0/5 * * * ?"); + updatedConfig.put("cronExecutingRoles", "[\"test\"]"); + updatedConfig.put("cronExecutingUser", "test"); + notebookService.updateNote(note1Id, "note_test_update", updatedConfig, context, callback); + notebook.processNote(note1Id, + note1 -> { + assertEquals("note_test_update", note1.getName()); + assertEquals("test", note1.getConfig().get("cronExecutingUser")); + assertEquals("[\"test\"]", note1.getConfig().get("cronExecutingRoles")); + assertEquals("0 0/5 * * * ?", note1.getConfig().get("cron")); + verify(callback).onSuccess(any(Note.class), any(ServiceContext.class)); + return null; + }); + // update note with cron for every 1 hour + reset(callback); + updatedConfig.put("cron", "0 0 0/1 * * ?"); + notebookService.updateNote(note1Id, "note_test_update", updatedConfig, context, callback); + notebook.processNote(note1Id, + note1 -> { + assertEquals("0 0 0/1 * * ?", note1.getConfig().get("cron")); + verify(callback).onSuccess(any(Note.class), any(ServiceContext.class)); + return null; + }); + // update note with wrong user + reset(callback); + updatedConfig.put("cronExecutingUser", "wrong_user"); + notebookService.updateNote(note1Id, "note_test_update", updatedConfig, context, callback); + notebook.processNote(note1Id, + note1 -> { + verify(callback).onFailure(any(IllegalArgumentException.class), any(ServiceContext.class)); + return null; + }); + // disable cron + reset(callback); + updatedConfig.put("cron", null); + updatedConfig.put("cronExecutingRoles", ""); + updatedConfig.put("cronExecutingUser", ""); + notebookService.updateNote(note1Id, "note_test_update", updatedConfig, context, callback); + notebook.processNote(note1Id, + note1 -> { + assertNull(note1.getConfig().get("cron")); + assertEquals("", note1.getConfig().get("cronExecutingRoles")); + assertEquals("", note1.getConfig().get("cronExecutingUser")); + verify(callback).onSuccess(any(Note.class), any(ServiceContext.class)); + return null; + }); + } + @Test void testRenameNoteRejectsDuplicate() throws IOException { String note1Id = notebookService.createNote("/folder/note1", "test", true, context, callback);