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 2022/07/13 07:09:10 UTC
[GitHub] [zeppelin] Reamer opened a new pull request, #4407: Roles in api
Reamer opened a new pull request, #4407:
URL: https://github.com/apache/zeppelin/pull/4407
### What is this PR for?
This pull request adds the roles of a user to API request. See `zeppelin-server/src/main/java/org/apache/zeppelin/rest/AbstractRestApi.java`-
Various Rest-API classes now inherit from AbstractRestApi.java to avoid duplicate code.
Added some request classes in preparation for a possible switch to JSON-B.
The deserialization of request to Java objects has been unified.
`zeppelin-server/src/main/java/org/apache/zeppelin/rest/message/NotebookResponse.java` was removed because no usage was detected.
### What type of PR is it?
Bug Fix, Refactoring
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-5769
### How should this be tested?
* CI
### Questions:
* Does the licenses files need to 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.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4407: [ZEPPELIN-5769] Roles in API-Request
Posted by GitBox <gi...@apache.org>.
jongyoul commented on code in PR #4407:
URL: https://github.com/apache/zeppelin/pull/4407#discussion_r921898336
##########
zeppelin-server/src/main/java/org/apache/zeppelin/rest/message/NewNoteRequest.java:
##########
@@ -16,30 +16,33 @@
*/
package org.apache.zeppelin.rest.message;
-import com.google.gson.Gson;
-
import java.util.List;
-import org.apache.zeppelin.common.JsonSerializable;
-
/**
* NewNoteRequest rest api request message.
*/
-public class NewNoteRequest implements JsonSerializable {
- private static final Gson GSON = new Gson();
+public class NewNoteRequest {
//TODO(zjffdu) rename it to be notePath instead of name
- private String name;
- private String defaultInterpreterGroup;
- private boolean addingEmptyParagraph = false;
- private List<NewParagraphRequest> paragraphs;
- private String revisionId;
+ private final String name;
+ private final String defaultInterpreterGroup;
+ private final Boolean addingEmptyParagraph;
Review Comment:
I'll also check it how to improve it.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4407: [ZEPPELIN-5769] Roles in API-Request
Posted by GitBox <gi...@apache.org>.
jongyoul commented on code in PR #4407:
URL: https://github.com/apache/zeppelin/pull/4407#discussion_r919976353
##########
zeppelin-server/src/main/java/org/apache/zeppelin/rest/message/NewNoteRequest.java:
##########
@@ -16,30 +16,33 @@
*/
package org.apache.zeppelin.rest.message;
-import com.google.gson.Gson;
-
import java.util.List;
-import org.apache.zeppelin.common.JsonSerializable;
-
/**
* NewNoteRequest rest api request message.
*/
-public class NewNoteRequest implements JsonSerializable {
- private static final Gson GSON = new Gson();
+public class NewNoteRequest {
//TODO(zjffdu) rename it to be notePath instead of name
- private String name;
- private String defaultInterpreterGroup;
- private boolean addingEmptyParagraph = false;
- private List<NewParagraphRequest> paragraphs;
- private String revisionId;
+ private final String name;
+ private final String defaultInterpreterGroup;
+ private final Boolean addingEmptyParagraph;
Review Comment:
It looks quite complicated. I understood your intention.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul merged pull request #4407: [ZEPPELIN-5769] Roles in API-Request
Posted by GitBox <gi...@apache.org>.
jongyoul merged PR #4407:
URL: https://github.com/apache/zeppelin/pull/4407
--
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.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a diff in pull request #4407: [ZEPPELIN-5769] Roles in API-Request
Posted by GitBox <gi...@apache.org>.
Reamer commented on code in PR #4407:
URL: https://github.com/apache/zeppelin/pull/4407#discussion_r921875340
##########
zeppelin-server/src/main/java/org/apache/zeppelin/rest/message/NewNoteRequest.java:
##########
@@ -16,30 +16,33 @@
*/
package org.apache.zeppelin.rest.message;
-import com.google.gson.Gson;
-
import java.util.List;
-import org.apache.zeppelin.common.JsonSerializable;
-
/**
* NewNoteRequest rest api request message.
*/
-public class NewNoteRequest implements JsonSerializable {
- private static final Gson GSON = new Gson();
+public class NewNoteRequest {
//TODO(zjffdu) rename it to be notePath instead of name
- private String name;
- private String defaultInterpreterGroup;
- private boolean addingEmptyParagraph = false;
- private List<NewParagraphRequest> paragraphs;
- private String revisionId;
+ private final String name;
+ private final String defaultInterpreterGroup;
+ private final Boolean addingEmptyParagraph;
Review Comment:
Do you have any idea how I can improve this. The `final` keyword on the `Boolean` is quite important to me, so that it is clear that you cannot edit the object from the request.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4407: [ZEPPELIN-5769] Roles in API-Request
Posted by GitBox <gi...@apache.org>.
jongyoul commented on code in PR #4407:
URL: https://github.com/apache/zeppelin/pull/4407#discussion_r919802360
##########
zeppelin-server/src/main/java/org/apache/zeppelin/rest/message/NewNoteRequest.java:
##########
@@ -16,30 +16,33 @@
*/
package org.apache.zeppelin.rest.message;
-import com.google.gson.Gson;
-
import java.util.List;
-import org.apache.zeppelin.common.JsonSerializable;
-
/**
* NewNoteRequest rest api request message.
*/
-public class NewNoteRequest implements JsonSerializable {
- private static final Gson GSON = new Gson();
+public class NewNoteRequest {
//TODO(zjffdu) rename it to be notePath instead of name
- private String name;
- private String defaultInterpreterGroup;
- private boolean addingEmptyParagraph = false;
- private List<NewParagraphRequest> paragraphs;
- private String revisionId;
+ private final String name;
+ private final String defaultInterpreterGroup;
+ private final Boolean addingEmptyParagraph;
Review Comment:
How about changing `Boolean` to `boolean`? It will make the getter function simple. WDYT?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4407: [ZEPPELIN-5769] Roles in API-Request
Posted by GitBox <gi...@apache.org>.
jongyoul commented on code in PR #4407:
URL: https://github.com/apache/zeppelin/pull/4407#discussion_r921898336
##########
zeppelin-server/src/main/java/org/apache/zeppelin/rest/message/NewNoteRequest.java:
##########
@@ -16,30 +16,33 @@
*/
package org.apache.zeppelin.rest.message;
-import com.google.gson.Gson;
-
import java.util.List;
-import org.apache.zeppelin.common.JsonSerializable;
-
/**
* NewNoteRequest rest api request message.
*/
-public class NewNoteRequest implements JsonSerializable {
- private static final Gson GSON = new Gson();
+public class NewNoteRequest {
//TODO(zjffdu) rename it to be notePath instead of name
- private String name;
- private String defaultInterpreterGroup;
- private boolean addingEmptyParagraph = false;
- private List<NewParagraphRequest> paragraphs;
- private String revisionId;
+ private final String name;
+ private final String defaultInterpreterGroup;
+ private final Boolean addingEmptyParagraph;
Review Comment:
I'll also check how to improve it.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a diff in pull request #4407: [ZEPPELIN-5769] Roles in API-Request
Posted by GitBox <gi...@apache.org>.
Reamer commented on code in PR #4407:
URL: https://github.com/apache/zeppelin/pull/4407#discussion_r919929896
##########
zeppelin-server/src/main/java/org/apache/zeppelin/rest/message/NewNoteRequest.java:
##########
@@ -16,30 +16,33 @@
*/
package org.apache.zeppelin.rest.message;
-import com.google.gson.Gson;
-
import java.util.List;
-import org.apache.zeppelin.common.JsonSerializable;
-
/**
* NewNoteRequest rest api request message.
*/
-public class NewNoteRequest implements JsonSerializable {
- private static final Gson GSON = new Gson();
+public class NewNoteRequest {
//TODO(zjffdu) rename it to be notePath instead of name
- private String name;
- private String defaultInterpreterGroup;
- private boolean addingEmptyParagraph = false;
- private List<NewParagraphRequest> paragraphs;
- private String revisionId;
+ private final String name;
+ private final String defaultInterpreterGroup;
+ private final Boolean addingEmptyParagraph;
Review Comment:
If the parameter is missing in the request and GSON deserializes a primitive data type, then the value cannot be null. Here it is a boolean, the default here is probably false.
Unfortunately I did not find any GSON documentation of the default values of the primitive datatypes. So I decided to use the class Boolean to keep the default value with the null check.
If you have a better idea, I would be very grateful. It should be clear what the default value is.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on pull request #4407: [ZEPPELIN-5769] Roles in API-Request
Posted by GitBox <gi...@apache.org>.
Reamer commented on PR #4407:
URL: https://github.com/apache/zeppelin/pull/4407#issuecomment-1192219156
I will merge this next Monday if no further comments are received.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a diff in pull request #4407: [ZEPPELIN-5769] Roles in API-Request
Posted by GitBox <gi...@apache.org>.
Reamer commented on code in PR #4407:
URL: https://github.com/apache/zeppelin/pull/4407#discussion_r923374722
##########
zeppelin-server/src/main/java/org/apache/zeppelin/rest/message/NewNoteRequest.java:
##########
@@ -16,30 +16,33 @@
*/
package org.apache.zeppelin.rest.message;
-import com.google.gson.Gson;
-
import java.util.List;
-import org.apache.zeppelin.common.JsonSerializable;
-
/**
* NewNoteRequest rest api request message.
*/
-public class NewNoteRequest implements JsonSerializable {
- private static final Gson GSON = new Gson();
+public class NewNoteRequest {
//TODO(zjffdu) rename it to be notePath instead of name
- private String name;
- private String defaultInterpreterGroup;
- private boolean addingEmptyParagraph = false;
- private List<NewParagraphRequest> paragraphs;
- private String revisionId;
+ private final String name;
+ private final String defaultInterpreterGroup;
+ private final Boolean addingEmptyParagraph;
Review Comment:
I found the part of the documentation that describes the default value for boolean and other primitive types.
https://github.com/google/gson/blob/master/UserGuide.md#finer-points-with-objects
After consulting with the developers in my company, I will look for primitive data types in other API endpoints and add null checking here as well.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org