On Fri, 28 Feb 2025 10:09:30 GMT, Roman Marchenko <rmarche...@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
