Skip to content

Conversation

@rranjan03
Copy link
Contributor

What is this PR for?

Make search aware of notebook permissions and allow only those search results for which user has read permission.

What type of PR is it?

Bug Fix

Todos

NA

What is the Jira issue?

ZEPPELIN-705

How should this be tested?

Screenshots (if appropriate)

Questions:

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

@Leemoonsoo
Copy link
Member

@ravicodder
Thanks for the patch. Would it be difficult to make corresponding rest api test?

@prabhjyotsingh
Copy link
Contributor

LGTM, +1 for adding test.

@rranjan03
Copy link
Contributor Author

rranjan03 commented Apr 24, 2016

@Leemoonsoo @prabhjyotsingh Sorry for late response. Added test . Please have a look.

@Leemoonsoo
Copy link
Member

@ravicodder No worries. Thanks for the fix and tests. Looks good to me.

@bzz
Copy link
Member

bzz commented Apr 25, 2016

@ravicodder thank you for fixing it.

Could you also please make sure that coded formatting is consistent project's style guide?

👍 for having tests! Right now testSearch is bit long though, would you be willing to extract some methods to make it a bit more readable?

@rranjan03
Copy link
Contributor Author

@bzz Thanks for the review,
I have tried to make test more readable. Please have a look.

@Leemoonsoo
Copy link
Member

LGTM

@bzz
Copy link
Member

bzz commented May 2, 2016

Looks great, thank you for taking care.

I have one more question: in case of no auth configured, can somebody please help me to understand what is the performance implication or price per-API call to /serach/, that is added by this changeset?

We want search to be as fast as possible in a common case, so just want to make sure that we keep an eye on it. Would that be reasonable to add something like this to the query API code and at least manually compare results once (post here), to make sure preformance does not degraded, or see if any optimization are needed for case when no auth is configuted?

Please let me know what do you think!

@rranjan03
Copy link
Contributor Author

@bzz
At a time we have set the search results to show at most 20 results. I feel, for such a small value there should not be any performance issues.

screen shot 2016-05-02 at 5 35 31 pm

screen shot 2016-05-02 at 5 40 59 pm

I noted the time to search in both the cases, with permission check and without permission check.
From manual test, We can see there is no performance degradation.

@bzz
Copy link
Member

bzz commented May 3, 2016

@ravicodder looks great, thank you for double-checking!
Merging if there is no more discussion

@asfgit asfgit closed this in 2d85f3a May 3, 2016
onkarshedge pushed a commit to onkarshedge/incubator-zeppelin that referenced this pull request May 11, 2016
### What is this PR for?
Make search aware of notebook permissions and allow only those search results for which user has read permission.

### What type of PR is it?
Bug Fix

### Todos
NA

### What is the Jira issue?
[ZEPPELIN-705]( https://issues.apache.org/jira/browse/ZEPPELIN-705?jql=project%20%3D%20ZEPPELIN%20AND%20status%20in%20(Open%2C%20Resolved)%20AND%20resolution%20%3D%20Unresolved%20AND%20fixVersion%20%3D%200.6.0%20ORDER%20BY%20due%20ASC%2C%20priority%20DESC%2C%20created%20ASC)

### How should this be tested?

### 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: Ravi Ranjan <[email protected]>

Closes apache#833 from ravicodder/ZEPPELIN-705 and squashes the following commits:

a4a9999 [Ravi Ranjan] Make test more  Readable
c42573e [Ravi Ranjan] Add check to see  search searching all allowed notebook
7a624d0 [Ravi Ranjan] Add rest API test
2fe33e5 [Ravi Ranjan] search should  aware notebook permissions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants