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 2020/03/11 13:22:59 UTC
[GitHub] [zeppelin] prabhjyotsingh opened a new pull request #3684:
ZEPPELIN-4679: Add API validation for creating interpreter
prabhjyotsingh opened a new pull request #3684: ZEPPELIN-4679: Add API validation for creating interpreter
URL: https://github.com/apache/zeppelin/pull/3684
### What is this PR for?
This is an extension of ZEPPELIN-4377, I believe we should have an API validation for the same.
### What type of PR is it?
[Bug Fix]
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-4679
### How should this be tested?
Calling directly API should fail, and if somehow this is by pass from UI, UI should also return proper error.
```
curl 'http://localhost:8080/api/interpreter/setting' -H 'Content-Type: application/json;charset=UTF-8' --data-binary '{"name":"test space","group":"angular","properties":{},"dependencies":[],"option":{"remote":true,"isExistingProcess":false,"setPermission":false,"session":false,"process":false,"perNote":"shared","perUser":"shared","owners":[]},"propertyValue":"","propertyKey":"","propertyType":"textarea"}'
```
### Screenshots (if appropriate)
N/A
### Questions:
* Does the licenses files need update? N/A
* Is there breaking changes for older versions? N/A
* Does this needs documentation? N/A
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zeppelin] zjffdu commented on issue #3684: ZEPPELIN-4679: Add API
validation for creating interpreter
Posted by GitBox <gi...@apache.org>.
zjffdu commented on issue #3684: ZEPPELIN-4679: Add API validation for creating interpreter
URL: https://github.com/apache/zeppelin/pull/3684#issuecomment-597681107
@prabhjyotsingh We should also add unit test for `InterpreterSettingManager`
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zeppelin] zjffdu commented on issue #3684: ZEPPELIN-4679: Add API
validation for creating interpreter
Posted by GitBox <gi...@apache.org>.
zjffdu commented on issue #3684: ZEPPELIN-4679: Add API validation for creating interpreter
URL: https://github.com/apache/zeppelin/pull/3684#issuecomment-598593779
LGTM
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zeppelin] prabhjyotsingh commented on issue #3684: ZEPPELIN-4679:
Add API validation for creating interpreter
Posted by GitBox <gi...@apache.org>.
prabhjyotsingh commented on issue #3684: ZEPPELIN-4679: Add API validation for creating interpreter
URL: https://github.com/apache/zeppelin/pull/3684#issuecomment-597631375
@alexott, @zjffdu can you help review this?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zeppelin] prabhjyotsingh commented on issue #3684: ZEPPELIN-4679:
Add API validation for creating interpreter
Posted by GitBox <gi...@apache.org>.
prabhjyotsingh commented on issue #3684: ZEPPELIN-4679: Add API validation for creating interpreter
URL: https://github.com/apache/zeppelin/pull/3684#issuecomment-598050116
@zjffdu sure that makes sense, have added 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zeppelin] alexott commented on a change in pull request #3684:
ZEPPELIN-4679: Add API validation for creating interpreter
Posted by GitBox <gi...@apache.org>.
alexott commented on a change in pull request #3684: ZEPPELIN-4679: Add API validation for creating interpreter
URL: https://github.com/apache/zeppelin/pull/3684#discussion_r390968458
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -767,9 +769,17 @@ private InterpreterSetting inlineCreateNewSetting(String name, String group,
InterpreterOption option,
Map<String, InterpreterProperty> properties)
throws IOException {
- if (name.indexOf(".") >= 0) {
- throw new IOException("'.' is invalid for InterpreterSetting name.");
+ if (name == null) {
+ throw new IOException("Interpreter name shouldn't be empty.");
}
+
+ // check if name is valid
+ Pattern pattern = Pattern.compile("^[-_a-zA-Z0-9]+$");
Review comment:
I would move the pattern into static constant to avoid its compilation on every call....
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zeppelin] prabhjyotsingh commented on a change in pull request
#3684: ZEPPELIN-4679: Add API validation for creating interpreter
Posted by GitBox <gi...@apache.org>.
prabhjyotsingh commented on a change in pull request #3684: ZEPPELIN-4679: Add API validation for creating interpreter
URL: https://github.com/apache/zeppelin/pull/3684#discussion_r390990843
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -767,9 +769,17 @@ private InterpreterSetting inlineCreateNewSetting(String name, String group,
InterpreterOption option,
Map<String, InterpreterProperty> properties)
throws IOException {
- if (name.indexOf(".") >= 0) {
- throw new IOException("'.' is invalid for InterpreterSetting name.");
+ if (name == null) {
+ throw new IOException("Interpreter name shouldn't be empty.");
}
+
+ // check if name is valid
+ Pattern pattern = Pattern.compile("^[-_a-zA-Z0-9]+$");
+ Matcher matcher = pattern.matcher(name);
+ if(!matcher.find()){
+ throw new IOException("Interpreter name shouldn't be empty, and can consist only of: -_a-zA-Z0-9");
Review comment:
Yes, lazy me, I looked up what other APIs are using, and have used the same.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zeppelin] prabhjyotsingh commented on issue #3684: ZEPPELIN-4679:
Add API validation for creating interpreter
Posted by GitBox <gi...@apache.org>.
prabhjyotsingh commented on issue #3684: ZEPPELIN-4679: Add API validation for creating interpreter
URL: https://github.com/apache/zeppelin/pull/3684#issuecomment-598851180
Thank you for the review will merge this to master.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zeppelin] alexott commented on a change in pull request #3684:
ZEPPELIN-4679: Add API validation for creating interpreter
Posted by GitBox <gi...@apache.org>.
alexott commented on a change in pull request #3684: ZEPPELIN-4679: Add API validation for creating interpreter
URL: https://github.com/apache/zeppelin/pull/3684#discussion_r390992626
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -767,9 +769,17 @@ private InterpreterSetting inlineCreateNewSetting(String name, String group,
InterpreterOption option,
Map<String, InterpreterProperty> properties)
throws IOException {
- if (name.indexOf(".") >= 0) {
- throw new IOException("'.' is invalid for InterpreterSetting name.");
+ if (name == null) {
+ throw new IOException("Interpreter name shouldn't be empty.");
}
+
+ // check if name is valid
+ Pattern pattern = Pattern.compile("^[-_a-zA-Z0-9]+$");
+ Matcher matcher = pattern.matcher(name);
+ if(!matcher.find()){
+ throw new IOException("Interpreter name shouldn't be empty, and can consist only of: -_a-zA-Z0-9");
Review comment:
I have a TODO list to go through all places & fix this :-)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zeppelin] alexott commented on a change in pull request #3684:
ZEPPELIN-4679: Add API validation for creating interpreter
Posted by GitBox <gi...@apache.org>.
alexott commented on a change in pull request #3684: ZEPPELIN-4679: Add API validation for creating interpreter
URL: https://github.com/apache/zeppelin/pull/3684#discussion_r390972238
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -767,9 +769,17 @@ private InterpreterSetting inlineCreateNewSetting(String name, String group,
InterpreterOption option,
Map<String, InterpreterProperty> properties)
throws IOException {
- if (name.indexOf(".") >= 0) {
- throw new IOException("'.' is invalid for InterpreterSetting name.");
+ if (name == null) {
+ throw new IOException("Interpreter name shouldn't be empty.");
}
+
+ // check if name is valid
+ Pattern pattern = Pattern.compile("^[-_a-zA-Z0-9]+$");
+ Matcher matcher = pattern.matcher(name);
+ if(!matcher.find()){
+ throw new IOException("Interpreter name shouldn't be empty, and can consist only of: -_a-zA-Z0-9");
Review comment:
I'm not 100% sure that we should use `IOException` here, it's more like "Invalid parameters, or something like". But maybe it will require big changes along the call chain to use something else, so I'm ok with 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zeppelin] prabhjyotsingh commented on issue #3684: ZEPPELIN-4679:
Add API validation for creating interpreter
Posted by GitBox <gi...@apache.org>.
prabhjyotsingh commented on issue #3684: ZEPPELIN-4679: Add API validation for creating interpreter
URL: https://github.com/apache/zeppelin/pull/3684#issuecomment-598588136
@zjffdu @alexott does these changes looks ok?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zeppelin] zjffdu commented on a change in pull request #3684:
ZEPPELIN-4679: Add API validation for creating interpreter
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #3684: ZEPPELIN-4679: Add API validation for creating interpreter
URL: https://github.com/apache/zeppelin/pull/3684#discussion_r391028501
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -767,9 +769,17 @@ private InterpreterSetting inlineCreateNewSetting(String name, String group,
InterpreterOption option,
Map<String, InterpreterProperty> properties)
throws IOException {
- if (name.indexOf(".") >= 0) {
- throw new IOException("'.' is invalid for InterpreterSetting name.");
+ if (name == null) {
+ throw new IOException("Interpreter name shouldn't be empty.");
}
+
+ // check if name is valid
+ Pattern pattern = Pattern.compile("^[-_a-zA-Z0-9]+$");
+ Matcher matcher = pattern.matcher(name);
+ if(!matcher.find()){
+ throw new IOException("Interpreter name shouldn't be empty, and can consist only of: -_a-zA-Z0-9");
Review comment:
Regarding this, we do need to revisit that, and could do it in other PR
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zeppelin] prabhjyotsingh commented on a change in pull request
#3684: ZEPPELIN-4679: Add API validation for creating interpreter
Posted by GitBox <gi...@apache.org>.
prabhjyotsingh commented on a change in pull request #3684: ZEPPELIN-4679: Add API validation for creating interpreter
URL: https://github.com/apache/zeppelin/pull/3684#discussion_r391448028
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -767,9 +769,17 @@ private InterpreterSetting inlineCreateNewSetting(String name, String group,
InterpreterOption option,
Map<String, InterpreterProperty> properties)
throws IOException {
- if (name.indexOf(".") >= 0) {
- throw new IOException("'.' is invalid for InterpreterSetting name.");
+ if (name == null) {
+ throw new IOException("Interpreter name shouldn't be empty.");
}
+
+ // check if name is valid
+ Pattern pattern = Pattern.compile("^[-_a-zA-Z0-9]+$");
Review comment:
Sure let me do that.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zeppelin] asfgit closed pull request #3684: ZEPPELIN-4679: Add API
validation for creating interpreter
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3684: ZEPPELIN-4679: Add API validation for creating interpreter
URL: https://github.com/apache/zeppelin/pull/3684
----------------------------------------------------------------
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
With regards,
Apache Git Services