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

Make Component.equals()/hashCode() final #2984

Closed
mvysny opened this issue Nov 20, 2017 · 6 comments
Closed

Make Component.equals()/hashCode() final #2984

mvysny opened this issue Nov 20, 2017 · 6 comments
Milestone

Comments

@mvysny
Copy link
Member

mvysny commented Nov 20, 2017

Please see thread https://vaadin.com/forum/#!/thread/16866153 - user overrode equals/hashCode and got a very confusing behavior in response.

It is often assumed that Vaadin components use object identity for comparison, yet this is not documented anywhere explicitly. If this is true and Vaadin components instances must be unique, then it would be great to prohibit overriding of equals()/hashCode() (by making it final and calling super); a javadoc should then explain why exactly this is prohibited.

@mvysny mvysny changed the title Make equals()/hashCode() final Make Component.equals()/hashCode() final Nov 20, 2017
@Legioth
Copy link
Member

Legioth commented Nov 20, 2017

Making those methods final would make it impossible to create dynamic proxies of such classes, which would in turn break lots of use case with e.g. Spring or CDI. I remember discussing something similar a couple of months ago, but I think the conclusion was that there isn't really anything we can do about the issue other than clarifying documentation.

@mvysny
Copy link
Member Author

mvysny commented Nov 21, 2017

@Legioth I'm not well-versed with Spring/CDI, but if one is not attempting to intercept calls to equals()/hashCode() then I believe that creating a proxy should in general succeed: the Component itself is not final after all?

EDIT: You may be right: according to https://docs.jboss.org/cdi/spec/1.2/cdi-spec.html:

classes which have non-static, final methods with public, protected or default visibility
are considered unproxyable.

Still, if those methods are not subject to interception, I would assume that dynamic proxy will simply not try to override those methods?

@Legioth
Copy link
Member

Legioth commented Nov 21, 2017

The entire class is considered unproxyable once there is a single method that cannot be proxied. All method calls must be intercepted e.g. in the case of a session scoped bean that delegates all calls to the actual instance that is bound to the current scope.

@pleku
Copy link
Contributor

pleku commented Jan 31, 2018

Not sure what documentation to improve here, closing as won't fix. Please comment if you run into this issue and can point out how we should improve the documentation.

@pleku pleku closed this as completed Jan 31, 2018
@pleku pleku added this to the Abandoned milestone Jan 31, 2018
@mvysny
Copy link
Member Author

mvysny commented Dec 4, 2020

Fine, then at least override the methods and deprecate them, without making them final. The documentation is clearly insufficient since it tells absolutely nothing about bad things waiting to happen when you start messing with Component.equals() and hashCode(). I have no idea what exactly will go wrong, that's for the Framework guys to answer, but things clearly can go wrong: https://vaadin.com/forum/thread/16866153

@circlesmiler
Copy link

I currently "felt in this hole" because SonarLint rule "Subclasses that add fields should override "equals" (java:S2160)" told me to override equals/hashcode in a Component subclass.

I did this with the Lombok annotation @EqualsAndHashCode. After I did this, my dialog didn't close anymore when the user changed data in it. The "bug" was hard to find, because there was some time between the code change and founding the misbehaviour.

(I'm still on Vaadin 8, but migrating to Vaadin 23 is in progress... not sure, if the problem still exists there)

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

No branches or pull requests

4 participants