-
Notifications
You must be signed in to change notification settings - Fork 469
fix: FastExcel won't ignore the field while both annotated @ExcelIgnore in Child and @ExcelProperty in Parent #427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: FastExcel won't ignore the field while both annotated @ExcelIgnore in Child and @ExcelProperty in Parent #427
Conversation
1. Add fieldNameToField to filter final ignore field 2. Add necessary case for verification
There was a problem hiding this 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
@ExcelIgnoreannotated fields before processing - Comprehensive test coverage for all inheritance scenarios with
@ExcelPropertyand@ExcelIgnorecombinations
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 |
fastexcel-core/src/main/java/cn/idev/excel/util/ClassUtils.java
Outdated
Show resolved
Hide resolved
fastexcel-core/src/main/java/cn/idev/excel/util/ClassUtils.java
Outdated
Show resolved
Hide resolved
fastexcel-test/src/test/java/cn/idev/excel/test/core/skip/ClassUtilsFieldOverrideTest.java
Outdated
Show resolved
Hide resolved
|
I'm just using GitHub Copilot for some common code-review suggestions. If no changes are necessary, I'll do a code-review ASAP :) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Seems copilot only find out typo~ |
|
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". |
Do you mean to update parent annotation? Hmmm... In my opinion , we should not modify the parent behavior for child requirement. 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
|
This PR conflicts due to some previous changes. Would you be willing to fix these conflicts and submit it again? |
psxjoy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR conflict
…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
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? |
|
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. |
Might be I missed your point? |
|
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. |
|
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. |
psxjoy
left a comment
There was a problem hiding this 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.
Purpose of the pull request
Related: #425
What's changed?
fastexcel-core/src/main/java/cn/idev/excel/util/ClassUtils.java
Checklist