Skip to content
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

[BEANUTILS-541] FluentPropertyBeanIntrospector caches corrupted writeMethod (2.x) #68

Merged
merged 1 commit into from
Apr 14, 2024

Conversation

seregamorph
Copy link
Contributor

https://issues.apache.org/jira/browse/BEANUTILS-541

There is an issue in FluentPropertyBeanIntrospector (at line 144 for 1.9.3), it caches wrong writeMethod in the static cache of java.beans.Introspector when base class has at least two subclasses. Simple snippet to reproduce can be found in covering Jira541TestCase (revert FluentPropertyBeanIntrospector.java changes locally and you'll see the failure).

It is critical, because wrong information is stored globally unless explicit Introspector.flushCaches (or Introspector.flushFromCaches) is called.

@seregamorph seregamorph changed the title BEANUTILS-541 - FluentPropertyBeanIntrospector caches corrupted write… BEANUTILS-541 - FluentPropertyBeanIntrospector caches corrupted writeMethod Jan 10, 2021
* @return the descriptor
* @throws IntrospectionException if an error occurs
*/
private PropertyDescriptor createFluentPropertyDescritor(final Method m,
final String propertyName) throws IntrospectionException {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

propertyName argument is redundant in private method

@seregamorph
Copy link
Contributor Author

Backported for 1.x in #69

@seregamorph seregamorph changed the title BEANUTILS-541 - FluentPropertyBeanIntrospector caches corrupted writeMethod BEANUTILS-541 - FluentPropertyBeanIntrospector caches corrupted writeMethod (2.x) Jan 11, 2021
@garydgregory
Copy link
Member

Hi @seregamorph
Thank you for the ping, please rebase on git master, and run mvn by itself to run all checks before you push.

@seregamorph
Copy link
Contributor Author

@garydgregory thanks for your reply. The branch is rebased.
Also I'd like you to recommend to read this article https://medium.com/p/33ce3433126e with the deep research on the topic of fluent setters

@seregamorph
Copy link
Contributor Author

seregamorph commented Apr 14, 2024

Please take this issue and PR into consideration before releasing 2.0.0 🙏

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 80.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 65.13%. Comparing base (99578cc) to head (54381a1).
Report is 42 commits behind head on master.

Files Patch % Lines
...ons/beanutils2/FluentPropertyBeanIntrospector.java 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master      #68      +/-   ##
============================================
+ Coverage     65.06%   65.13%   +0.07%     
- Complexity     1484     1517      +33     
============================================
  Files           105      111       +6     
  Lines          5504     5650     +146     
  Branches       1068     1086      +18     
============================================
+ Hits           3581     3680      +99     
- Misses         1464     1490      +26     
- Partials        459      480      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@garydgregory garydgregory merged commit 6d6c521 into apache:master Apr 14, 2024
8 checks passed
@garydgregory garydgregory changed the title BEANUTILS-541 - FluentPropertyBeanIntrospector caches corrupted writeMethod (2.x) [BEANUTILS-541] FluentPropertyBeanIntrospector caches corrupted writeMethod (2.x) Apr 14, 2024
asfgit pushed a commit that referenced this pull request Apr 14, 2024
@seregamorph seregamorph deleted the bugfix/BEANUTILS-541 branch April 14, 2024 12:48
@seregamorph
Copy link
Contributor Author

Thank you for accepting @garydgregory

@garydgregory
Copy link
Member

@seregamorph YW

@seregamorph
Copy link
Contributor Author

seregamorph commented Apr 14, 2024

@garydgregory I just realized this solution makes the situation much better, but in an environment with high concurrency the issue can be still reproducible (pretty hard to reproduce).
Please check this PR with detailed description #234
Let me know if you need more details here.

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.

4 participants