You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "EdColeman (via GitHub)" <gi...@apache.org> on 2023/02/15 20:48:26 UTC
[GitHub] [accumulo] EdColeman opened a new pull request, #3195: Use Collections.unmodifiableList for static ACL declarations
EdColeman opened a new pull request, #3195:
URL: https://github.com/apache/accumulo/pull/3195
- Uses Collections.unmodifiableList to limit the exposure of the PRIVATE and PUBLIC collections.
- Adds tests that replicate the current ZooKeeper check (in ZooKeeper, the method is private)
Spotbugs warns (MS_MUTABLE_COLLECTION) for the the public, static definitions in ZooUtil for PRIVATE and PUBLIC ZooKeeper ACLs that expose a mutable collection (ArrayList). This was discovered when refactoring ServiceLock from fate to another package in core. Verified, that with this change, spotbugs does not flag the issue.
Using an immutable collection like List.of() or List.copyOf() result in an NPE from ZooKeeper. ZooKeeper that performs a ACL check on operations like create,... The ZooKeeper check calls `acl.contains((Object) null)` which throws an NPE in Java (JDK-8265905)
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman merged pull request #3195: Use Collections.unmodifiableList for static ACL declarations
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman merged PR #3195:
URL: https://github.com/apache/accumulo/pull/3195
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3195: Use Collections.unmodifiableList for static ACL declarations
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3195:
URL: https://github.com/apache/accumulo/pull/3195#discussion_r1117149232
##########
core/src/test/java/org/apache/accumulo/core/fate/zookeeper/ZooUtilTest.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.fate.zookeeper;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.KeeperException.InvalidACLException;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.data.ACL;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+class ZooUtilTest {
+ Logger log = LoggerFactory.getLogger(ZooUtilTest.class);
+
+ @Test
+ void checkUnmodifiable() throws Exception {
+ assertTrue(validateACL(ZooUtil.PRIVATE));
+ assertTrue(validateACL(ZooUtil.PUBLIC));
+ }
+
+ /**
+ * This test replicates the acl check in ZooKeeper.java to show ZooKeeper will not accept an
+ * ImmutableCollection for the ACL list. Callers need to use Collections.unmodifiableList()
+ * instead of List.of() or List.copyOf(), because ImmutableCollections.contains() doesn't handle
+ * nulls properly (JDK-8265905) and ZooKeeper (as of 3.8.1) calls acl.contains((Object) null)
+ * which throws a NPE when passed an immutable collection
+ */
+ @Test
+ public void checkImmutableAcl() throws Exception {
+
+ final List<ACL> mutable = new ArrayList<>(ZooDefs.Ids.CREATOR_ALL_ACL);
+ assertTrue(validateACL(mutable));
+
+ try {
+ final List<ACL> immutable = List.copyOf(ZooDefs.Ids.CREATOR_ALL_ACL);
+ assertThrows(NullPointerException.class, () -> validateACL(immutable));
+ } catch (Exception ex) {
+ log.warn("validateAcls failed with exception", ex);
+ }
+ }
+
+ /**
+ * Copied from ZooKeeper 3.8.1,(ZooKeeper.validateACL())[] for stand-alone testing,
+ *
+ * @see <a
+ * href="https://github.com/apache/zookeeper/blob/2e9c3f3ceda90aeb9380acc87b253bf7661b7794/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java#L3075/>
+ */
+ boolean validateACL(List<ACL> acl) throws KeeperException.InvalidACLException {
Review Comment:
You know, we don't actually need the comments to be javadoc comments for tests. We're not generating javadocs for test code, so you're only going to see these comments looking at the source code. And, in that case, it's easier to omit the HTML and javadoc tags. Also, this method can be private, even if the original wasn't.
```suggestion
// Copied from ZooKeeper 3.8.1 for stand-alone testing here
// https://github.com/apache/zookeeper/blob/2e9c3f3ceda90aeb9380acc87b253bf7661b7794/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java#L3075/
private boolean validateACL(List<ACL> acl) throws KeeperException.InvalidACLException {
```
Also, the HTML was malformed in yours anyway... you forgot the closing double quote around the URL.
##########
core/src/test/java/org/apache/accumulo/core/fate/zookeeper/ZooUtilTest.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.fate.zookeeper;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.KeeperException.InvalidACLException;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.data.ACL;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+class ZooUtilTest {
+ Logger log = LoggerFactory.getLogger(ZooUtilTest.class);
+
+ @Test
+ void checkUnmodifiable() throws Exception {
+ assertTrue(validateACL(ZooUtil.PRIVATE));
+ assertTrue(validateACL(ZooUtil.PUBLIC));
+ }
+
+ /**
Review Comment:
Here also, this doesn't need to be a javadoc comment, especially since the classes mentioned in the body of this comment don't even use links, so this is just a freeform comment. It's easier to avoid breaking javadoc rules/conventions/expectations if we don't bother making it a javadoc when we don't need to in the first place, and we don't need to here.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3195: Use Collections.unmodifiableList for static ACL declarations
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3195:
URL: https://github.com/apache/accumulo/pull/3195#issuecomment-1432015968
Co-authored-by: @ctubbsii
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3195: Use Collections.unmodifiableList for static ACL declarations
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3195:
URL: https://github.com/apache/accumulo/pull/3195#discussion_r1117349185
##########
core/src/test/java/org/apache/accumulo/core/fate/zookeeper/ZooUtilTest.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.fate.zookeeper;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.KeeperException.InvalidACLException;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.data.ACL;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+class ZooUtilTest {
+ Logger log = LoggerFactory.getLogger(ZooUtilTest.class);
+
+ @Test
+ void checkUnmodifiable() throws Exception {
+ assertTrue(validateACL(ZooUtil.PRIVATE));
+ assertTrue(validateACL(ZooUtil.PUBLIC));
+ }
+
+ /**
Review Comment:
Addressed in c50465c65df
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org