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

[Semyon Sadetsky]

On 5/27/2016 1:57 AM, Sergey Bylokhov wrote:
> On 26.05.16 19:48, Sergey Bylokhov wrote:
>> That's the separate bug, this one is about "Unstable behavior of
>> PropertyDescriptor's getWriteMethod() in case of overloaded setters" and
>> the root cause here is that the same code which is executed on a
>> different systems behave differently. After the fix in all cases the
>> same code will work unconditionally from the system where it is
>> executed. Do you have an objections that the change is not fix this
>> instability?
>
> 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.
>
>>
>>>>> 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
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>
>