ZEPPLIN-976 ] HotFix - zeppelin server does not restart when incorrect credentials data. #976

cloverhearts · 2016-06-08T09:36:47Z

What is this PR

zeppelin server does not restart when incorrect credentials data.

reproduce.

Click to zeppelin home for web ui.
Click to zeppelin Credentials.
'Entity' information without writing, username and password only written to storage.
Click to zeppelin home for web ui.
Click to zeppelin Credentials.
and zeppelin restart.

but, Zeppelin does not work.

It creates the wrong json file. 'conf / credentials.json' according to the null.

What type of PR is it?

Hot Fix

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-976

How should this be tested?

Click to zeppelin home for web ui.
Click to zeppelin Credentials.
'Entity' information without writing, username and password only written to storage.
Click to zeppelin home for web ui.
Click to zeppelin Credentials.
and zeppelin restart.

zeppelin does work!

Reproduced Screenshots

Questions:

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

cloverhearts · 2016-06-08T10:32:35Z

retry ci

echarles · 2016-06-08T11:11:30Z

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

    String password = messageMap.get("password");
+
+    if (entity == null || username == null || password == null) {
+      return new JsonResponse(Status.INTERNAL_SERVER_ERROR, "", "").build();


Status.BAD_REQUEST would be more adequate here.

@echarles
Thank you for your feedback.
You're right. i've changed.

bzz · 2016-06-09T10:14:51Z

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

    String username = messageMap.get("username");
    String password = messageMap.get("password");
+
+    if (entity == null || username == null || password == null) {


And they could not be "", or is that case is ok?

An empty string does not have a problem in Zeppelin server.
I only solve a serious trouble in this pr.

bzz · 2016-06-09T10:16:31Z

@cloverhearts thank you for hotfixing!

How do you think, is there a chance we could have a test that reproduces this issue under this PR or do you think it would be to hard add?

cloverhearts · 2016-06-10T03:27:42Z

@bzz
I think this is a very serious problem.
In my personal opinion, first merge this PR, and improve Credentials feature in future ( new pr ).

Problem situation, please refer to the screenshot.

Reproduce the problem, please note the following:

Click to zeppelin home for web ui.
Click to zeppelin Credentials.
'Entity' information without writing, username and password only written to storage.
Click to zeppelin home for web ui.
Click to zeppelin Credentials.
and zeppelin restart.

Leemoonsoo · 2016-06-13T07:37:49Z

The fix looks good to me. But like @bzz mentioned, it will be even better if there're unittest for this problem.

cloverhearts · 2016-06-13T08:05:27Z

@bzz @Leemoonsoo
I am sorry.
do you mean the test code for this feature?

bzz · 2016-06-13T10:03:13Z

@cloverhearts yes, if that is not very hard to do

cloverhearts · 2016-06-13T10:10:25Z

@bzz Okay, i would be make it.
Thank you so much.

cloverhearts · 2016-06-14T07:32:46Z

add test code about invalid request for credentials api

cloverhearts · 2016-06-14T10:07:24Z

retry ci

minahlee · 2016-06-14T18:12:14Z

@cloverhearts Seems like integration test fails, can you fix it please?

Tests run: 8, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 120.219 sec <<< FAILURE! - in org.apache.zeppelin.integration.ParagraphActionsIT
testTitleButton(org.apache.zeppelin.integration.ParagraphActionsIT)  Time elapsed: 16.504 sec  <<< FAILURE!
java.lang.AssertionError: After Hide Title : The title field contains
Expected: ""
     but: was "Untitled"
    at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
    at org.junit.Assert.assertThat(Assert.java:865)
    at org.junit.rules.ErrorCollector$1.call(ErrorCollector.java:65)
    at org.junit.rules.ErrorCollector.checkSucceeds(ErrorCollector.java:78)
    at org.junit.rules.ErrorCollector.checkThat(ErrorCollector.java:63)
    at org.apache.zeppelin.integration.ParagraphActionsIT.testTitleButton(ParagraphActionsIT.java:355)

cloverhearts · 2016-06-15T02:31:02Z

@minahlee it's not my issue.
I think the problem seems to arise from a sudden UI test master branch.
Let's solve the problem from a different PR.
One will stop working on this feature.

astroshim · 2016-06-15T04:12:08Z

LGTM.
@minahlee The selenium test of travis has a problem so @corneadoug is fixing now.

cloverhearts · 2016-06-15T06:44:56Z

ci pass.
please, check to this pr.

bzz · 2016-06-15T08:04:07Z

@cloverhearts thank you for adding tests!
Looks great to me, merging if there is no more discussion

…t credentials data. ### What is this PR zeppelin server does not restart when incorrect credentials data. reproduce. 1. Click to zeppelin home for web ui. 2. Click to zeppelin Credentials. 3. 'Entity' information without writing, username and password only written to storage. 4. Click to zeppelin home for web ui. 5. Click to zeppelin Credentials. and zeppelin restart. ##### but, Zeppelin does not work. It creates the wrong json file. 'conf / credentials.json' according to the null. ### What type of PR is it? Hot Fix ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-976 ### How should this be tested? 1. Click to zeppelin home for web ui. 2. Click to zeppelin Credentials. 3. 'Entity' information without writing, username and password only written to storage. 4. Click to zeppelin home for web ui. 5. Click to zeppelin Credentials. and zeppelin restart. zeppelin does work! ### Reproduced Screenshots ![zeppelin-server-error2](https://cloud.githubusercontent.com/assets/10525473/15889828/b92590d8-2da7-11e6-9b51-0a82c3bb9f1f.gif) ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: CloverHearts <cloverheartsdev@gmail.com> Closes #976 from cloverhearts/hotfix/CredentialsJsonBug and squashes the following commits: 293ab08 [CloverHearts] Merge branch 'master' into hotfix/CredentialsJsonBug ef256c2 [CloverHearts] Merge branch 'master' into hotfix/CredentialsJsonBug 5079495 [CloverHearts] add test code for credentials backends about invalid request. e9a1e93 [CloverHearts] Merge branch 'master' into hotfix/CredentialsJsonBug 4b9aba3 [CloverHearts] changed status code for CredentialsRestapi 1e5cd72 [CloverHearts] Credentials Json serialize backend bug.

…t credentials data. ### What is this PR zeppelin server does not restart when incorrect credentials data. reproduce. 1. Click to zeppelin home for web ui. 2. Click to zeppelin Credentials. 3. 'Entity' information without writing, username and password only written to storage. 4. Click to zeppelin home for web ui. 5. Click to zeppelin Credentials. and zeppelin restart. ##### but, Zeppelin does not work. It creates the wrong json file. 'conf / credentials.json' according to the null. ### What type of PR is it? Hot Fix ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-976 ### How should this be tested? 1. Click to zeppelin home for web ui. 2. Click to zeppelin Credentials. 3. 'Entity' information without writing, username and password only written to storage. 4. Click to zeppelin home for web ui. 5. Click to zeppelin Credentials. and zeppelin restart. zeppelin does work! ### Reproduced Screenshots ![zeppelin-server-error2](https://cloud.githubusercontent.com/assets/10525473/15889828/b92590d8-2da7-11e6-9b51-0a82c3bb9f1f.gif) ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: CloverHearts <cloverheartsdev@gmail.com> Closes apache#976 from cloverhearts/hotfix/CredentialsJsonBug and squashes the following commits: 293ab08 [CloverHearts] Merge branch 'master' into hotfix/CredentialsJsonBug ef256c2 [CloverHearts] Merge branch 'master' into hotfix/CredentialsJsonBug 5079495 [CloverHearts] add test code for credentials backends about invalid request. e9a1e93 [CloverHearts] Merge branch 'master' into hotfix/CredentialsJsonBug 4b9aba3 [CloverHearts] changed status code for CredentialsRestapi 1e5cd72 [CloverHearts] Credentials Json serialize backend bug.

Credentials Json serialize backend bug.

1e5cd72

cloverhearts closed this Jun 8, 2016

cloverhearts reopened this Jun 8, 2016

echarles reviewed Jun 8, 2016
View reviewed changes

changed status code for CredentialsRestapi

4b9aba3

cloverhearts changed the title ~~ZEPPLIN-976 ] HotFix - Incorrect "Credentials" server does not restart.~~ ZEPPLIN-976 ] HotFix - zeppelin server does not restart when incorrect credentials data. Jun 9, 2016

bzz reviewed Jun 9, 2016
View reviewed changes

cloverhearts added 2 commits June 14, 2016 14:08

Merge branch 'master' into hotfix/CredentialsJsonBug

e9a1e93

add test code for credentials backends about invalid request.

5079495

cloverhearts closed this Jun 14, 2016

cloverhearts reopened this Jun 14, 2016

cloverhearts closed this Jun 14, 2016

cloverhearts reopened this Jun 14, 2016

cloverhearts closed this Jun 14, 2016

cloverhearts reopened this Jun 14, 2016

Merge branch 'master' into hotfix/CredentialsJsonBug

ef256c2

Merge branch 'master' into hotfix/CredentialsJsonBug

293ab08

asfgit closed this in 1bc3e9c Jun 15, 2016

cloverhearts deleted the hotfix/CredentialsJsonBug branch June 16, 2016 06:21

ZEPPLIN-976 ] HotFix - zeppelin server does not restart when incorrect credentials data. #976

ZEPPLIN-976 ] HotFix - zeppelin server does not restart when incorrect credentials data. #976

Uh oh!

Conversation

cloverhearts commented Jun 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR

but, Zeppelin does not work.

What type of PR is it?

What is the Jira issue?

How should this be tested?

Reproduced Screenshots

Questions:

Uh oh!

cloverhearts commented Jun 8, 2016

Uh oh!

echarles Jun 8, 2016

Choose a reason for hiding this comment

Uh oh!

cloverhearts Jun 8, 2016

Choose a reason for hiding this comment

Uh oh!

bzz Jun 9, 2016

Choose a reason for hiding this comment

Uh oh!

cloverhearts Jun 10, 2016

Choose a reason for hiding this comment

Uh oh!

bzz commented Jun 9, 2016

Uh oh!

cloverhearts commented Jun 10, 2016

Uh oh!

Leemoonsoo commented Jun 13, 2016

Uh oh!

cloverhearts commented Jun 13, 2016

Uh oh!

bzz commented Jun 13, 2016

Uh oh!

cloverhearts commented Jun 13, 2016

Uh oh!

cloverhearts commented Jun 14, 2016

Uh oh!

cloverhearts commented Jun 14, 2016

Uh oh!

minahlee commented Jun 14, 2016

Uh oh!

cloverhearts commented Jun 15, 2016

Uh oh!

astroshim commented Jun 15, 2016

Uh oh!

cloverhearts commented Jun 15, 2016

Uh oh!

bzz commented Jun 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cloverhearts commented Jun 8, 2016 •

edited

Loading