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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
