aajisaka commented on code in PR #8259: URL: https://github.com/apache/hadoop/pull/8259#discussion_r3331459870
########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/find/TestSize.java: ########## @@ -0,0 +1,151 @@ +/** + * 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 + * + * http://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.hadoop.fs.shell.find; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.Mockito.*; +import static org.apache.hadoop.fs.shell.find.TestHelper.*; + +import java.io.IOException; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import org.apache.hadoop.fs.FileStatus; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.shell.PathData; + +public class TestSize { + private FileSystem mockFs; + + private PathData fiveBlocks; + private PathData fourBlocks; + private PathData sixBlocks; + private PathData fiveBlocksMinus; + private PathData fiveBlocksPlus; + + @BeforeEach + public void resetMock() throws IOException { + mockFs = MockFileSystem.setup(); + + fiveBlocks = createPathData("fiveBlocks", 5L * 512L); + sixBlocks = createPathData("sixBlocks", 6L * 512L); + fourBlocks = createPathData("fourBlocks", 4L * 512L); + fiveBlocksPlus = createPathData("fiveBlocksPlus", (5L * 512L) + 511L); + fiveBlocksMinus = createPathData("fiveBlocksMinus", (5L * 512L) - 1L); + } + + private PathData createPathData(String name, long length) throws IOException { + Path path = new Path(name); + FileStatus fileStatus = mock(FileStatus.class); + when(fileStatus.getLen()).thenReturn(length); + when(mockFs.getFileStatus(eq(path))).thenReturn(fileStatus); + return new PathData(name, mockFs.getConf()); + } + + // test exact match in blocks + @Test + public void applyEqualsBlock() throws IOException { + Size size = new Size(); + addArgument(size, "5"); + size.setOptions(new FindOptions()); + size.prepare(); + + assertEquals(Result.PASS, size.apply(fiveBlocks, -1)); + assertEquals(Result.FAIL, size.apply(sixBlocks, -1)); + assertEquals(Result.FAIL, size.apply(fourBlocks, -1)); + assertEquals(Result.PASS, size.apply(fiveBlocksPlus, -1)); + assertEquals(Result.FAIL, size.apply(fiveBlocksMinus, -1)); + } + + // test greater than match in blocks + @Test + public void applyGreaterThanBlock() throws IOException { + Size size = new Size(); + addArgument(size, "+5"); + size.setOptions(new FindOptions()); + size.prepare(); + + assertEquals(Result.FAIL, size.apply(fiveBlocks, -1)); + assertEquals(Result.PASS, size.apply(sixBlocks, -1)); + assertEquals(Result.FAIL, size.apply(fourBlocks, -1)); + assertEquals(Result.FAIL, size.apply(fiveBlocksPlus, -1)); Review Comment: fiveBlocksPlus should pass ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/find/NumberExpression.java: ########## @@ -0,0 +1,109 @@ +/** + * 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 + * + * http://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.hadoop.fs.shell.find; + +import java.io.IOException; +import java.util.Deque; + +/** + * Base class for numeric {@link Expression}s. + * Implements helper methods for processing numeric arguments. + */ +public abstract class NumberExpression extends BaseExpression { + /** + * Number of milliseconds in a day. + */ + protected static final long DAY_IN_MILLISECONDS = 86400000L; + + /** + * Number of milliseconds in a minute. + */ + protected static final long MINUTE_IN_MILLISECONDS = 60000L; + + private long max = -1L; + private long min = -1L; + private long units = 1L; + + /** + * Constructor specifying for size of units to used as the expression + * argument. + * + * @param units size of the expression argument compared to the item being + * processed. + */ + protected NumberExpression(long units) { + setUnits(units); + } + + protected NumberExpression() { + this(1L); + } + + protected void setUnits(long units) { + this.units = units; + } + + @Override + public void prepare() throws IOException { + parseArgument(getArgument(1)); + } + + /** + * Parse the argument string to extract the numeric argument. + * + * @param arg String to be parsed + * @throws IllegalArgumentException if there is a problem parsing the argument + */ + protected void parseArgument(String arg) throws IllegalArgumentException { + if (arg == null) { + throw new IllegalArgumentException("Invalid null argument"); + } else if (arg.isEmpty()) { + throw new IllegalArgumentException("Invalid empty argument"); + } + if (arg.startsWith("+")) { + min = (Long.parseLong(arg.substring(1)) * units) + units; + } else if (arg.startsWith("-")) { + max = (Long.parseLong(arg.substring(1)) * units) - 1L; Review Comment: It sets `max = -1` for arguments like `-size -0` / `-mtime -0`. Then `applyNumber` treats `-1` as unset and the upper bound is ignored. This makes `-0` pass all entries. ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/find/TestSize.java: ########## @@ -0,0 +1,151 @@ +/** + * 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 + * + * http://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.hadoop.fs.shell.find; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.Mockito.*; +import static org.apache.hadoop.fs.shell.find.TestHelper.*; + +import java.io.IOException; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import org.apache.hadoop.fs.FileStatus; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.shell.PathData; + +public class TestSize { + private FileSystem mockFs; + + private PathData fiveBlocks; + private PathData fourBlocks; + private PathData sixBlocks; + private PathData fiveBlocksMinus; + private PathData fiveBlocksPlus; + + @BeforeEach + public void resetMock() throws IOException { + mockFs = MockFileSystem.setup(); + + fiveBlocks = createPathData("fiveBlocks", 5L * 512L); + sixBlocks = createPathData("sixBlocks", 6L * 512L); + fourBlocks = createPathData("fourBlocks", 4L * 512L); + fiveBlocksPlus = createPathData("fiveBlocksPlus", (5L * 512L) + 511L); + fiveBlocksMinus = createPathData("fiveBlocksMinus", (5L * 512L) - 1L); + } + + private PathData createPathData(String name, long length) throws IOException { + Path path = new Path(name); + FileStatus fileStatus = mock(FileStatus.class); + when(fileStatus.getLen()).thenReturn(length); + when(mockFs.getFileStatus(eq(path))).thenReturn(fileStatus); + return new PathData(name, mockFs.getConf()); + } + + // test exact match in blocks + @Test + public void applyEqualsBlock() throws IOException { + Size size = new Size(); + addArgument(size, "5"); + size.setOptions(new FindOptions()); + size.prepare(); + + assertEquals(Result.PASS, size.apply(fiveBlocks, -1)); + assertEquals(Result.FAIL, size.apply(sixBlocks, -1)); + assertEquals(Result.FAIL, size.apply(fourBlocks, -1)); + assertEquals(Result.PASS, size.apply(fiveBlocksPlus, -1)); + assertEquals(Result.FAIL, size.apply(fiveBlocksMinus, -1)); + } + + // test greater than match in blocks + @Test + public void applyGreaterThanBlock() throws IOException { + Size size = new Size(); + addArgument(size, "+5"); + size.setOptions(new FindOptions()); + size.prepare(); + + assertEquals(Result.FAIL, size.apply(fiveBlocks, -1)); + assertEquals(Result.PASS, size.apply(sixBlocks, -1)); + assertEquals(Result.FAIL, size.apply(fourBlocks, -1)); + assertEquals(Result.FAIL, size.apply(fiveBlocksPlus, -1)); + assertEquals(Result.FAIL, size.apply(fiveBlocksMinus, -1)); + } + + // test less than match in blocks + @Test + public void applyLessThanBlock() throws IOException { + Size size = new Size(); + addArgument(size, "-5"); + size.setOptions(new FindOptions()); + size.prepare(); + + assertEquals(Result.FAIL, size.apply(fiveBlocks, -1)); + assertEquals(Result.FAIL, size.apply(sixBlocks, -1)); + assertEquals(Result.PASS, size.apply(fourBlocks, -1)); + assertEquals(Result.FAIL, size.apply(fiveBlocksPlus, -1)); + assertEquals(Result.PASS, size.apply(fiveBlocksMinus, -1)); Review Comment: fiveBlocksMinus should fail, because it's rounded up to 5 blocks. ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/find/TestSize.java: ########## @@ -0,0 +1,151 @@ +/** + * 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 + * + * http://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.hadoop.fs.shell.find; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.Mockito.*; +import static org.apache.hadoop.fs.shell.find.TestHelper.*; + +import java.io.IOException; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import org.apache.hadoop.fs.FileStatus; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.shell.PathData; + +public class TestSize { + private FileSystem mockFs; + + private PathData fiveBlocks; + private PathData fourBlocks; + private PathData sixBlocks; + private PathData fiveBlocksMinus; + private PathData fiveBlocksPlus; + + @BeforeEach + public void resetMock() throws IOException { + mockFs = MockFileSystem.setup(); + + fiveBlocks = createPathData("fiveBlocks", 5L * 512L); + sixBlocks = createPathData("sixBlocks", 6L * 512L); + fourBlocks = createPathData("fourBlocks", 4L * 512L); + fiveBlocksPlus = createPathData("fiveBlocksPlus", (5L * 512L) + 511L); + fiveBlocksMinus = createPathData("fiveBlocksMinus", (5L * 512L) - 1L); + } + + private PathData createPathData(String name, long length) throws IOException { + Path path = new Path(name); + FileStatus fileStatus = mock(FileStatus.class); + when(fileStatus.getLen()).thenReturn(length); + when(mockFs.getFileStatus(eq(path))).thenReturn(fileStatus); + return new PathData(name, mockFs.getConf()); + } + + // test exact match in blocks + @Test + public void applyEqualsBlock() throws IOException { + Size size = new Size(); + addArgument(size, "5"); + size.setOptions(new FindOptions()); + size.prepare(); + + assertEquals(Result.PASS, size.apply(fiveBlocks, -1)); + assertEquals(Result.FAIL, size.apply(sixBlocks, -1)); + assertEquals(Result.FAIL, size.apply(fourBlocks, -1)); + assertEquals(Result.PASS, size.apply(fiveBlocksPlus, -1)); + assertEquals(Result.FAIL, size.apply(fiveBlocksMinus, -1)); Review Comment: In this case, fiveBocksMinus should pass and fiveBlocksPlus should fail. This is because fiveBlocksMinus fits in 5 blocks and fiveBLocksPlus exceeds 5 blocks. ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/find/Perm.java: ########## @@ -0,0 +1,209 @@ +/** + * 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 + * + * http://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.hadoop.fs.shell.find; + +import java.io.IOException; +import java.util.Deque; + +import org.apache.hadoop.fs.permission.FsPermission; +import org.apache.hadoop.fs.shell.PathData; + +/** + * Implements the -perm expression for the {@link org.apache.hadoop.fs.shell.find.Find} command. + */ +final class Perm extends BaseExpression { + /** + * Registers this expression with the specified factory. + */ + public static void registerExpression(ExpressionFactory factory) throws IOException { + factory.addClass(Perm.class, "-perm"); + } + + private static final String[] USAGE = {"-perm [-]mode", "-perm [-]onum"}; + private static final String[] HELP = {"Evaluates as true if the file permissions match that", + "specified. If the hyphen is specified then the expression", + "shall evaluate as true if at least the bits specified", + "match, otherwise an exact match is required.", + "The mode may be specified using either symbolic notation,", + "eg 'u=rwx,g+x+w' or as an octal number."}; Review Comment: Does `g+x+w` pass? Looks like it's rejected in L132. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
