vaadin / flow

Vaadin Flow is a Java framework binding Vaadin web components to Java. This is part of Vaadin 10+.
Apache License 2.0
616 stars 167 forks source link

BeanPropertySet doesn't find bean properties from interface default method getters & setters #18524

Open archiecobbs opened 8 months ago

archiecobbs commented 8 months ago

Description of the bug

BeanPropertySet is supposed to identify Java bean properties, which are defined by the existence of getter and setter methods.

However, BeanPropertySet fails to find getter and setter methods that are inherited as default methods from implemented interfaces.

If the methods are defined in an abstract superclass instead of an interface, everything works as it should... but there should be no difference in behavior in these two scenarios, as they are equally valid.

Expected behavior

BeanPropertySet should find inherited getter and setter methods no matter how they are inherited.

Minimal reproducible example

This test program demonstrates the problem:

import com.vaadin.flow.data.binder.*;
import java.util.regex.*;

public class BeanPropertySetTest {

    public interface HasName {

        default String getName() {
            return this.getLastName() + ", " + this.getFirstName();
        }
        default void setName(String name) {
            final Matcher matcher = Pattern.compile("^(.+), (.+)$").matcher(name);
            this.setLastName(matcher.group(1));
            this.setFirstName(matcher.group(2));
        }

        String getLastName();
        void setLastName(String lastName);

        String getFirstName();
        void setFirstName(String firstName);
    }   

    public class MyClass implements HasName {

        private String lastName;
        private String firstName;

        @Override
        public String getLastName() {
            return this.lastName;
        }
        @Override
        public void setLastName(String lastName) {
            this.lastName = lastName;
        }

        @Override
        public String getFirstName() {
            return this.firstName;
        }
        @Override
        public void setFirstName(String firstName) {
            this.firstName = firstName;
        }
    }

    public static void main(String[] args) {
        final PropertySet<MyClass> propertySet = BeanPropertySet.get(MyClass.class);
        for (String propertyName : new String[] { "lastName", "firstName", "name" }) {
            System.out.println(String.format("\"%s\" -> %s",
              propertyName, propertySet.getProperty(propertyName)));
        }
    }
}

There is a "name" property defined in the MyClass class, but BeanPropertySet doesn't find it.

As a result the test program above outputs this:

"lastName" -> Optional[com.vaadin.flow.data.binder.BeanPropertySet$BeanPropertyDefinition@312b1dae]
"firstName" -> Optional[com.vaadin.flow.data.binder.BeanPropertySet$BeanPropertyDefinition@7530d0a]
"name" -> Optional.empty

Versions

archiecobbs commented 8 months ago

This is caused by a JDK bug JDK-8071693 in the Introspector class.

However this bug is not fixed until JDK 21. It would be nice for Vaadin to include a workaround for JDK < 21 if possible.

mstahv commented 8 months ago

But JDK 21 is already here 😎

Seriously, I think this is just one more reason to drop usage of the Introspector class altogether, like Jackson did it a decade ago.

Pros:

Cons:

mstahv commented 8 months ago

Random idea to consider: maybe the code in Jackson could be used directly? We already have that dependency and it is (and probably will be) actively maintained library.

archiecobbs commented 8 months ago

Using Jackson makes sense to me. Bean property detection is a core function that Vaadin relies on so it would be good to ensure that it's being "done right".

mstahv commented 8 months ago

Slightly related issue from V7/8 version: https://github.com/vaadin/framework/issues/8980

mstahv commented 8 months ago

@archiecobbs Do you have the problem with Grid or with some other component? If it is Grid, try the latest cut of Viritin's VGrid (in.virit:viritin:2.6.3, soon in Maven central), that now supports default methods. Prototyped using Jackson's underlaying bean introspection instead of what Vaadin core uses (the legacy Introspector class from java.desktop module). Also the bean properties are in more reasonable order and support for records 😎:

https://github.com/viritin/flow-viritin/blob/v24/src/test/java/org/vaadin/firitin/AdvancedBeanIntrospectionWithGrid.java

archiecobbs commented 8 months ago

Hi Matti,

Do you have the problem with Grid or with some other component?

It's with Grid (indirectly, see below). I've not used VGrid because in general we prefer to stick with standard Vaadin components. Of course, that preference is based on an assumption that standard Vaadin components are basically working like they should... ;)

By "indirectly" I mean @GridColumn, which is part of my own collection of Vaadin enhancements. FWIW let me know if Vaadin is interested in any of this stuff, I've found these very helpful over the years and I'd be happy to contribute...

mstahv commented 8 months ago

Well, Viritin components generally work as I think Vaadin components should work 🤣 Sometimes I'm able to promote some solutions to core though. I'd guess this (not using JDK Introspector) would be one (but with less hackish code).

Gotta check throught your enhancments some day, looks like your Viritin 😎 If there is something that you think would be ready for Vaadin core, don't hesitate to propose it (enhancment issue + PR).

archiecobbs commented 8 months ago

FYI besides Jackson Spring's BeanUtils is another good option for introspection. In the Javadoc it also references a few other options, e.g., Apache Commons BeanUtils.

mstahv commented 8 months ago

IIRC, both those store properties like Introspector as well (and would be new deps for core Vaadin).