This is an automated email from the ASF dual-hosted git repository.
pdallig pushed a commit to branch branch-0.12
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/branch-0.12 by this push:
new 89316a2ff9 [ZEPPELIN-6247] Fix flag parsing bug in FileInterpreter
89316a2ff9 is described below
commit 89316a2ff9013b79e8800a3042b2c5ce538b4685
Author: renechoi <[email protected]>
AuthorDate: Mon Jul 21 20:47:12 2025 +0900
[ZEPPELIN-6247] Fix flag parsing bug in FileInterpreter
# [MINOR] Fix flag parsing bug in FileInterpreter
## What is this PR for?
This PR fixes a bug in the `FileInterpreter` class where the dash character
(`-`) was incorrectly included as a flag when parsing command arguments.
## What type of PR is it?
Bug Fix
## What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-6247
## How should this be tested?
1. Run the new unit tests: `FileInterpreterTest.java`
2. Verify that flag parsing works correctly:
- Command: `ls -l` should only have flag `'l'`, not `'-'` and `'l'`
- Command: `ls -lah` should have flags `'l'`, `'a'`, `'h'`, without `'-'`
## Screenshots (if appropriate)
N/A
## Questions:
* Does the license files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
## Description
### Problem
The current implementation of `parseArg()` method in `FileInterpreter.java`
incorrectly includes the dash character as a flag:
```java
// Before (line 73)
for (int i = 0; i < arg.length(); i++) {
```
When parsing `-l`, this adds both `'-'` and `'l'` to the flags HashSet.
### Solution
Start the loop from index 1 to skip the dash character:
```java
// After
for (int i = 1; i < arg.length(); i++) {
```
### Impact Analysis
- **Current behavior is not affected** because the existing code
(`HDFSFileInterpreter`) only checks for specific flag characters like `'l'` and
`'h'`
- However, this is a **correctness issue** that could cause problems in
future implementations
- The fix ensures that only actual flag characters are stored in the flags
set
### Test Coverage
Added comprehensive unit tests in `FileInterpreterTest.java` to verify:
- Single flags (`-l`)
- Multiple flags (`-la`, `-lah`)
- Separate flags (`-l -h`)
- Edge cases (empty flag `-`)
- Verification that dash is not included in the flags set
Closes #4976 from renechoi/fix-fileinterpreter-flag-parsing.
Signed-off-by: Philipp Dallig <[email protected]>
(cherry picked from commit 1a66333d50642c996a5bc3035154337eac4bcdc9)
Signed-off-by: Philipp Dallig <[email protected]>
---
.../org/apache/zeppelin/file/FileInterpreter.java | 2 +-
.../apache/zeppelin/file/FileInterpreterTest.java | 181 +++++++++++++++++++++
2 files changed, 182 insertions(+), 1 deletion(-)
diff --git a/file/src/main/java/org/apache/zeppelin/file/FileInterpreter.java
b/file/src/main/java/org/apache/zeppelin/file/FileInterpreter.java
index 9f87a0275a..286d315ba8 100644
--- a/file/src/main/java/org/apache/zeppelin/file/FileInterpreter.java
+++ b/file/src/main/java/org/apache/zeppelin/file/FileInterpreter.java
@@ -70,7 +70,7 @@ public abstract class FileInterpreter extends Interpreter {
private void parseArg(String arg) {
if (arg.charAt(0) == '-') { // handle flags
- for (int i = 0; i < arg.length(); i++) {
+ for (int i = 1; i < arg.length(); i++) {
Character c = arg.charAt(i);
flags.add(c);
}
diff --git
a/file/src/test/java/org/apache/zeppelin/file/FileInterpreterTest.java
b/file/src/test/java/org/apache/zeppelin/file/FileInterpreterTest.java
new file mode 100644
index 0000000000..6097d5fc59
--- /dev/null
+++ b/file/src/test/java/org/apache/zeppelin/file/FileInterpreterTest.java
@@ -0,0 +1,181 @@
+/**
+ * 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.zeppelin.file;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+import java.util.Properties;
+
+import org.apache.zeppelin.interpreter.InterpreterException;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for FileInterpreter CommandArgs parsing functionality.
+ */
+class FileInterpreterTest {
+
+ /**
+ * Mock FileInterpreter for testing CommandArgs functionality
+ */
+ private static class TestFileInterpreter extends FileInterpreter {
+ TestFileInterpreter(Properties property) {
+ super(property);
+ }
+
+ @Override
+ public String listAll(String path) throws InterpreterException {
+ return "";
+ }
+
+ @Override
+ public boolean isDirectory(String path) {
+ return true;
+ }
+
+ @Override
+ public void open() {
+ }
+
+ @Override
+ public void close() {
+ }
+
+ // Expose CommandArgs for testing
+ public CommandArgs getCommandArgs(String cmd) {
+ CommandArgs args = new CommandArgs(cmd);
+ args.parseArgs();
+ return args;
+ }
+ }
+
+ @Test
+ void testCommandArgsParsing() {
+ TestFileInterpreter interpreter = new TestFileInterpreter(new
Properties());
+
+ // Test simple command without flags
+ FileInterpreter.CommandArgs args1 = interpreter.getCommandArgs("ls");
+ assertEquals("ls", args1.command);
+ assertEquals(0, args1.args.size());
+ assertEquals(0, args1.flags.size());
+
+ // Test command with path
+ FileInterpreter.CommandArgs args2 = interpreter.getCommandArgs("ls /user");
+ assertEquals("ls", args2.command);
+ assertEquals(1, args2.args.size());
+ assertEquals("/user", args2.args.get(0));
+ assertEquals(0, args2.flags.size());
+
+ // Test command with single flag
+ FileInterpreter.CommandArgs args3 = interpreter.getCommandArgs("ls -l");
+ assertEquals("ls", args3.command);
+ assertEquals(0, args3.args.size());
+ assertEquals(1, args3.flags.size());
+ assertTrue(args3.flags.contains('l'));
+ assertFalse(args3.flags.contains('-'));
+
+ // Test command with multiple flags
+ FileInterpreter.CommandArgs args4 = interpreter.getCommandArgs("ls -la");
+ assertEquals("ls", args4.command);
+ assertEquals(0, args4.args.size());
+ assertEquals(2, args4.flags.size());
+ assertTrue(args4.flags.contains('l'));
+ assertTrue(args4.flags.contains('a'));
+ assertFalse(args4.flags.contains('-'));
+
+ // Test command with flags and path
+ FileInterpreter.CommandArgs args5 = interpreter.getCommandArgs("ls -l
/user");
+ assertEquals("ls", args5.command);
+ assertEquals(1, args5.args.size());
+ assertEquals("/user", args5.args.get(0));
+ assertEquals(1, args5.flags.size());
+ assertTrue(args5.flags.contains('l'));
+ assertFalse(args5.flags.contains('-'));
+
+ // Test command with separate flags
+ FileInterpreter.CommandArgs args6 = interpreter.getCommandArgs("ls -l -h
/user");
+ assertEquals("ls", args6.command);
+ assertEquals(1, args6.args.size());
+ assertEquals("/user", args6.args.get(0));
+ assertEquals(2, args6.flags.size());
+ assertTrue(args6.flags.contains('l'));
+ assertTrue(args6.flags.contains('h'));
+ assertFalse(args6.flags.contains('-'));
+
+ // Test command with combined flags
+ FileInterpreter.CommandArgs args7 = interpreter.getCommandArgs("ls -lah
/user");
+ assertEquals("ls", args7.command);
+ assertEquals(1, args7.args.size());
+ assertEquals("/user", args7.args.get(0));
+ assertEquals(3, args7.flags.size());
+ assertTrue(args7.flags.contains('l'));
+ assertTrue(args7.flags.contains('a'));
+ assertTrue(args7.flags.contains('h'));
+ assertFalse(args7.flags.contains('-'));
+ }
+
+ @Test
+ void testCommandArgsWithDashNotInFlags() {
+ TestFileInterpreter interpreter = new TestFileInterpreter(new
Properties());
+
+ // Test that dash character is not included in flags after fix
+ FileInterpreter.CommandArgs args = interpreter.getCommandArgs("ls -l");
+
+ // Verify dash is not in flags
+ assertFalse(args.flags.contains('-'),
+ "Dash character should not be included in flags");
+
+ // Verify correct flag is included
+ assertTrue(args.flags.contains('l'),
+ "Flag 'l' should be included");
+
+ // Verify flag count
+ assertEquals(1, args.flags.size(),
+ "Should only have one flag character");
+ }
+
+ @Test
+ void testEmptyFlags() {
+ TestFileInterpreter interpreter = new TestFileInterpreter(new
Properties());
+
+ // Test empty flag (just dash)
+ FileInterpreter.CommandArgs args = interpreter.getCommandArgs("ls -");
+ assertEquals("ls", args.command);
+ assertEquals(0, args.args.size());
+ assertEquals(0, args.flags.size());
+ }
+
+ @Test
+ void testComplexCommand() {
+ TestFileInterpreter interpreter = new TestFileInterpreter(new
Properties());
+
+ // Test complex command with multiple flags and arguments
+ FileInterpreter.CommandArgs args = interpreter.getCommandArgs("ls -la -h
/user /tmp");
+ assertEquals("ls", args.command);
+ assertEquals(2, args.args.size());
+ assertEquals("/user", args.args.get(0));
+ assertEquals("/tmp", args.args.get(1));
+ assertEquals(3, args.flags.size());
+ assertTrue(args.flags.contains('l'));
+ assertTrue(args.flags.contains('a'));
+ assertTrue(args.flags.contains('h'));
+ assertFalse(args.flags.contains('-'));
+ }
+}
