<Beans Dev> [9] Review Request: 8156043 Unstable behavior of PropertyDescriptor's getWriteMethod() in case of overloaded setters

[Sergey Bylokhov]

On 27.05.16 10:51, Semyon Sadetsky wrote:
>> Do you have some other objections about the current fix? Are there
>> some issues in the added code, without linking some other known bugs,
>> or you doubts that the order still unstable?
> Yes. I don't like the rank you use to discard properties. I'd prefer to
> have it not the biggest alphabeticaly but more meaningful.

It is obviously that the added code discards nothing, it is a wrapper on 
top of the Class.getMethods(), which make the order of methods stable.

>>>>>> And why it is not discarded if I change SUB.setFoo(Integer) to
>>>>>> SUB.setFoo(String) for example? That seems very odd.
>>>>>
>>>>> I assume because in this case instead of foo=int we will select
>>>>> foo=long and will skip foo=String.
>>>>>
>>>>>>
>>>>>> And before your fix there was no such behavior. You wrote that it was
>>>>>> random before the fix, but I ran the test may times and could not get
>>>>>> the same effect.
>>>>>> So I suppose it was introduced by the fix.
>>>>>
>>>>> It means that when you run the app Class.getMethods() always return
>>>>> the same order of methods. You can add a logging to the test attached
>>>>> to the fix and run it on current jdk9, you will see that the type of
>>>>> foo property is different time to time.
>>>>>
>>>>>>
>>>>>> Also there is yet another problem here. The default interface methods
>>>>>> are totally ignored. This is probably a separate issue.
>>>>>
>>>>> This is a known bug:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8139193
>>>>>
>>>>>>
>>>>>> --Semyon
>>>>>>>
>>>>>>>>
>>>>>>>> In my opinion, the method may return any of this two (also a
>>>>>>>> secondary
>>>>>>>> rank could be used), but not a mixture of them in any
>>>>>>>> circumstances.
>>>>>>>>
>>>>>>>> --Semyon
>>>>>>>>
>>>>>>>>>>>> In my understanding, if I override the setter I should get the
>>>>>>>>>>>> over-ridden method for the top level class introspection.
>>>>>>>>>>>> Otherwise
>>>>>>>>>>>> I do
>>>>>>>>>>>> not understand for what such introspector can be used:
>>>>>>>>>>>>
>>>>>>>>>>>> I have some object which property I want to set, but using the
>>>>>>>>>>>> introspector for that I'm calling a wrong method which
>>>>>>>>>>>> breaks the
>>>>>>>>>>>> inheritance and so corrupts the object state.
>>>>>>>>>>>>
>>>>>>>>>>>> And moreover, if I reverse the situation and override the
>>>>>>>>>>>> getter in
>>>>>>>>>>>> Sub
>>>>>>>>>>>> instead of setter :
>>>>>>>>>>>>
>>>>>>>>>>>> class Super {
>>>>>>>>>>>>     public void setFoo(Long i) {}
>>>>>>>>>>>>     public Long getFoo() {return null;}
>>>>>>>>>>>> }
>>>>>>>>>>>> class Sub extends Super {
>>>>>>>>>>>>     public Long getFoo() {return null;}
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> the introspector returns the over-ridden getter method for the
>>>>>>>>>>>> property :
>>>>>>>>>>>>
>>>>>>>>>>>> type: class java.lang.Long
>>>>>>>>>>>> getter: public java.lang.Long Sub.getFoo()
>>>>>>>>>>>> setter: public void Super.setFoo(java.lang.Long)
>>>>>>>>>>>>
>>>>>>>>>>>> This looks inconsistent.
>>>>>>>>>>>>
>>>>>>>>>>>> --Semyon
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 5/25/2016 2:14 PM, Sergey Bylokhov wrote:
>>>>>>>>>>>>>>> On 25.05.16 11:38, Semyon Sadetsky wrote:
>>>>>>>>>>>>>>>>> In this case the type of foo property will be Enum, before
>>>>>>>>>>>>>>>>> and
>>>>>>>>>>>>>>>>> after
>>>>>>>>>>>>>>>>> the fix. But the write method will be found only if this
>>>>>>>>>>>>>>>>> method is
>>>>>>>>>>>>>>>>> added to Sub, in other case the write method is recognized
>>>>>>>>>>>>>>>>> only
>>>>>>>>>>>>>>>>> if we
>>>>>>>>>>>>>>>>> remove all duplicates of set(xxx). Not sure is it intended
>>>>>>>>>>>>>>>>> behavior in
>>>>>>>>>>>>>>>>> jdk9 to skip such writers or not. I will file CR for that.
>>>>>>>>>>>>>>>> That maybe an another issue.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I dig to the history and found that it was done
>>>>>>>>>>>>>>> intentionally
>>>>>>>>>>>>>>> when
>>>>>>>>>>>>>>> JavaBean jep was implemented. but I filed
>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8157828 for
>>>>>>>>>>>>>>> additional
>>>>>>>>>>>>>>> investigation.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> But the current fix need to be checked by
>>>>>>>>>>>>>>>> the scenario when there are several getters (over-ridden
>>>>>>>>>>>>>>>> with
>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>> return
>>>>>>>>>>>>>>>> type substitutability) in addition to the setters.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Tescase is updated, the case: getE + multiple setE is added:
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~serb/8156043/webrev.01/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 5/17/2016 3:20 PM, Sergey Bylokhov wrote:
>>>>>>>>>>>>>>>>>>> Hello.
>>>>>>>>>>>>>>>>>>> Please review the fix for jdk9.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> We have a number of bugs which state that our JavaBeans
>>>>>>>>>>>>>>>>>>> randomly
>>>>>>>>>>>>>>>>>>> does
>>>>>>>>>>>>>>>>>>> not work, examples: JDK-6807471[1] , JDK-6788525[2], the
>>>>>>>>>>>>>>>>>>> reason is
>>>>>>>>>>>>>>>>>>> that the order of methods from Class.getMethods() is not
>>>>>>>>>>>>>>>>>>> specified.
>>>>>>>>>>>>>>>>>>> So I propose to fix this bug totally and sort the
>>>>>>>>>>>>>>>>>>> methods in
>>>>>>>>>>>>>>>>>>> some
>>>>>>>>>>>>>>>>>>> order. Note that the resulted list is cached, and we
>>>>>>>>>>>>>>>>>>> sort the
>>>>>>>>>>>>>>>>>>> list
>>>>>>>>>>>>>>>>>>> only the once.
>>>>>>>>>>>>>>>>>>> The code partly was copied from
>>>>>>>>>>>>>>>>>>> com.sun.jmx.mbeanserver.MethodOrder
>>>>>>>>>>>>>>>>>>> [3], but the parameters check and the order for return
>>>>>>>>>>>>>>>>>>> values
>>>>>>>>>>>>>>>>>>> were
>>>>>>>>>>>>>>>>>>> changed. After this fix our bugs(if any) can be easily
>>>>>>>>>>>>>>>>>>> reproduced.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-6807471
>>>>>>>>>>>>>>>>>>> [2] https://bugs.openjdk.java.net/browse/JDK-6788525
>>>>>>>>>>>>>>>>>>> [3]
>>>>>>>>>>>>>>>>>>> http://hg.openjdk.java.net/jdk9/client/jdk/file/fb38b0925915/src/java.management/share/classes/com/sun/jmx/mbeanserver/MBeanAnalyzer.java
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8156043
>>>>>>>>>>>>>>>>>>> Webrev can be found at:
>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~serb/8156043/webrev.00
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>>
>


-- 
Best regards, Sergey.