8347826: Introspector shows wrong method list after 8071693 #23443

wkia · 2025-02-04T16:14:44Z

Fixed com.sun.beans.introspect.MethodInfo and com.sun.beans.introspect.PropertyInfo 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 and static methods defined in interfaces. The result doesn't include methods which were declared and not implemented, bridge methods, or methods which were overriden in subclasses.

When a subclass "overrides" a static method from a parent class getMethodDescriptors() behaves as follows:

In case of a clash with a class method, the class version will take precedence rather than interface's one.
If the same method appears in multiple classes/interfaces within the same hierarchy, the version from the most specific sub-interface will be selected.
If the method appears in classes/interfaces from different hierarchies, the result is undefined, either version can be returned.

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).
Gettters/setters declared and not implemented are not considered.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8347826: Introspector shows wrong method list after 8071693 (Bug - P3)

Reviewers

Alexander Zvegintsev (@azvegint - Reviewer)
Alexey Ivanov (@aivanov-jdk - Reviewer) Review applies to 4f29444a
Sergey Bylokhov (@mrserb - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23443/head:pull/23443
$ git checkout pull/23443

Update a local copy of the PR:
$ git checkout pull/23443
$ git pull https://git.openjdk.org/jdk.git pull/23443/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23443

View PR using the GUI difftool:
$ git pr show -t 23443

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23443.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-02-04T16:15:14Z

👋 Welcome back rmarchenko! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-02-04T16:15:32Z

@wkia This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8347826: Introspector shows wrong method list after 8071693

Reviewed-by: azvegint, serb, aivanov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 121 new commits pushed to the master branch:

832c5b0: 8350880: (zipfs) Add support for read-only zip file systems
24edd3b: 8354930: IGV: dump C2 graph before and after live range stretching
c96803d: 8358035: Remove unused compute_fingerprint declaration in ClassFileStream
... and 118 more: https://git.openjdk.org/jdk/compare/72b9aafd5a0dfb379a979f554fa99a767eef3e50...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@azvegint, @aivanov-jdk, @mrserb) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-02-04T16:16:02Z

@wkia The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-02-04T16:18:46Z

Webrevs

azvegint

Looks good.

test/jdk/java/beans/Introspector/DefaultMethodBeanPropertyTest.java

anuj-ion · 2025-02-06T13:19:26Z

Hi, As this PR is approved, hopefully it will be merged to master soon. But ticket JDK-8347826 doesn't mention Java 17 backporting. Can you please also add request for backporting to Java 17 also. Many Thanks,

….java Co-authored-by: Aleksandr Zvegintsev <77687766+azvegint@users.noreply.github.com>

wkia · 2025-02-06T14:37:18Z

@anuj-ion

Hi, As this PR is approved, hopefully it will be merged to master soon. But ticket JDK-8347826 doesn't mention Java 17 backporting. Can you please also add request for backporting to Java 17 also. Many Thanks,

Sure, however usually backports are requested in backward order (23, 21, 17, ...) after the change is integrated to the upstream. So, I'm going to backport this after the integration is completed.

wkia · 2025-02-06T14:38:17Z

/integrate

openjdk · 2025-02-06T14:39:02Z

@wkia
Your change (at version 5782dd7) is now ready to be sponsored by a Committer.

prrace · 2025-02-06T17:29:48Z

/reviewers 2 reviewer

openjdk · 2025-02-06T17:30:35Z

@prrace
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

prrace · 2025-02-06T18:08:58Z

https://openjdk.org/groups/client-libs/
See "Code Reviews" where it says
"The standard requirement in the JDK project is for one reviewer to approve the fix.
The Java Client Library Group has always standardized on two approvals."
...
"The fixer therefore needs to understand the policies and wait for a second approval."

mrserb · 2025-02-08T04:21:20Z

In jdk 9 we started to sort the list of methods for each class for a few reasons, and the main one was:

We had a number of bugs which state that our JavaBeans randomly does not work, examples: JDK-6807471[1] , JDK-6788525[2], the reason was that the order of methods from Class.getMethods() is not specified.

So MethodOrder is just a workaround to help us reproduce the bugs. I understand how the current change will fix the problem, but it would be nice to update the code that retrieves the method from that list, or at least provide some information why we can't do that.

Example from the past #7190 where MethodOrder was simplified due to JDK-8196373

wkia · 2025-02-10T10:17:28Z

it would be nice to update the code that retrieves the method from that list, or at least provide some information why we can't do that.
Example from the past #7190 where MethodOrder was simplified due to JDK-8196373 do that.

@mrserb Sorry, I don't feel I completely understand what are you asking for. What do you mean "the code that retrieves the method from that list"? Should I add comments somewhere in source code with the issue details why does Introspector.addMethod() work wrong for now, or with details about sorting? Or should I update the PR description? I couldn't find any clues in #7190, sorry.

And BTW, I'm concerning why I can't find a check in this PR which builds and tests the change. Should I trigger it somehow, or is it just invisible for me?

mrserb · 2025-02-11T03:25:28Z

@mrserb Sorry, I don't feel I completely understand what are you asking for. What do you mean "the code that retrieves the method from that list"? Should I add comments somewhere in source code with the issue details why does Introspector.addMethod() work wrong for now, or with details about sorting? Or should I update the PR description? I couldn't find any clues in #7190, sorry.

In the past we received various reports that were caused by bugs in the Introspector implementation, but we could not reproduce them because that bugs depended on some order of methods returned by the Class.getMethods().

So, the method you updated was added to make the method list stable, it is just a utility wrapper. You don't need to update that method.

But instead you can change the code that uses the result of MethodInfo.get() in PropertyInfo.get() or PropertyInfo.initialize() where we created the property to pick the correct default/non-default method.

Or, most likely, you can skip adding the default method in MethodInfo.get() if non-default methods are already added.

And BTW, I'm concerning why I can't find a check in this PR which builds and tests the change. Should I trigger it somehow, or is it just invisible for me?

You should enable that actions/checks in your fork.

mrserb · 2025-02-11T03:36:00Z

For example, if you change the return type of After8071693.B.getDefault1() to String + mark parent as non-default + change "A" from interface to class, the correct method will be chosen, because the code for covariance types will discard the "Object version" and choose the "String version" of the method.

Similar logic should be added to discard default methods if the same non-default method exists.

mrserb · 2025-02-11T04:56:09Z

Also please check the next example:

import java.beans.IntrospectionException;

public class Test {

    public interface A {
        default void setFoo(Integer num) {
        }
    }

    public class D implements A {
        public void setFoo(Number num) {
        }
//        public Integer getFoo() {
//            return null;
//        }
    }

    public class AC {
        public void setFoo(Integer num) {
        }
    }

    public class DC extends AC {
        public void setFoo(Number num) {
        }
//        public Integer getFoo() {
//            return null;
//        }
    }

    public static void main(String[] args) throws java.beans.IntrospectionException {
        test(D.class);
        test(DC.class);
    }

    private static void test(Class beanClass) throws IntrospectionException {
        System.out.println("\nbeanClass = " + beanClass);
        var info = java.beans.Introspector.getBeanInfo(beanClass, Object.class);
        System.out.println(info.getBeanDescriptor());
        System.out.println("--- properties");
        for (var desc : info.getPropertyDescriptors()) {
            System.out.println(desc.getReadMethod());
            System.out.println(desc.getWriteMethod());
        }
    }
}

Is the next output expected()?

beanClass = class Test$D
java.beans.BeanDescriptor[name=Test$D; beanClass=class Test$D]
--- properties
public java.lang.Integer Test$D.getFoo()
public default void Test$A.setFoo(java.lang.Integer)

beanClass = class Test$DC
java.beans.BeanDescriptor[name=Test$DC; beanClass=class Test$DC]
--- properties
public java.lang.Integer Test$DC.getFoo()
public void Test$AC.setFoo(java.lang.Integer)

btw it will be good to cover this scenario in the test.

aivanov-jdk · 2025-04-14T13:28:09Z

I do not have any other comments, @aivanov-jdk please take a look.

@aivanov-jdk …
Could you review the PR, please?

I'm currently busy, I'll get to look at this PR as soon as I can.

Does it make sense to expand tests to cover cases that @mrserb brought up? With module system and exported vs non-exported classes and interfaces?

src/java.desktop/share/classes/java/beans/Introspector.java

wkia · 2025-05-23T11:45:35Z

Could you review, please?
@aivanov-jdk @mrserb @azvegint

mrserb · 2025-05-27T18:28:54Z

Could you review, please? @aivanov-jdk @mrserb @azvegint

Please update the test to cover the latest change.

mrserb · 2025-05-28T02:30:13Z

It would be nice to merge the master, the branch is quite outdated

wkia · 2025-05-30T11:12:07Z

Could you review, please? @aivanov-jdk @mrserb @azvegint

Please update the test to cover the latest change.

Done

wkia · 2025-06-03T05:40:29Z

/integrate

openjdk · 2025-06-03T05:41:03Z

@wkia
Your change (at version 8d46a88) is now ready to be sponsored by a Committer.

mrserb · 2025-06-03T05:59:27Z

/sponsor

openjdk · 2025-06-03T06:00:28Z

Going to push as commit c5f235c.
Since your change was applied there have been 121 commits pushed to the master branch:

832c5b0: 8350880: (zipfs) Add support for read-only zip file systems
24edd3b: 8354930: IGV: dump C2 graph before and after live range stretching
c96803d: 8358035: Remove unused compute_fingerprint declaration in ClassFileStream
... and 118 more: https://git.openjdk.org/jdk/compare/72b9aafd5a0dfb379a979f554fa99a767eef3e50...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-06-03T06:00:36Z

@mrserb @wkia Pushed as commit c5f235c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

fix

605ec5a

openjdk bot added the rfr Pull request is ready for review label Feb 4, 2025

openjdk bot added the client client-libs-dev@openjdk.org label Feb 4, 2025

Roman Marchenko added 2 commits February 4, 2025 18:22

test refactoring

3cea2bb

minor test fix

a913b86

azvegint approved these changes Feb 6, 2025

View reviewed changes

test/jdk/java/beans/Introspector/DefaultMethodBeanPropertyTest.java Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label Feb 6, 2025

Update test/jdk/java/beans/Introspector/DefaultMethodBeanPropertyTest…

5782dd7

….java Co-authored-by: Aleksandr Zvegintsev <77687766+azvegint@users.noreply.github.com>

openjdk bot removed the ready Pull request is ready to be integrated label Feb 6, 2025

azvegint approved these changes Feb 6, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 6, 2025

openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 6, 2025

openjdk bot removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated labels Feb 6, 2025

azvegint reviewed Apr 23, 2025

View reviewed changes

src/java.desktop/share/classes/java/beans/Introspector.java Show resolved Hide resolved

mrserb mentioned this pull request May 2, 2025

Annotations are missing in Beaninfo if a method has default implementation in the interface [17.0.14] corretto/corretto-17#228

Closed

Restored the original order processing

431705a

wkia requested review from azvegint and mrserb May 23, 2025 11:45

azvegint approved these changes May 27, 2025

View reviewed changes

wkia and others added 2 commits May 28, 2025 08:42

Merge branch 'openjdk:master' into jdk-8347826

779194f

Addes test scenario 10

82bad7d

minor change

8d46a88

mrserb approved these changes May 31, 2025

View reviewed changes

wkia requested a review from azvegint June 2, 2025 06:38

azvegint approved these changes Jun 3, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 3, 2025

openjdk bot added the sponsor Pull request is ready to be sponsored label Jun 3, 2025

openjdk bot added the integrated Pull request has been integrated label Jun 3, 2025

openjdk bot closed this Jun 3, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jun 3, 2025

wkia deleted the jdk-8347826 branch June 4, 2025 10:11

8347826: Introspector shows wrong method list after 8071693 #23443

8347826: Introspector shows wrong method list after 8071693 #23443

Uh oh!

Conversation

wkia commented Feb 4, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 4, 2025

Uh oh!

openjdk bot commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Feb 4, 2025

Uh oh!

mlbridge bot commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

azvegint left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anuj-ion commented Feb 6, 2025 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wkia commented Feb 6, 2025

Uh oh!

wkia commented Feb 6, 2025

Uh oh!

openjdk bot commented Feb 6, 2025

Uh oh!

prrace commented Feb 6, 2025

Uh oh!

openjdk bot commented Feb 6, 2025

Uh oh!

prrace commented Feb 6, 2025

Uh oh!

mrserb commented Feb 8, 2025

Uh oh!

wkia commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrserb commented Feb 11, 2025

Uh oh!

mrserb commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrserb commented Feb 11, 2025

Uh oh!

aivanov-jdk commented Apr 14, 2025

Uh oh!

Uh oh!

wkia commented May 23, 2025

Uh oh!

mrserb commented May 27, 2025

Uh oh!

mrserb commented May 28, 2025

Uh oh!

wkia commented May 30, 2025

Uh oh!

wkia commented Jun 3, 2025

Uh oh!

openjdk bot commented Jun 3, 2025

Uh oh!

mrserb commented Jun 3, 2025

Uh oh!

openjdk bot commented Jun 3, 2025

Uh oh!

openjdk bot commented Jun 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

wkia commented Feb 4, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 4, 2025 •

edited

Loading

mlbridge bot commented Feb 4, 2025 •

edited

Loading

anuj-ion commented Feb 6, 2025 •

edited by bridgekeeper bot

Loading

wkia commented Feb 10, 2025 •

edited

Loading

mrserb commented Feb 11, 2025 •

edited

Loading