[ZEPPELIN-1564] Enable note deletion and paragraph output clear from main page #1565

minahlee · 2016-10-27T09:30:59Z

What is this PR for?

Enables removing note and clear all paragraph's output from Zeppelin main page.
Add rest api for clearing all paragraph output

Next possible improvement can be removing notes in folder level and rename folder.

What type of PR is it?

Improvement

Todos

- Merge [ZEPPELIN-1586] Add security check in NotebookRestApi #1567 and apply security to clearAllParagraphOutput rest api method

What is the Jira issue?

ZEPPELIN-1564

Screenshots (if appropriate)

Questions:

Does the licenses files need update? no
Is there breaking changes for older versions? no
Does this needs documentation? yes

khalidhuseynov

@minahlee thanks for the improvement. as i can see from screenshot, clearing all paragraph output done on the list view. what do you think is more user friendly: doing it on list or inside of a note. any reasons for choice maybe.

khalidhuseynov · 2016-10-28T03:57:01Z

zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java

+      Note note = notebook.getNote(noteId);
+      if (note == null) {
+        return new JsonResponse(Status.NOT_FOUND, "note not found.").build();
+      }


I wonder how we enforce note authorization values here, for example someone without owner/write privileges does a call via rest api.

yeah, i think @khalidhuseynov is right here.
But after checking the code of notebookRestApi, there are no check here. we need to fix this...

I checked notebookRestApi class and there are a lot of method which doesn't take care of privileges like getNote, getParagraph. I think it is better to create another PR to refactor notebookRestApi.

@minahlee thats right, i created the issue ZEPPELIN-1586 i will create the PR soon

@anthonycorbacho Thank you! @khalidhuseynov Let me update this PR to handle permission issue once ZEPPELIN-1586 is ready

minahlee · 2016-10-28T05:57:29Z

@khalidhuseynov Thank you for review. I made "clear paragraph output" in main page for the case that note's output data is too big so it causes Zeppelin fail to load note and hang.
Here are tickets reporting this issue: ZEPPELIN-1564, ZEPPELIN-1491

The more optimal solution would be improving data loading and rendering itself. We might not need "clear paragraph output" button once we improve this performance issue.

I believe having delete button on the main page is beneficial in terms of UX in case user wants to delete multiple notes. They won't need to navigate into each note to delete them. I am planning to implement remove folder & edit folder name in main page after this PR is merged.

tae-jun · 2016-10-31T14:21:07Z

Hi @minahlee :) This is a cool feature! I have an experience that deleted output through editing note.json because of huge output stopping Zeppelin web.

By the way, I've wanted a renaming folder feature. So as soon as I saw this PR, I implemented renaming note, and I am almost done with implementing renaming folder! (I saw you are planning this after coding, sorry 😢) This is a really good example for implementing it!

So, the question is this:
My branch is based on your PR's branch. How should I create JIRA issue? As well as PR.
Is it easy to wait to merge this PR?

minahlee · 2016-10-31T14:55:54Z

Hi @tae-jun, thank you for your interest. I had to rebase this branch to resolve conflict so you will need too since it is based on mine.

I am planning to finish this PR once #1567 merged so that I can handle permission issue. It will speed up merging process if you can review it (I am also going to review it as soon as all the tasks are done).

About the jira, there is nothing stops you from creating one so feel free to create a ticket on Jira. And make sure that you assign your self on that task, and put PROGRESS label so everyone can see that you are working on it. And for the PR, I suggest you to do the cherry pick after #1567, #1565 merged, otherwise it will cause some confusion for the ones who don't know the context.

tae-jun · 2016-11-01T13:16:07Z

Thanks @minahlee! I appreciate your kind comment 😄

I created a ticket on JIRA: ZEPPELIN-1598

But, I don't know how to set myself to assignee, and change status to PROGRESS. I guess I don't have a permission to do that 😢

I will check #1567. Thanks again!

minahlee · 2016-11-01T13:26:16Z

@tae-jun I just added your name to contributor list so you will be able to set yourself as assignee from now.

tae-jun · 2016-11-01T13:28:49Z

@minahlee Awesome! I just changed status to IN PROGRESS now. Thanks 👍

Leemoonsoo · 2016-11-02T22:15:36Z

Tested and LGTM

…ELIN-1564

minahlee · 2016-11-03T08:41:30Z

@khalidhuseynov I updated rest api to do privilege check. Could you help review?

khalidhuseynov · 2016-11-03T12:00:05Z

@minahlee looks much better now! thanks for addressing the issue and LGTM

minahlee · 2016-11-03T12:06:48Z

@khalidhuseynov Thank you for quick response!
Merging if there is no more discussion.

anthonycorbacho · 2016-11-03T12:07:34Z

docs/rest-api/rest-notebook.md

+    </tr>
+    <tr>
+      <td> Fail code</td>
+      <td> 500 </td>


shall we also add 404 (not found) and 401(unauthorised)? as a possible response

tae-jun · 2016-11-03T12:27:03Z

@minahlee I have a question about REST API style. Is there any purpose that you used PUT method?

In my opinion, DELETE method is more proper since it does not PUT clear under a noteId. This action DELETEs noteId's outputs. So, this is API style I suggest:

DELETE {noteId}/outputs

And also maybe we can implement one paragraph output deletion in the future like:

DELETE {noteId}/outputs/{paragraphId}

minahlee · 2016-11-03T12:39:44Z

@tae-jun the reason I use PUT is that clearAllParagraphOutput method is not really deleting result field from paragraph, but updating result field to empty.
I was considering using DELETE but I didn't because it can confuse user that it will literally 'DELETE' result field.

What do you think?

Update clear output rest api doc response code

tae-jun · 2016-11-04T04:39:05Z

@minahlee Oh, in that context, it makes sense. LGTM.

But, after looking all around Notebook REST API, it works well now but it doesn't seem RESTful. I summarized current list of REST API:

GET /
POST /
GET /[noteId]
POST /[noteId]
DELETE /[noteId]
GET /job/[noteId]
POST /job/[noteId]
DELETE /job/[noteId]
GET /job/[noteId]/[paragraphId]
POST /job/[noteId]/[paragraphId]
DELETE /job/[noteId]/[paragraphId]
POST /run/[noteId]/[paragraphId]
GET /cron/[noteId]
POST /cron/[noteId]
DELETE /cron/[noteId]
GET /search?q=[query]
POST /[noteId]/paragraph
GET /[noteId]/paragraph/[paragraphId]
DELETE /[noteId]/paragraph/[paragraphId]
POST /[noteId]/paragraph/[paragraphId]/move/[newIndex]
GET /export/[noteId]
POST /import

A good REST API is API that doesn't need a documentation, which means users can understands only with HTTP method and URL. So, it should be simple. However, I think some of current API may need to be changed.

REST API is UI for developers, so should be well designed. There are problems I can see now:

Need to organize resources of Zeppelin: hierarchy of resources, and what they have; For example:
- Need hierarchical URL design: notebook/:noteId/paragraphs/:paragraphId/{title|text|result|job}
- Do jobs belong to notebook or paragraph?
Use plural
Use nouns as far as you can, no verbs. Make use of HTTP methods. URL should point resources.
For in the case of a request that doesn't fit CRUD(Create(POST), Read(GET), Update(PUT|PATCH), Detele(DELETE)), we need a rule for this. Heroku deals with this by placing actions for the resource under /actions. For example, in our case, run all paragraphs can be represented as POST notebook/:noteId/actions/run
Need API versioning

Zeppelin needs more REST API, so eventually, it will be getting bigger. I think it's time to organize REST APIs. After all of this, the REST API documentation should also be updated. Not by listing actions what you can, just by listing APIs. This is easier to read since users can know what they can do with resources.

Would you mind if I open an issue for this?

anthonycorbacho · 2016-11-04T04:47:40Z

@tae-jun This is a great sum of the status of the rest api in zeppelin, and i think we all agree that currently zeppelin doenst follow rest standard (just by looking at the endpoint + response (mostly 500 ...)).

But you have to consider that going on the path of refacto this logic will be a huge task, not only in backend but you need to understand that a lot of users are using this api already changing the hole api will brings a lot of breaking change in they current workflow. So we need to be careful here.

khalidhuseynov · 2016-11-04T05:52:58Z

@tae-jun thanks for summarizing and giving feedback on REST API. Although we can't address all at once as @anthonycorbacho mentioned, but I think it deserves an issue with above content and feedback logged in there

tae-jun · 2016-11-04T06:20:46Z

@anthonycorbacho @khalidhuseynov Thanks for the comment! I agree with you guys.

This is a breaking change, so yeah we must be careful. So, if we proceed this, new API URL's prefix will be v1/api/notebook. And keep legacy API under api/notebook for backward compatibility. This will be the start of versioning.

I think we should design REST API together. I will create a proposal on JIRA soon, please review it when the time comes!

It will be harder to fix later. So, let's start now! 😄

…ELIN-1564

khalidhuseynov reviewed Oct 28, 2016

View reviewed changes

minahlee added 6 commits October 31, 2016 23:38

Clear all paragraphs and remove note from main page

4adddb4

Add clearAllParagraphOutput unit test

98d7604

Add rest api for clear all paragraph result and add test

3d19141

Add rest api endpoint for clear paragraph result to document

d66600c

Remove unused import

dea3ef6

Give writer permission to clear output

7eb3521

minahlee force-pushed the ZEPPELIN-1564 branch from 70eb294 to 7eb3521 Compare October 31, 2016 14:38

Change post to put

f349dbf

minahlee added 2 commits November 3, 2016 15:20

Merge branch 'master' of https://github.com/apache/zeppelin into ZEPP…

fb7e6ae

…ELIN-1564

Add auth check before clearing all paragraph

2ee452e

anthonycorbacho reviewed Nov 3, 2016

View reviewed changes

Rename class name from UnauthorizedException to ForbiddenException

1393ee9

Update clear output rest api doc response code

minahlee force-pushed the ZEPPELIN-1564 branch from 56a6c44 to 1393ee9 Compare November 3, 2016 14:50

Merge branch 'master' of https://github.com/apache/zeppelin into ZEPP…

749aebe

…ELIN-1564

asfgit closed this in dd20e7b Nov 5, 2016

minahlee deleted the ZEPPELIN-1564 branch March 6, 2017 04:49

[ZEPPELIN-1564] Enable note deletion and paragraph output clear from main page #1565

[ZEPPELIN-1564] Enable note deletion and paragraph output clear from main page #1565

Uh oh!

Conversation

minahlee commented Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

Screenshots (if appropriate)

Questions:

Uh oh!

khalidhuseynov left a comment

Choose a reason for hiding this comment

Uh oh!

khalidhuseynov Oct 28, 2016

Choose a reason for hiding this comment

Uh oh!

anthonycorbacho Oct 28, 2016

Choose a reason for hiding this comment

Uh oh!

minahlee Oct 28, 2016

Choose a reason for hiding this comment

Uh oh!

anthonycorbacho Oct 28, 2016

Choose a reason for hiding this comment

Uh oh!

minahlee Oct 28, 2016

Choose a reason for hiding this comment

Uh oh!

minahlee commented Oct 28, 2016

Uh oh!

tae-jun commented Oct 31, 2016

Uh oh!

minahlee commented Oct 31, 2016

Uh oh!

tae-jun commented Nov 1, 2016

Uh oh!

minahlee commented Nov 1, 2016

Uh oh!

tae-jun commented Nov 1, 2016

Uh oh!

Leemoonsoo commented Nov 2, 2016

Uh oh!

minahlee commented Nov 3, 2016

Uh oh!

khalidhuseynov commented Nov 3, 2016

Uh oh!

minahlee commented Nov 3, 2016

Uh oh!

anthonycorbacho Nov 3, 2016

Choose a reason for hiding this comment

Uh oh!

tae-jun commented Nov 3, 2016

Uh oh!

minahlee commented Nov 3, 2016

Uh oh!

tae-jun commented Nov 4, 2016

Uh oh!

anthonycorbacho commented Nov 4, 2016

Uh oh!

khalidhuseynov commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tae-jun commented Nov 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

minahlee commented Oct 27, 2016 •

edited

Loading

khalidhuseynov commented Nov 4, 2016 •

edited

Loading