Skip to content

feat: add view-based filtering support for sheet writing - #925

Open
bengbengbalabalabeng wants to merge 17 commits into
apache:mainfrom
bengbengbalabalabeng:feature-issues-854
Open

feat: add view-based filtering support for sheet writing#925
bengbengbalabalabeng wants to merge 17 commits into
apache:mainfrom
bengbengbalabalabeng:feature-issues-854

Conversation

@bengbengbalabalabeng

Copy link
Copy Markdown
Contributor

Purpose of the pull request

Closed: #854

What's changed?

For usage examples, please refer to: Proposal

  • Introduce a new annotation @ExcelView to handle View-based filtering logic, supporting both Type-based and String-based modes through asTypes and asNames.
  • Extend the original filtering logic chain of fesod-sheet.

Filtering Priority:

@ExcelIgnore / @ExcelIgnoreUnannotated > [NEW] @ExcelView > includeColumn* / excludeColumn*

Checklist

  • I have read the Contributor Guide.
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

@bengbengbalabalabeng
bengbengbalabalabeng marked this pull request as ready for review June 3, 2026 13:30

Copilot AI 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.

Pull request overview

This PR adds view-based column filtering for sheet writing (Closed: #854) by introducing a new @ExcelView annotation and a groups(...) API on the writer builder to select fields by type-based or string-based “view” identifiers, integrating the view filter into the existing write-time ignore/include/exclude pipeline. It also adds unit/integration tests to validate header output across XLS/XLSX/CSV.

Changes:

  • Introduces @ExcelView plus view matchers (WriteViewMatcher, class-based, name-based, and default/noop) to decide which fields are exported.
  • Extends the writer builder with groups(Class<?>...) and groups(String...) to activate view filtering.
  • Adds tests covering typed/name/mixed view scenarios and updates ClassUtilsTest to account for the new write-holder matcher dependency.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
fesod-sheet/src/main/java/org/apache/fesod/sheet/annotation/write/ExcelView.java Adds @ExcelView annotation used to tag fields with view identifiers for write-time filtering.
fesod-sheet/src/main/java/org/apache/fesod/sheet/util/ClassUtils.java Integrates view filtering into field discovery/ignore logic and caches the matcher in the cache key.
fesod-sheet/src/main/java/org/apache/fesod/sheet/write/builder/AbstractExcelWriterParameterBuilder.java Adds groups(...) APIs that set the active view matcher for a write operation.
fesod-sheet/src/main/java/org/apache/fesod/sheet/write/metadata/WriteBasicParameter.java Adds writeViewMatcher parameter to carry view selection through writer configuration.
fesod-sheet/src/main/java/org/apache/fesod/sheet/write/metadata/holder/WriteHolder.java Exposes writeViewMatcher() to downstream write logic.
fesod-sheet/src/main/java/org/apache/fesod/sheet/write/metadata/holder/AbstractWriteHolder.java Initializes/inherits writeViewMatcher (defaults to noop).
fesod-sheet/src/main/java/org/apache/fesod/sheet/write/view/WriteViewMatcher.java Defines the strategy interface for write-time view matching.
fesod-sheet/src/main/java/org/apache/fesod/sheet/write/view/NoopWriteViewMatcher.java Provides the default matcher used when no views are configured.
fesod-sheet/src/main/java/org/apache/fesod/sheet/write/view/NameBasedViewMatcher.java Implements string-based view matching via @ExcelView(asNames=...).
fesod-sheet/src/main/java/org/apache/fesod/sheet/write/view/ClassBasedViewMatcher.java Implements type-based view matching via @ExcelView(asTypes=...).
fesod-sheet/src/test/java/org/apache/fesod/sheet/util/ClassUtilsTest.java Updates existing field-cache tests and adds new tests for typed/named view filtering.
fesod-sheet/src/test/java/org/apache/fesod/sheet/view/WriteSheetViewTests.java Adds end-to-end header verification tests across XLS/XLSX/CSV for view filtering behavior.
fesod-sheet/src/test/java/org/apache/fesod/sheet/view/WriteViewStrategy.java Test-only view marker types used by typed view tests.
fesod-sheet/src/test/java/org/apache/fesod/sheet/view/WriteTypedViewsData.java Test data model using @ExcelView(asTypes=...).
fesod-sheet/src/test/java/org/apache/fesod/sheet/view/WriteNamedViewsData.java Test data model using @ExcelView(asNames=...).
fesod-sheet/src/test/java/org/apache/fesod/sheet/view/WriteMixedViewData.java Test data model combining both asTypes and asNames, plus an unannotated field.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +22 to +26
import java.lang.annotation.ElementType;
import java.lang.annotation.Inherited;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines +48 to +51
@Target(ElementType.FIELD)
@Retention(RetentionPolicy.RUNTIME)
@Inherited
public @interface ExcelView {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines +53 to +57
/**
* View or views that annotated element is part of. Views are identified
* by classes, and use expected class inheritance relationship: child
* views contain all elements parent views have.
*/

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines +57 to +59
return Arrays.stream(fieldGroups).anyMatch(fieldGroup -> expectedGroups.stream()
.anyMatch(expectedGroup -> expectedGroup.isAssignableFrom(fieldGroup)));
}
Comment on lines +57 to +59
return Arrays.stream(fieldGroups).anyMatch(fieldGroup -> expectedGroups.stream()
.anyMatch(expectedGroup -> expectedGroup.equals(fieldGroup)));
}
Comment on lines +183 to +186
public T groups(Class<?>... types) {
parameter().setWriteViewMatcher(new ClassBasedViewMatcher(Arrays.asList(types)));
return self();
}
Comment on lines +194 to +197
public T groups(String... names) {
parameter().setWriteViewMatcher(new NameBasedViewMatcher(Arrays.asList(names)));
return self();
}
Comment on lines +58 to +61
void setUp(@TempDir Path tempDir) {
write03 = createTmpFile(tempDir, "write03.xls");
write07 = createTmpFile(tempDir, "write07.xls");
writeCsv = createTmpFile(tempDir, "writeCsv.csv");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines +38 to +41
@Override
public boolean matches(Field field) {
return false;
}
- Remove @inherited meta-annotation in @ExcelView
- Correct the javadoc of ExcelView#asTypes
- Refine WriteViewMatcher parameter validation

Copilot AI 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.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.

Suppressed comments (7)

fesod-sheet/src/main/java/org/apache/fesod/sheet/annotation/write/ExcelView.java:41

  • The Javadoc implies a field annotated with BasicView.class would also be included when the active view is a subclass ("BasicView.class (or its subclass)"), but the implemented matcher only includes fields when selectedView.isAssignableFrom(annotatedView) (i.e., selecting a subclass does not include fields annotated with a supertype). Please reword to match the actual behavior and avoid API confusion.
 * which would specify that field annotated would be included
 * when processing (writing) Sheet identified by <code>BasicView.class</code> (or its subclass) or
 * <code>"BasicView"</code>.
 * If multiple View class or string identifiers are included, the field will be part of all of them.

fesod-sheet/src/main/java/org/apache/fesod/sheet/write/builder/AbstractExcelWriterParameterBuilder.java:203

  • groups(String... names) throws for an empty array but allows null elements, which will later NPE in NameBasedViewMatcher#matches. Since this is a public API entry point, it should fail fast with a clear IllegalArgumentException when any element is null.
    public T groups(String... names) {
        if (ArrayUtils.isEmpty(names)) {
            throw new IllegalArgumentException("Names must not be empty");
        }
        parameter().setWriteViewMatcher(new NameBasedViewMatcher(names));

fesod-sheet/src/main/java/org/apache/fesod/sheet/write/view/NameBasedViewMatcher.java:50

  • expectedGroups is wrapped with Collections.unmodifiableCollection(expectedGroups) but not defensively copied, so later mutations of the caller-provided collection will change matcher behavior (and can also destabilize cache keys that include the matcher). Create an internal copy before wrapping it unmodifiable.
        if (CollectionUtils.isEmpty(expectedGroups)) {
            throw new IllegalArgumentException("Name-based view groups must not be empty");
        }
        this.expectedGroups = Collections.unmodifiableCollection(expectedGroups);
    }

fesod-sheet/src/main/java/org/apache/fesod/sheet/write/view/ClassBasedViewMatcher.java:50

  • expectedGroups is wrapped with Collections.unmodifiableCollection(expectedGroups) but not defensively copied, so later mutations of the caller-provided collection will change matcher behavior (and can also destabilize cache keys that include the matcher). Create an internal copy before wrapping it unmodifiable.
        if (CollectionUtils.isEmpty(expectedGroups)) {
            throw new IllegalArgumentException("Type-based view groups must not be empty");
        }
        this.expectedGroups = Collections.unmodifiableCollection(expectedGroups);
    }

fesod-sheet/src/main/java/org/apache/fesod/sheet/util/ClassUtils.java:353

  • writeHolder.writeViewMatcher() is assumed non-null; if any WriteHolder implementation (or a Mockito mock) returns null, this will throw an NPE when computing hasViews / calling matches. Treat null as WriteViewMatcher.NOOP to keep write behavior backwards-compatible and robust.
        // ignore field by grouping
        WriteViewMatcher writeViewMatcher = writeHolder.writeViewMatcher();
        boolean hasViews = (WriteViewMatcher.NOOP != writeViewMatcher);
        // ignore field by include*/exclude*

fesod-sheet/src/test/java/org/apache/fesod/sheet/view/WriteSheetViewTests.java:73

  • CSVParser is Closeable, but it is not closed here. Wrap it in try-with-resources to avoid leaking file handles, especially on Windows where open handles can block temp file cleanup.
                CSVParser parser = CSVFormat.DEFAULT.withFirstRecordAsHeader().parse(reader);

                Map<String, Integer> headerMap = parser.getHeaderMap();

fesod-sheet/src/main/java/org/apache/fesod/sheet/write/builder/AbstractExcelWriterParameterBuilder.java:188

  • groups(Class<?>... types) throws for an empty array but allows null elements, which will later NPE in ClassBasedViewMatcher#matches. Since this is a public API entry point, it should fail fast with a clear IllegalArgumentException when any element is null.

This issue also appears on line 199 of the same file.

    public T groups(Class<?>... types) {
        if (ArrayUtils.isEmpty(types)) {
            throw new IllegalArgumentException("Types must not be empty");
        }
        parameter().setWriteViewMatcher(new ClassBasedViewMatcher(types));

@alaahong alaahong left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No more concern on tech part, will anyone comment on the usage part too?

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.

[Task] ExcelProperty添加分组字段,可以根据传参动态渲染表头数量

3 participants