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

[WW-5126] use == instead of .equals in ModelDrivenInterceptor #485

Closed
wants to merge 225 commits into from

Conversation

yasserzamani
Copy link
Member

due to refreshing model regardless of potential overridden Object.equals

lukaszlenart and others added 30 commits May 2, 2018 08:04
Fix logging  message (for 2.5.x)
WW-4954 Moves XWorkList out of util package
(cherry picked from commit b213d58)
WW-4948 delete temp files on close instead of on JVM exists
…retains existing behaviour by default, but provides a configuration flag users can set (to true) in order to enable usage of response (page) encoding for s:include tags. A WARN level log output is also generated for failed FastByteArrayOutputStream decoding (can be suppressed by log configuration).
lukaszlenart and others added 17 commits November 24, 2020 08:16
2.5.x backport: WW-5116 Fix wrong regex range
Applied code review comments from JCgH4164838Gh792C124B5 into account
…es-2.5.x

[WW-5119] Fix: remove contention during localized text lookup (JDK 1.7+)
…on-2.5.x

[WW-5121] Fix: remove contention during Scope.SINGLETON injection
* Moves action on top just before the tag
* Moves pushes out of try-finally block
due to refreshing model regardless of potential overridden Object.equals
@yasserzamani yasserzamani self-assigned this Apr 25, 2021
@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 47.316% when pulling 5096637 on ww_5126_2_5 into 8a26c0d on struts-2-5-x.

@aleksandr-m
Copy link
Contributor

@yasserzamani Can you explain why comparing with == is better in this context?

@yasserzamani
Copy link
Member Author

Thanks for the review @aleksandr-m !

TBH I don't have a strong reason. I've asked reporter but no answer yet.

"Moreover the interceptor refresh process is based on call to the equals method and this method may have been redefined, in a JPA context for instance. The replacement is then not systematic and may not be done." he said. No user ever has asked for .equals so I guess .equals has been selected unintentionally and the reporter has found a valid case that shows that it should be ==. That was my reasoning.

@lukaszlenart
Copy link
Member

I'm good with that change, yet not sure if this supposed to be introduced into 2.5.x branch - some users can depend on this behaviour (even false one) and minor updates (as 2.5.27 will be) shouldn't introduce backward incompatibility.

@yasserzamani
Copy link
Member Author

I personally feel it's pretty unlikely that this change breaks someone whose intentionally is depend on .equals! but at bottom you know better 👍 please let me know

@lukaszlenart
Copy link
Member

The case is that this is unknown till someone will report a problem after upgrading to 2.5.27, I rather would like to be careful and make such potentially breaking changes in 2.6.x. The other thing is the reporter was addressing inconsistency in documentation and he was able to overcome the issue with custom interceptor - which is perfectly fine solution.

@yasserzamani yasserzamani changed the base branch from struts-2-5-x to master May 2, 2021 15:56
@yasserzamani yasserzamani deleted the ww_5126_2_5 branch May 2, 2021 15:57
@yasserzamani
Copy link
Member Author

I agree @lukaszlenart 👍 I closed it and opened its master counterpart at #486

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