RFR: 8347826: Introspector shows wrong method list after 8071693 [v8]

[Alexey Ivanov]

On Fri, 28 Feb 2025 10:09:30 GMT, Roman Marchenko <rmarchenko at openjdk.org> wrote:

>> Fixed `com.sun.beans.introspect.MethodInfo#MethodOrder` to make `Introspector.addMethod()` working properly when filtering methods out.
>> 
>> Also, after PR discussion, added the approptiate test cases with corresponding fixes in MethodInfo.java and PropertyInfo.java.
>> 
>> ---------
>> `getMethodDescriptors()` returns descriptors of public methods of a class and its parent classes, including default methods defined in interfaces. The result doesn't include bridge methods, or methods which were overriden in subclasses.
>> 
>> `getPropertyDescriptors()` returns descriptors of methods which were identified as getters or setters. As there can be the only method getter/setter per property, the following rules are applied when choosing a getter/setter:
>> 
>> * Getters/setters for the same property defined (not necessarily overriden) in subclasses have higher precedence.
>> * If there are getters/setters for the same property defined in the same class and argument types are assignable one to another, the method with the Least Common Supertype has the lower priority. If argument types are not assignable, the result is undefined (any method will be chosen).
>
> Roman Marchenko has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixing review comments 2

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/com/sun/beans/introspect/PropertyInfo.java line 81:

> 79:         if (!isInitedToIsGetter && this.readList != null) {
> 80:             for (MethodInfo info : this.readList) {
> 81:                 if ((this.read == null) || (!info.method.isDefault() && this.read.type.isAssignableFrom(info.type))) {

Suggestion:

                if ((this.read == null) || (!info.method.isDefault()
                                            && this.read.type.isAssignableFrom(info.type))) {

To avoid a rather long line; wrapping at `||` is also an option.

test/jdk/java/beans/Introspector/DefaultMethodBeanPropertyTest.java line 235:

> 233:         default public void setFoo(String num) {
> 234:         }
> 235:     }

Suggestion:

    public interface A5 {
        public default void setParentFoo(Integer num) {
        }
        public default void setFoo(String num) {
        }
    }

The `public` keyword should precede the `default` keyword in the list of method modifiers.

This applies to `A8` and `B8` too.

test/jdk/java/beans/Introspector/DefaultMethodBeanPropertyTest.java line 319:

> 317:     public static void testScenario7() {
> 318:         verifyMethods(D7.class,
> 319:             "public void DefaultMethodBeanPropertyTest$D7.setValue(java.lang.Runnable)"

`getValue` is not included because there's no implementation for it, right?

test/jdk/java/beans/Introspector/DefaultMethodBeanPropertyTest.java line 409:

> 407:                     + expected.stream()
> 408:                             .map(Object::toString)
> 409:                             .collect(Collectors.joining("\n  ")));

Suggestion:

                    + expected.stream()
                              .map(Object::toString)
                              .collect(Collectors.joining("\n  ")));

The dots in chained calls were aligned; they remain aligned for `actual` but aren't aligned for `expected`.

test/jdk/java/beans/Introspector/DefaultMethodBeanPropertyTest.java line 417:

> 415:             final Set<String> expected = new HashSet<>(Arrays.asList(methodNames));
> 416:             final Set<String> actual = Arrays
> 417:                     .stream(Introspector.getBeanInfo(type).getPropertyDescriptors())

Suggestion:

                    .stream(Introspector.getBeanInfo(type)
                                        .getPropertyDescriptors())

It may be better this way, the call to `getPropertyDescriptors` is clearly visible.

test/jdk/java/beans/Introspector/DefaultMethodBeanPropertyTest.java line 418:

> 416:             final Set<String> actual = Arrays
> 417:                     .stream(Introspector.getBeanInfo(type).getPropertyDescriptors())
> 418:                     .flatMap(pd -> Arrays.stream(new Method[]{pd.getReadMethod(), pd.getWriteMethod()}))

Suggestion:

                    .flatMap(pd -> Stream.of(pd.getReadMethod(), pd.getWriteMethod()))

test/jdk/java/beans/Introspector/DefaultMethodBeanPropertyTest.java line 419:

> 417:                     .stream(Introspector.getBeanInfo(type).getPropertyDescriptors())
> 418:                     .flatMap(pd -> Arrays.stream(new Method[]{pd.getReadMethod(), pd.getWriteMethod()}))
> 419:                     .filter(method -> method != null)

Suggestion:

                    .filter(Objects::nonNull)

test/jdk/java/beans/Introspector/DefaultMethodBeanPropertyTest.java line 432:

> 430:             final Set<String> expected = new HashSet<>(Arrays.asList(methodNames));
> 431:             final Set<String> actual = Arrays
> 432:                     .stream(Introspector.getBeanInfo(type, Object.class).getMethodDescriptors())

Suggestion:

                    .stream(Introspector.getBeanInfo(type, Object.class)
                                        .getMethodDescriptors())

-------------

PR Review: https://git.openjdk.org/jdk/pull/23443#pullrequestreview-2704062570
PR Review Comment: https://git.openjdk.org/jdk/pull/23443#discussion_r2006457546
PR Review Comment: https://git.openjdk.org/jdk/pull/23443#discussion_r2006426422
PR Review Comment: https://git.openjdk.org/jdk/pull/23443#discussion_r2006437794
PR Review Comment: https://git.openjdk.org/jdk/pull/23443#discussion_r2006445627
PR Review Comment: https://git.openjdk.org/jdk/pull/23443#discussion_r2006454544
PR Review Comment: https://git.openjdk.org/jdk/pull/23443#discussion_r2006450570
PR Review Comment: https://git.openjdk.org/jdk/pull/23443#discussion_r2006451061
PR Review Comment: https://git.openjdk.org/jdk/pull/23443#discussion_r2006455315