Skip to content

[ZEPPELIN-6247] Fix flag parsing bug in FileInterpreter#4976

Merged
Reamer merged 2 commits into
apache:masterfrom
renechoi:fix-fileinterpreter-flag-parsing
Jul 21, 2025
Merged

[ZEPPELIN-6247] Fix flag parsing bug in FileInterpreter#4976
Reamer merged 2 commits into
apache:masterfrom
renechoi:fix-fileinterpreter-flag-parsing

Conversation

@renechoi

@renechoi renechoi commented Jul 15, 2025

Copy link
Copy Markdown
Contributor

[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:

// 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:

// 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

Fixed a bug where the dash character (-) was incorrectly included as a flag when parsing command arguments. Now only the actual flag characters after the dash are added to the flags set.

Added unit tests to verify the correct parsing behavior.

@Reamer Reamer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but please create a JIRA ticket for this.

@renechoi

renechoi commented Jul 16, 2025

Copy link
Copy Markdown
Contributor Author

@Reamer Thank you for the review! I've created a JIRA ticket as requested: ZEPPELIN-6247

I'll update the PR title to include the JIRA issue number.

@renechoi renechoi changed the title [MINOR] Fix flag parsing bug in FileInterpreter [ZEPPELIN-6247] Fix flag parsing bug in FileInterpreter Jul 16, 2025
@Reamer

Reamer commented Jul 17, 2025

Copy link
Copy Markdown
Contributor

@renechoi Two Checkstyle errors

[INFO] --- checkstyle:3.4.0:check (checkstyle-fail-build) @ zeppelin-file ---
[INFO] There are 2 errors reported by Checkstyle 9.3 with zeppelin/checkstyle.xml ruleset.
Error:  src/test/java/org/apache/zeppelin/file/FileInterpreterTest.java:[1] (misc) NewlineAtEndOfFile: File does not end with a newline.
Error:  src/test/java/org/apache/zeppelin/file/FileInterpreterTest.java:[39,5] (modifier) RedundantModifier: Redundant 'public' modifier.

Fixed two checkstyle violations:
- Added newline at end of file (NewlineAtEndOfFile rule)
- Removed redundant 'public' modifier from inner class constructor (RedundantModifier rule)
@renechoi

Copy link
Copy Markdown
Contributor Author

@Reamer Thank you for catching those checkstyle errors! I've fixed both issues:

  1. Added the missing newline at the end of FileInterpreterTest.java
  2. Removed the redundant 'public' modifier from the TestFileInterpreter constructor

The changes have been applied and the code should now pass all checkstyle checks.

@Reamer

Reamer commented Jul 21, 2025

Copy link
Copy Markdown
Contributor

Let's wait for CI.

@Reamer Reamer self-assigned this Jul 21, 2025
@Reamer Reamer merged commit 1a66333 into apache:master Jul 21, 2025
16 of 18 checks passed
asf-gitbox-commits pushed a commit that referenced this pull request Jul 21, 2025
# [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 <philipp.dallig@gmail.com>
(cherry picked from commit 1a66333)
Signed-off-by: Philipp Dallig <philipp.dallig@gmail.com>
@Reamer

Reamer commented Jul 21, 2025

Copy link
Copy Markdown
Contributor

Merged into master and branch-0.12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants