[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
GitHub user jainbhupendra24 opened a pull request: https://github.com/apache/zookeeper/pull/279 ZOOKEEPER-2804:Node creation fails with NPE if ACLs are null 1) Handled Null case in server. Client will get InvalidACLException 2) Handled null check in create and setACL APIs in client side. as per their javadoc @throws KeeperException.InvalidACLException if the ACL is invalid, null, or empty 3) Not handling any validation for async API of create and setACL in this JIRA because these API doesn't throw KeeperException explicitly. So can not throw InvalidACLException from Client. If we throw IllegalArgumentException then it will not be consistent with other sync APIs. So Let server throw InvalidACLException for async API. Please review and provide suggestion. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jainbhupendra24/zookeeper ZOOKEEPER-2804-new Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/279.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #279 commit 9deacdabc061b95b24242a6bc557b7ee031bdf9e Author: bhupendra jain Date: 2017-06-10T20:16:44Z ZOOKEEPER-2804:Node creation fails with NPE if ACLs are null --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/279#discussion_r121280641
--- Diff:
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java ---
@@ -915,11 +915,13 @@ protected void pRequest(Request request) throws
RequestProcessorException {
private List removeDuplicates(List acl) {
ArrayList retval = new ArrayList();
-Iterator it = acl.iterator();
-while (it.hasNext()) {
-ACL a = it.next();
-if (retval.contains(a) == false) {
-retval.add(a);
+if(acl != null) {
--- End diff --
nit: space between `if` and `(`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/279#discussion_r121281455
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -1436,7 +1436,7 @@ public String create(final String path, byte data[],
List acl,
request.setData(data);
request.setFlags(createMode.toFlag());
request.setPath(serverPath);
-if (acl != null && acl.size() == 0) {
+if (acl == null || acl.size() == 0) {
--- End diff --
Wouldn't be better to account for `null` entries in the `acl` List like
below?
```
if (acl == null || acl.size() == 0 || acl.contains(null)) {
```
The same applies to other changes in this file.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/279#discussion_r121280799
--- Diff:
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java ---
@@ -915,11 +915,13 @@ protected void pRequest(Request request) throws
RequestProcessorException {
private List removeDuplicates(List acl) {
ArrayList retval = new ArrayList();
--- End diff --
If we make retval a `Set` then code on lines 918-924 can be simplified as
shown below.
```
Set retval = new ACL<>();
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/279#discussion_r121280993
--- Diff:
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java ---
@@ -915,11 +915,13 @@ protected void pRequest(Request request) throws
RequestProcessorException {
private List removeDuplicates(List acl) {
ArrayList retval = new ArrayList();
-Iterator it = acl.iterator();
-while (it.hasNext()) {
-ACL a = it.next();
-if (retval.contains(a) == false) {
-retval.add(a);
+if(acl != null) {
--- End diff --
```
if (acl != null) {
retval.addAll(acl);
}
}
return new List<>(retval);
```
OR
```
if (acl != null) {
for (ACL a: acl) {
if (a != null) {
retval.add(a);
}
}
}
}
return new List<>(retval);
```
Main difference between the two code snippets is that the latter accounts
for null entries in the `acl` List. As you know, Lists accept null entries, but
`Set` throws `NPE`. The original code doesn't prevent that, but this can never
be the case (sending a null ACL entry in the List), so the former snippet is
preferrable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
Github user jainbhupendra24 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/279#discussion_r121657958
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -1436,7 +1436,7 @@ public String create(final String path, byte data[],
List acl,
request.setData(data);
request.setFlags(createMode.toFlag());
request.setPath(serverPath);
-if (acl != null && acl.size() == 0) {
+if (acl == null || acl.size() == 0) {
--- End diff --
This client side nulll check is better to add. I will add
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
Github user jainbhupendra24 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/279#discussion_r121658390
--- Diff:
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java ---
@@ -915,11 +915,13 @@ protected void pRequest(Request request) throws
RequestProcessorException {
private List removeDuplicates(List acl) {
ArrayList retval = new ArrayList();
-Iterator it = acl.iterator();
-while (it.hasNext()) {
-ACL a = it.next();
-if (retval.contains(a) == false) {
-retval.add(a);
+if(acl != null) {
--- End diff --
Server side, the validation for null is already present in fixupACL method
and it throws InvalidACLException. This check is after the removeDuplicates
method call.
So do you mean we can handle null in removeDuplicates method itself and if
so,, we can remove the null validation code in fixupACL method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
Github user jainbhupendra24 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/279#discussion_r121658648
--- Diff:
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java ---
@@ -915,11 +915,13 @@ protected void pRequest(Request request) throws
RequestProcessorException {
private List removeDuplicates(List acl) {
ArrayList retval = new ArrayList();
--- End diff --
Thanks for review . I will add the test case
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/279#discussion_r121749385
--- Diff:
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java ---
@@ -915,11 +915,13 @@ protected void pRequest(Request request) throws
RequestProcessorException {
private List removeDuplicates(List acl) {
ArrayList retval = new ArrayList();
-Iterator it = acl.iterator();
-while (it.hasNext()) {
-ACL a = it.next();
-if (retval.contains(a) == false) {
-retval.add(a);
+if(acl != null) {
--- End diff --
Good point. I added the check for nulls on `removeDuplicates` because `Set`
doesn't accept null entries. So, if there is a null in `acl` list then the
first snippet I suggested would throw an exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/279#discussion_r121751200
--- Diff:
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java ---
@@ -915,11 +915,13 @@ protected void pRequest(Request request) throws
RequestProcessorException {
private List removeDuplicates(List acl) {
ArrayList retval = new ArrayList();
-Iterator it = acl.iterator();
-while (it.hasNext()) {
-ACL a = it.next();
-if (retval.contains(a) == false) {
-retval.add(a);
+if(acl != null) {
--- End diff --
If we are going to keep the code we can at least modernize it a bit as
below:
```
for (ACL a : acl) {
if (!retval.contains(a)) {
retval.add(a);
}
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/279#discussion_r126290078
--- Diff: src/java/test/org/apache/zookeeper/test/ACLTest.java ---
@@ -189,4 +191,110 @@ public void process(WatchedEvent event) {
}
}
}
+
+@Test
+public void testNullACL() throws Exception {
+File tmpDir = ClientBase.createTmpDir();
+ClientBase.setupTestEnv();
+ZooKeeperServer zks = new ZooKeeperServer(tmpDir, tmpDir, 3000);
+final int PORT = Integer.parseInt(HOSTPORT.split(":")[1]);
+ServerCnxnFactory f = ServerCnxnFactory.createFactory(PORT, -1);
+f.startup(zks);
+ZooKeeper zk = ClientBase.createZKClient(HOSTPORT);
+try {
+// case 1 : null ACL with create
+try {
+zk.create("/foo", "foo".getBytes(), null,
CreateMode.PERSISTENT);
--- End diff --
All tests can be consolidated into a single try block to avoid duplicate
the boilerplate code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
Github user jainbhupendra24 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/279#discussion_r126334642
--- Diff: src/java/test/org/apache/zookeeper/test/ACLTest.java ---
@@ -189,4 +191,110 @@ public void process(WatchedEvent event) {
}
}
}
+
+@Test
+public void testNullACL() throws Exception {
+File tmpDir = ClientBase.createTmpDir();
+ClientBase.setupTestEnv();
+ZooKeeperServer zks = new ZooKeeperServer(tmpDir, tmpDir, 3000);
+final int PORT = Integer.parseInt(HOSTPORT.split(":")[1]);
+ServerCnxnFactory f = ServerCnxnFactory.createFactory(PORT, -1);
+f.startup(zks);
+ZooKeeper zk = ClientBase.createZKClient(HOSTPORT);
+try {
+// case 1 : null ACL with create
+try {
+zk.create("/foo", "foo".getBytes(), null,
CreateMode.PERSISTENT);
--- End diff --
OK , will make these changes
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
Github user arshadmohammad commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/279#discussion_r126481060
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -1436,7 +1436,7 @@ public String create(final String path, byte data[],
List acl,
request.setData(data);
request.setFlags(createMode.toFlag());
request.setPath(serverPath);
-if (acl != null && acl.size() == 0) {
+if (acl == null || acl.isEmpty() || acl.contains(null)) {
--- End diff --
These validations should also be applied on async create APIs
1. org.apache.zookeeper.ZooKeeper.create(String, byte[], List,
CreateMode, Create2Callback, Object, long)
2. org.apache.zookeeper.ZooKeeper.create(String, byte[], List,
CreateMode, StringCallback, Object)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
Github user arshadmohammad commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/279#discussion_r126482088
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -1535,7 +1535,7 @@ public String create(final String path, byte data[],
List acl,
RequestHeader h = new RequestHeader();
setCreateHeader(createMode, h);
Create2Response response = new Create2Response();
-if (acl != null && acl.size() == 0) {
+if (acl == null || acl.isEmpty() || acl.contains(null)) {
--- End diff --
As this Validation we are doing multiple places it would be better if this
piece of code is extracted to method. Some thing like
` private void validateACL(List acl) throws InvalidACLException {
if (acl == null || acl.isEmpty() || acl.contains(null)) {
throw new KeeperException.InvalidACLException();
}
}`
and put this method along with other validation methods for example
`
PathUtils.validatePath(clientPath, createMode.isSequential());
EphemeralType.validateTTL(createMode, -1);
validateACL(acl);
`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/279#discussion_r126593104
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -1436,7 +1436,7 @@ public String create(final String path, byte data[],
List acl,
request.setData(data);
request.setFlags(createMode.toFlag());
request.setPath(serverPath);
-if (acl != null && acl.size() == 0) {
+if (acl == null || acl.isEmpty() || acl.contains(null)) {
--- End diff --
+1 on this. We can throw IllegalArgumentException instead of
KeeperException (which would change API signature) on async methods. It's good
to catch the bad arguments at client side.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
Github user jainbhupendra24 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/279#discussion_r127447190
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -1436,7 +1436,7 @@ public String create(final String path, byte data[],
List acl,
request.setData(data);
request.setFlags(createMode.toFlag());
request.setPath(serverPath);
-if (acl != null && acl.size() == 0) {
+if (acl == null || acl.isEmpty() || acl.contains(null)) {
--- End diff --
Throwing IllegalArgumentException is the correct way but it may break the
existing behavior. Consider the case where application code is like
zk.create(callBackHandler)` // call to create api and callback
handler is passed
doSomething();
- - - - // some more stuff
At present, if client pass null ACL parameter, the error will be handled by
callBackHandler and in application code doSomething() method will execute.
But if we throw IllegalArgumentException , then doSomething() method will
not execute. So it will change the existing behavior of application code.
Please suggest if we really need to handle this case.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
Github user arshadmohammad commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/279#discussion_r128291070
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -1436,7 +1436,7 @@ public String create(final String path, byte data[],
List acl,
request.setData(data);
request.setFlags(createMode.toFlag());
request.setPath(serverPath);
-if (acl != null && acl.size() == 0) {
+if (acl == null || acl.isEmpty() || acl.contains(null)) {
--- End diff --
Thanks @jainbhupendra24 for the details. We can leave these functions as it
is. It is trivial problem we can ignore over the backward compatibility.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #279: ZOOKEEPER-2804:Node creation fails with NPE if ...
Github user asfgit closed the pull request at: https://github.com/apache/zookeeper/pull/279 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
