You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@zeppelin.apache.org by zj...@apache.org on 2021/04/26 02:32:17 UTC
[zeppelin] branch branch-0.9 updated: [ZEPPELIN-4506] Check for
duplicate note on rename of a notebook
This is an automated email from the ASF dual-hosted git repository.
zjffdu pushed a commit to branch branch-0.9
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/branch-0.9 by this push:
new 01ce002 [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
01ce002 is described below
commit 01ce002040c646ca5a9b18782923d426b5dbb48b
Author: Danny Cranmer <da...@apache.org>
AuthorDate: Tue Apr 13 13:19:07 2021 +0100
[ZEPPELIN-4506] Check for duplicate note on rename of a notebook
### What is this PR for?
When a user renames a note Zeppelin does not check for existing notes with the same name/path. This can result in errors as multiple notes can have conflicting paths.
### What type of PR is it?
Bug Fix
### Todos
* [x] - Add validation to Java code
* [x] - Add java unit tests
### What is the Jira issue?
* [ZEPPELIN-4506](https://issues.apache.org/jira/browse/ZEPPELIN-4506)
### How should this be tested?
* Java unit tests
* Manual test
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
Author: Danny Cranmer <da...@apache.org>
Closes #4092 from dannycranmer/ZEPPELIN-4506 and squashes the following commits:
02701c784 [Danny Cranmer] [ZEPPELIN-4506] Renaming validation method inline with PR feedback
a7467d52f [Danny Cranmer] [ZEPPELIN-4506] Updates after PR feedback. Removing web changes in favour of server side only change
e264a6df0 [Danny Cranmer] [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
(cherry picked from commit 164e517bea65c0b145967afc159cf9971bc9a63e)
Signed-off-by: Jeff Zhang <zj...@apache.org>
---
.../apache/zeppelin/service/NotebookService.java | 9 ++++--
.../org/apache/zeppelin/socket/NotebookServer.java | 8 ++++++
.../zeppelin/service/NotebookServiceTest.java | 22 +++++++++++++++
zeppelin-web/README.md | 2 +-
.../org/apache/zeppelin/notebook/NoteManager.java | 32 ++++++++++++++++++++--
.../exception/NotePathAlreadyExistsException.java | 29 ++++++++++++++++++++
.../apache/zeppelin/notebook/NoteManagerTest.java | 32 ++++++++++++++++++++++
7 files changed, 128 insertions(+), 6 deletions(-)
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 a20036a..dd6bb08 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
@@ -48,6 +48,7 @@ import org.apache.zeppelin.notebook.NoteManager;
import org.apache.zeppelin.notebook.Notebook;
import org.apache.zeppelin.notebook.Paragraph;
import org.apache.zeppelin.notebook.AuthorizationService;
+import org.apache.zeppelin.notebook.exception.NotePathAlreadyExistsException;
import org.apache.zeppelin.notebook.repo.NotebookRepoWithVersionControl;
import org.apache.zeppelin.notebook.scheduler.SchedulerService;
import org.apache.zeppelin.common.Message;
@@ -256,8 +257,12 @@ public class NotebookService {
newNotePath = "/" + newNotePath;
}
}
- notebook.moveNote(noteId, newNotePath, context.getAutheInfo());
- callback.onSuccess(note, context);
+ try {
+ notebook.moveNote(noteId, newNotePath, context.getAutheInfo());
+ callback.onSuccess(note, context);
+ } catch (NotePathAlreadyExistsException e) {
+ callback.onFailure(e, context);
+ }
} else {
callback.onFailure(new NoteNotFoundException(noteId), context);
}
diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java b/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
index cfbfc51..0fd52f7 100644
--- a/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
+++ b/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
@@ -962,6 +962,14 @@ public class NotebookServer extends WebSocketServlet
broadcastNote(note);
broadcastNoteList(context.getAutheInfo(), context.getUserAndRoles());
}
+
+ @Override
+ public void onFailure(Exception ex, ServiceContext context) throws IOException {
+ super.onFailure(ex, context);
+
+ // If there was a failure, then resend the latest notebook information to update stale UI
+ broadcastNote(getNotebook().getNote(noteId));
+ }
});
}
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 4c2156a..7168bbd 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
@@ -28,6 +28,7 @@ import static org.mockito.Matchers.any;
import static org.mockito.Matchers.eq;
import static org.mockito.Mockito.doCallRealMethod;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
@@ -58,6 +59,7 @@ import org.apache.zeppelin.notebook.NoteInfo;
import org.apache.zeppelin.notebook.NoteManager;
import org.apache.zeppelin.notebook.Notebook;
import org.apache.zeppelin.notebook.Paragraph;
+import org.apache.zeppelin.notebook.exception.NotePathAlreadyExistsException;
import org.apache.zeppelin.notebook.repo.InMemoryNotebookRepo;
import org.apache.zeppelin.notebook.repo.NotebookRepo;
import org.apache.zeppelin.notebook.repo.VFSNotebookRepo;
@@ -332,6 +334,26 @@ public class NotebookServiceTest {
}
@Test
+ public void testRenameNoteRejectsDuplicate() throws IOException {
+ Note note1 = notebookService.createNote("/folder/note1", "test", true, context, callback);
+ assertEquals("note1", note1.getName());
+ verify(callback).onSuccess(note1, context);
+
+ reset(callback);
+ Note note2 = notebookService.createNote("/folder/note2", "test", true, context, callback);
+ assertEquals("note2", note2.getName());
+ verify(callback).onSuccess(note2, context);
+
+ reset(callback);
+ ArgumentCaptor<NotePathAlreadyExistsException> exception = ArgumentCaptor.forClass(NotePathAlreadyExistsException.class);
+ notebookService.renameNote(note1.getId(), "/folder/note2", false, context, callback);
+ verify(callback).onFailure(exception.capture(), any(ServiceContext.class));
+ assertEquals("Note '/folder/note2' existed", exception.getValue().getMessage());
+ verify(callback, never()).onSuccess(any(), any());
+ }
+
+
+ @Test
public void testParagraphOperations() throws IOException {
// create note
Note note1 = notebookService.createNote("note1", "python", false, context, callback);
diff --git a/zeppelin-web/README.md b/zeppelin-web/README.md
index 0194bd7..a269c09 100644
--- a/zeppelin-web/README.md
+++ b/zeppelin-web/README.md
@@ -39,7 +39,7 @@ $ WEB_PORT=YOUR_WEB_DEV_PORT yarn run dev
```sh
# running unit tests
-$ yarn run test
+$ yarn run karma-test
# running e2e tests: make sure that zeppelin instance started (localhost:8080)
$ yarn run e2e
diff --git a/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NoteManager.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NoteManager.java
index 779d173..484ee09 100644
--- a/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NoteManager.java
+++ b/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NoteManager.java
@@ -20,6 +20,7 @@ package org.apache.zeppelin.notebook;
import org.apache.commons.lang3.StringUtils;
import org.apache.zeppelin.conf.ZeppelinConfiguration;
+import org.apache.zeppelin.notebook.exception.NotePathAlreadyExistsException;
import org.apache.zeppelin.notebook.repo.NotebookRepo;
import org.apache.zeppelin.user.AuthenticationInfo;
import org.slf4j.Logger;
@@ -118,6 +119,11 @@ public class NoteManager {
private void addOrUpdateNoteNode(Note note, boolean checkDuplicates) throws IOException {
String notePath = note.getPath();
+
+ if (checkDuplicates && !isNotePathAvailable(notePath)) {
+ throw new NotePathAlreadyExistsException("Note '" + notePath + "' existed");
+ }
+
String[] tokens = notePath.split("/");
Folder curFolder = root;
for (int i = 0; i < tokens.length - 1; ++i) {
@@ -125,9 +131,7 @@ public class NoteManager {
curFolder = curFolder.getOrCreateFolder(tokens[i]);
}
}
- if (checkDuplicates && curFolder.containsNote(tokens[tokens.length - 1])) {
- throw new IOException("Note '" + note.getPath() + "' existed");
- }
+
curFolder.addNote(tokens[tokens.length -1], note);
this.notesInfo.put(note.getId(), note.getPath());
}
@@ -225,6 +229,10 @@ public class NoteManager {
throw new IOException("No metadata found for this note: " + noteId);
}
+ if (!isNotePathAvailable(newNotePath)) {
+ throw new NotePathAlreadyExistsException("Note '" + newNotePath + "' existed");
+ }
+
// move the old NoteNode from notePath to newNotePath
NoteNode noteNode = getNoteNode(notePath);
noteNode.getParent().removeNote(getNoteName(notePath));
@@ -378,6 +386,24 @@ public class NoteManager {
return notePath.substring(pos + 1);
}
+ private boolean isNotePathAvailable(String notePath) {
+ String[] tokens = notePath.split("/");
+ Folder curFolder = root;
+ for (int i = 0; i < tokens.length - 1; ++i) {
+ if (!StringUtils.isBlank(tokens[i])) {
+ curFolder = curFolder.getFolder(tokens[i]);
+ if (curFolder == null) {
+ return true;
+ }
+ }
+ }
+ if (curFolder.containsNote(tokens[tokens.length - 1])) {
+ return false;
+ }
+
+ return true;
+ }
+
/**
* Represent one folder that could contains sub folders and note files.
*/
diff --git a/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/exception/NotePathAlreadyExistsException.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/exception/NotePathAlreadyExistsException.java
new file mode 100644
index 0000000..ee89758
--- /dev/null
+++ b/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/exception/NotePathAlreadyExistsException.java
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zeppelin.notebook.exception;
+
+import java.io.IOException;
+
+public class NotePathAlreadyExistsException extends IOException {
+ private static final long serialVersionUID = -9004313429043423507L;
+
+ public NotePathAlreadyExistsException(final String message) {
+ super(message);
+ }
+
+}
diff --git a/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NoteManagerTest.java b/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NoteManagerTest.java
index 12e1e11..47a7a6b 100644
--- a/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NoteManagerTest.java
+++ b/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NoteManagerTest.java
@@ -1,9 +1,12 @@
package org.apache.zeppelin.notebook;
+import org.apache.zeppelin.notebook.exception.NotePathAlreadyExistsException;
import org.apache.zeppelin.notebook.repo.InMemoryNotebookRepo;
import org.apache.zeppelin.user.AuthenticationInfo;
import org.junit.Before;
+import org.junit.Rule;
import org.junit.Test;
+import org.junit.rules.ExpectedException;
import java.io.IOException;
import java.util.Map;
@@ -13,6 +16,9 @@ import static org.junit.Assert.assertEquals;
public class NoteManagerTest {
private NoteManager noteManager;
+ @Rule
+ public ExpectedException thrown = ExpectedException.none();
+
@Before
public void setUp() throws IOException {
this.noteManager = new NoteManager(new InMemoryNotebookRepo());
@@ -60,6 +66,32 @@ public class NoteManagerTest {
assertEquals(0, notesInfo.size());
}
+ @Test
+ public void testAddNoteRejectsDuplicatePath() throws IOException {
+ thrown.expect(NotePathAlreadyExistsException.class);
+ thrown.expectMessage("Note '/prod/note' existed");
+
+ Note note1 = createNote("/prod/note");
+ Note note2 = createNote("/prod/note");
+
+ noteManager.addNote(note1, AuthenticationInfo.ANONYMOUS);
+ noteManager.addNote(note2, AuthenticationInfo.ANONYMOUS);
+ }
+
+ @Test
+ public void testMoveNoteRejectsDuplicatePath() throws IOException {
+ thrown.expect(NotePathAlreadyExistsException.class);
+ thrown.expectMessage("Note '/prod/note-1' existed");
+
+ Note note1 = createNote("/prod/note-1");
+ Note note2 = createNote("/prod/note-2");
+
+ noteManager.addNote(note1, AuthenticationInfo.ANONYMOUS);
+ noteManager.addNote(note2, AuthenticationInfo.ANONYMOUS);
+
+ noteManager.moveNote(note2.getId(), "/prod/note-1", AuthenticationInfo.ANONYMOUS);
+ }
+
private Note createNote(String notePath) {
return new Note(notePath, "test", null, null, null, null, null);
}