Skip to content

Conversation

@alaahong
Copy link
Member

Purpose of the pull request

Related: #425

  1. Add fieldNameToField to filter final ignore field
  2. Add necessary case for verification

What's changed?

fastexcel-core/src/main/java/cn/idev/excel/util/ClassUtils.java

Checklist

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

1. Add fieldNameToField to filter final ignore field
2. Add necessary case for verification
@alaahong alaahong changed the title fix: ExcelIgnore won't ignore in child class fix: FastExcel won't ignore the filed while both annotated @ExcelIgnore in Child and @ExcelProperty in Parent Jul 21, 2025
@alaahong alaahong changed the title fix: FastExcel won't ignore the filed while both annotated @ExcelIgnore in Child and @ExcelProperty in Parent fix: FastExcel won't ignore the field while both annotated @ExcelIgnore in Child and @ExcelProperty in Parent Jul 21, 2025
@psxjoy psxjoy requested a review from Copilot July 21, 2025 14:17
Copy link
Contributor

Copilot AI left a comment

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 fixes a bug where FastExcel incorrectly processes fields when a child class uses @ExcelIgnore on a field that has @ExcelProperty in the parent class. The fix ensures that child class annotations properly override parent class annotations for field processing.

  • Refactored field discovery logic to prioritize child class field annotations over parent class annotations
  • Added filtering logic to collect all @ExcelIgnore annotated fields before processing
  • Comprehensive test coverage for all inheritance scenarios with @ExcelProperty and @ExcelIgnore combinations

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
fastexcel-core/src/main/java/cn/idev/excel/util/ClassUtils.java Modified field discovery algorithm to properly handle annotation inheritance and field overriding
fastexcel-test/src/test/java/cn/idev/excel/test/core/skip/ClassUtilsFieldOverrideTest.java Added comprehensive test cases covering all scenarios of field annotation inheritance

@psxjoy
Copy link
Member

psxjoy commented Jul 21, 2025

I'm just using GitHub Copilot for some common code-review suggestions. If no changes are necessary, I'll do a code-review ASAP :)

alaahong and others added 2 commits July 21, 2025 22:24
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@alaahong
Copy link
Member Author

I'm just using GitHub Copilot for some common code-review suggestions. If no changes are necessary, I'll do a code-review ASAP :)

Seems copilot only find out typo~

@mymde
Copy link

mymde commented Jul 22, 2025

Unlike method overriding, the "field" that the subclass redefines is its own and has no direct connection to the parent class's "field". If you want the parent class's "field" to be ignored, simply add the @ExcelIgnore annotation to the parent class's "field".
和方法重写不同,我认为子类变量和父类变量相互独立。如果想忽略父类变量,可以在其上加@ExcelIgnore注解

@alaahong
Copy link
Member Author

Unlike method overriding, the "field" that the subclass redefines is its own and has no direct connection to the parent class's "field". If you want the parent class's "field" to be ignored, simply add the @ExcelIgnore annotation to the parent class's "field". 和方法重写不同,我认为子类变量和父类变量相互独立。如果想忽略父类变量,可以在其上加@ExcelIgnore注解

Do you mean to update parent annotation? Hmmm...

In my opinion , we should not modify the parent behavior for child requirement.
e.g.
A parent class with two child classes

    class Fruit {        
        @ExcelProperty("parent1Field")
        private String field;
    }

    class Apple extends Fruit {
       @ExcelIgnore
        private String field;
    }
    class Orange extends Fruit {
         private String field;
    }

If ignore Apple field, we should not impact Orange. So it's fine to just update the child but not the parent.

…re in Child and @ExcelProperty in Parent

1. Add more cases for no annotation case
@psxjoy
Copy link
Member

psxjoy commented Jul 22, 2025

This PR conflicts due to some previous changes. Would you be willing to fix these conflicts and submit it again?

Copy link
Member

@psxjoy psxjoy left a comment

Choose a reason for hiding this comment

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

PR conflict

alaahong added 2 commits July 23, 2025 09:17
…on_child_class

# Conflicts:
#	fastexcel/src/main/java/cn/idev/excel/util/ClassUtils.java
…re in Child and @ExcelProperty in Parent

1. Format with spotless apply
@alaahong alaahong requested a review from psxjoy July 23, 2025 01:48
@mymde
Copy link

mymde commented Jul 24, 2025

Unlike method overriding, the "field" that the subclass redefines is its own and has no direct connection to the parent class's "field". If you want the parent class's "field" to be ignored, simply add the @ExcelIgnore annotation to the parent class's "field". 和方法重写不同,我认为子类变量和父类变量相互独立。如果想忽略父类变量,可以在其上加@ExcelIgnore注解

Do you mean to update parent annotation? Hmmm...

In my opinion , we should not modify the parent behavior for child requirement. e.g. A parent class with two child classes

    class Fruit {        
        @ExcelProperty("parent1Field")
        private String field;
    }

    class Apple extends Fruit {
       @ExcelIgnore
        private String field;
    }
    class Orange extends Fruit {
         private String field;
    }

If ignore Apple field, we should not impact Orange. So it's fine to just update the child but not the parent.

class Fruit {        
    @ExcelProperty("parent1Field")
    private String field;
}

class Apple extends Fruit {
   @ExcelIgnore
    private Integer field;
}
class Orange extends Fruit {
     private Boolean field;
}

Since hidden fields are independent of each other, how should we handle scenarios where only the parent class field or the subclass field is retained after export, rather than ignoring both?

@alaahong
Copy link
Member Author

as the independence, so you could find out all my given cases in commit are verified that no impact while call parent and child class separately.
e.g. the case parent4&6 and child4&6. ignore parent won't impact child export.

@alaahong
Copy link
Member Author

Unlike method overriding, the "field" that the subclass redefines is its own and has no direct connection to the parent class's "field". If you want the parent class's "field" to be ignored, simply add the @ExcelIgnore annotation to the parent class's "field". 和方法重写不同,我认为子类变量和父类变量相互独立。如果想忽略父类变量,可以在其上加@ExcelIgnore注解

Do you mean to update parent annotation? Hmmm...
In my opinion , we should not modify the parent behavior for child requirement. e.g. A parent class with two child classes

    class Fruit {        
        @ExcelProperty("parent1Field")
        private String field;
    }

    class Apple extends Fruit {
       @ExcelIgnore
        private String field;
    }
    class Orange extends Fruit {
         private String field;
    }

If ignore Apple field, we should not impact Orange. So it's fine to just update the child but not the parent.

class Fruit {        
    @ExcelProperty("parent1Field")
    private String field;
}

class Apple extends Fruit {
   @ExcelIgnore
    private Integer field;
}
class Orange extends Fruit {
     private Boolean field;
}

Since hidden fields are independent of each other, how should we handle scenarios where only the parent class field or the subclass field is retained after export, rather than ignoring both?

Might be I missed your point?
As this PR is going to fix the behavior of @ExcelIgnore in Child class, you can check my test case in commit, then list your expected behavior~

@imsai-sh
Copy link

To minimize the potential impact on users of older versions, would it be possible to consider introducing a new annotation or adding a parameter to control whether parent class fields should be overridden, instead of modifying the behavior of @ExcelIgnore directly? Since @ExcelIgnore does not raise any errors, there’s a chance that some existing users may already rely on its current behavior.

@alaahong
Copy link
Member Author

To minimize the potential impact on users of older versions, would it be possible to consider introducing a new annotation or adding a parameter to control whether parent class fields should be overridden, instead of modifying the behavior of @ExcelIgnore directly? Since @ExcelIgnore does not raise any errors, there’s a chance that some existing users may already rely on its current behavior.

it depends on this is a bug or enhancement. as I found, there are several issues related this in easyexcel, even some pr merged for this, not sure why still unexpected case now.
in my opinion, i prefer to fix this logic within existing annotation, one hand is it always treating as a bug in legacy issues,another hand is in current FastExcel/EasyExcel as case in my commit , the default logic is following what declare in final/calling class, which means annotate child> exends parent

@mymde
Copy link

mymde commented Jul 25, 2025

Maybe the writing of variables with the same name in parent and child is not standardized, and it involves the concept of hidden fields, which is easy to confuse with overriding. Is it more reasonable to make it an enhancement?

@alaahong
Copy link
Member Author

alaahong commented Jul 25, 2025

Maybe the writing of variables with the same name in parent and child is not standardized, and it involves the concept of hidden fields, which is easy to confuse with overriding. Is it more reasonable to make it an enhancement?

this fix is only covering the field both define at parent and child, it won't correct what user defined, just make sure the logic/behavior same within FastExcel.
we can move to other issues for the right usage or practice~

Copy link
Member

@psxjoy psxjoy left a comment

Choose a reason for hiding this comment

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

Sorry for not checking this PR in time. I don't see any problems with it. If no one has any objections or other issues, I will merge the PR within 24 hours.

@psxjoy psxjoy merged commit 8d68a92 into apache:main Jul 29, 2025
5 checks passed
@delei delei added this to the 1.3.0 milestone Jul 30, 2025
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.

5 participants