You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2021/04/12 09:45:56 UTC
[GitHub] [zeppelin] dannycranmer opened a new pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
dannycranmer opened a new pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092
### 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
* [x] - Update Note UI to revert name to previous name when update fails
* [x] - Add unit tests for UI
### What is the Jira issue?
* [ZEPPELIN-4506](https://issues.apache.org/jira/browse/ZEPPELIN-4506)
### How should this be tested?
* Java unit tests
* JavaScript unit tests
* Manual unit 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] cuspymd commented on a change in pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
Posted by GitBox <gi...@apache.org>.
cuspymd commented on a change in pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#discussion_r612500675
##########
File path: zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java
##########
@@ -331,6 +333,26 @@ public void testNoteOperations() throws IOException {
assertEquals(0, notesInfo.size());
}
+ @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));
Review comment:
It would have been better to check that the newly added `broadcastNote()` call part works properly when the error occurs through the `NotebookServer::onMessage()` method test, but the test for that part seems quite complicated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] dannycranmer commented on pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
Posted by GitBox <gi...@apache.org>.
dannycranmer commented on pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#issuecomment-818563782
I have reworked this change to be server side only, no UI changes required now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] dannycranmer commented on pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
Posted by GitBox <gi...@apache.org>.
dannycranmer commented on pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#issuecomment-817891494
> I see that you have made changes in the `zeppelin-web` submodule. We a newer frontend submodule `zeppelin-web-angular`. Unfortunately, I am not a NodeJS developer, so I cannot check these parts. Why don't you have any changes in `zeppelin-web-angular`?
I ran the new UI using the README and in the browser the UI looked very similar, and my changes were there. I assumed that this component was still using the old code, however I will take another look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] dannycranmer commented on a change in pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
Posted by GitBox <gi...@apache.org>.
dannycranmer commented on a change in pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#discussion_r612252990
##########
File path: zeppelin-web/src/app/notebook/notebook.controller.js
##########
@@ -539,11 +539,23 @@ function NotebookCtrl($scope, $route, $routeParams, $location, $rootScope,
$scope.updateNoteName = function(newName) {
const trimmedNewName = newName.trim();
if (trimmedNewName.length > 0 && $scope.note.name !== trimmedNewName) {
+ $scope.note.oldName = $scope.note.name;
$scope.note.name = trimmedNewName;
websocketMsgSrv.renameNote($scope.note.id, $scope.note.name, true);
}
};
+ $scope.$on('setNoteMenu', function(event, notes) {
+ $scope.note.oldName = undefined;
+ });
+
+ $scope.$on('errorInfo', function(event, notes) {
+ if ($scope.note.oldName !== undefined) {
+ $scope.note.name = $scope.note.oldName;
+ $scope.note.oldName = undefined;
+ }
+ });
Review comment:
I went with a combined approach:
- Define an explicit exception for this error
- Push a note update message to refresh stale UI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#issuecomment-826334395
Thanks @dannycranmer will merge it soon
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#issuecomment-824535580
@dannycranmer Could you rebase and retrigger the CI ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] dannycranmer commented on pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
Posted by GitBox <gi...@apache.org>.
dannycranmer commented on pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#issuecomment-824641631
@zjffdu done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] asfgit closed pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] dannycranmer commented on a change in pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
Posted by GitBox <gi...@apache.org>.
dannycranmer commented on a change in pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#discussion_r612393692
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NoteManager.java
##########
@@ -378,6 +386,24 @@ private String getNoteName(String notePath) {
return notePath.substring(pos + 1);
}
+ private boolean validateNotePathDoesNotExist(String notePath) {
Review comment:
I have renamed inline with the feedback. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] dannycranmer commented on pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
Posted by GitBox <gi...@apache.org>.
dannycranmer commented on pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#issuecomment-817891879
Looks like I have a flaky UI test, will address
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] dannycranmer commented on pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
Posted by GitBox <gi...@apache.org>.
dannycranmer commented on pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#issuecomment-823047051
@Reamer/@cuspymd what are the next steps to get this merged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] cuspymd commented on a change in pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
Posted by GitBox <gi...@apache.org>.
cuspymd commented on a change in pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#discussion_r611663144
##########
File path: zeppelin-web/src/app/notebook/notebook.controller.js
##########
@@ -539,11 +539,23 @@ function NotebookCtrl($scope, $route, $routeParams, $location, $rootScope,
$scope.updateNoteName = function(newName) {
const trimmedNewName = newName.trim();
if (trimmedNewName.length > 0 && $scope.note.name !== trimmedNewName) {
+ $scope.note.oldName = $scope.note.name;
$scope.note.name = trimmedNewName;
websocketMsgSrv.renameNote($scope.note.id, $scope.note.name, true);
}
};
+ $scope.$on('setNoteMenu', function(event, notes) {
+ $scope.note.oldName = undefined;
+ });
+
+ $scope.$on('errorInfo', function(event, notes) {
+ if ($scope.note.oldName !== undefined) {
+ $scope.note.name = $scope.note.oldName;
+ $scope.note.oldName = undefined;
+ }
+ });
Review comment:
Couldn't other errors occur while `renameNote()` is in progress?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] asfgit closed pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] dannycranmer commented on a change in pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
Posted by GitBox <gi...@apache.org>.
dannycranmer commented on a change in pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#discussion_r611713414
##########
File path: zeppelin-web/src/app/notebook/notebook.controller.js
##########
@@ -539,11 +539,23 @@ function NotebookCtrl($scope, $route, $routeParams, $location, $rootScope,
$scope.updateNoteName = function(newName) {
const trimmedNewName = newName.trim();
if (trimmedNewName.length > 0 && $scope.note.name !== trimmedNewName) {
+ $scope.note.oldName = $scope.note.name;
$scope.note.name = trimmedNewName;
websocketMsgSrv.renameNote($scope.note.id, $scope.note.name, true);
}
};
+ $scope.$on('setNoteMenu', function(event, notes) {
+ $scope.note.oldName = undefined;
+ });
+
+ $scope.$on('errorInfo', function(event, notes) {
+ if ($scope.note.oldName !== undefined) {
+ $scope.note.name = $scope.note.oldName;
+ $scope.note.oldName = undefined;
+ }
+ });
Review comment:
Yes, I can think of 2 alternatives:
- Return a specific error response for the rename action
- Push a `NOTES_INFO` update in the error case to update the UI
What are you thoughts on the 2 approaches?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#discussion_r611652026
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NoteManager.java
##########
@@ -225,6 +228,8 @@ public void moveNote(String noteId,
throw new IOException("No metadata found for this note: " + noteId);
}
+ validateNotePathDoesNotExist(newNotePath);
Review comment:
Just a style change:
Please return a boolean value and do not throw the IOExecption out of the private method.
````
if (! isNotePathUnused(newNotePath) {
throw new IOException("Note '" + notePath + "' is already in use");
}
````
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#discussion_r612301146
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NoteManager.java
##########
@@ -378,6 +386,24 @@ private String getNoteName(String notePath) {
return notePath.substring(pos + 1);
}
+ private boolean validateNotePathDoesNotExist(String notePath) {
Review comment:
Just a small style change.
Please rename your method name to improve readability.
https://rules.sonarsource.com/java/tag/convention/RSPEC-2047
btw. +1 for the new Exception
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#issuecomment-826334395
Thanks @dannycranmer will merge it soon
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org