dev@javaserverfaces.java.net

Re: Code review request for https://java.net/jira/browse/JAVASERVERFACES-3582

From: zhijun Ren <ren.zhijun_at_oracle.com>
Date: Thu, 12 Feb 2015 09:25:24 +0800

Hi Arjan,

Great thanks for your detailed explanation, got your meaning now.

But I still have question about the public API of UIInput in 2.1.20 for
your guys, it overrides the method setValue() as:

/ @Override
     public void setValue(Object value) {
         super.setValue(value);
         // Mark the local value as set.
         setLocalValueSet(true);
     }/

But there is not corresponding getValue() overridden , as is done in JSF
2.2, is this reasonable? Logically the following getValue() is more
reasonable:

/ public Object getValue() {
         return isLocalValueSet() ? getLocalValue() : super.getValue();
     }/

than the getValue() in it's parent class UIOutput:
/
     /**
      * <p class="changed_added_2_2">Return the value property.</p>
      *
      * @since 2.2
      */
     public Object getValue() {

         return getStateHelper().eval(PropertyKeys.value);

     }/

Is this design intent or a defect? if is a defect, the published doc for
UIInput should be not reasonable either, right?

BR,
Zhijun



On 2/11/15, 20:02, arjan tijms wrote:
> Hi Zhijun,
>
> If I understand correctly you'd like to add the public Object
> getValue() method to UIInput in the Mojarra 2.1 (JSF 2.1) branch?
>
>> So what's your concern? I can't take any action before you make me
>> understand what you mean.
> If my understanding of the issue is correct, then the problem is that
> you'll be making a change to the public API of UIInput.
>
> From an implementer and user perspective this is sometimes hard to
> understand, as indeed the exact same method is already present in the
> base class. For callers (users), nothing would change, so the change
> would seem "safe".
>
> Yet, from a spec perspective it IS a change to the public API (Manfred
> uses the term "signature" loosely here, to refer to the entire API,
> not just a single method). Even JavaDoc corrections constitute such a
> change, and these too will not have any effect on the caller.
>
> Again loosely speaking, the public API is basically what you see here:
> http://docs.oracle.com/javaee/6/api/javax/faces/component/UIInput.html
>
> As you can see, public Object getValue() is not in the list of methods
> beneath "Method Summary". If you would add it, that list would change,
> and http://docs.oracle.com/javaee/6/api/javax/faces/component/UIInput.html
> has to be republished (as it would not be correct otherwise). On top
> of that, the TCK would have to be updated (which can not only check
> for the existence of methods that should be there, but if I'm not
> mistaken also for the existence of methods that should NOT be there).
>
> Then, you would have to convince Leonardo from MyFaces that this
> method needs to be added, and he has to implement it in MyFaces as
> well, exactly like the JavaDoc says it should (since it's a public
> method on a spec class). If you wouldn't do that and
> http://docs.oracle.com/javaee/6/api/javax/faces/component/UIInput.html
> was updated for Mojarra, then it would only apply to Mojarra and not
> to MyFaces anymore. The spec explicitly prohibits this.
>
> There is a process though for all of this work, and that's the JCP MR
> process. I think you could *theoretically* start that, but I'm not
> sure how feasible it would be to start a MR process for JSF 2.1 at
> this moment, seeing how JSF 2.2 is the current version and JSF 2.3 is
> being developed now. Maybe it's not even possible (but you could ask
> Ed or Heather to be sure).
>
> At any length, an MR process should not be taken lightly, hence I
> guess Manfred's request to see if the bug can be fixed in any other
> way.
>
> Kind regards,
> Arjan
>
>
>
>
>
> On Wed, Feb 11, 2015 at 3:35 AM, zhijun Ren<ren.zhijun_at_oracle.com> wrote:
>> Hi Manfred,
>>
>> Let me clarify the concept things:
>>
>> Signature means the way how the caller do the invocation: method name, input
>> parameters and the expected return values;
>>
>> Override changes what the method do, means the behavior changes.
>>
>> The key point here is:
>> 1. The feature UIRepeat is also in 2.1.20 and the complained issue is also
>> there;
>> 2. I have done test locally which covered the tests set which is same as
>> that in Hudson and didn't find any of the Spec tests was broken.
>>
>> Briefly, the issue exists and the fixing works and no legacy tests broken.
>>
>> So what's your concern? I can't take any action before you make me
>> understand what you mean.
>>
>> BR,
>> Zhijun
>>
>>
>>
>> On 2/10/15, 21:56, manfred riem wrote:
>>
>> Hi Zhijun,
>>
>> An override is a signature change as well. Ed, can you please weigh in as
>> well?
>>
>> Thanks!
>> Manfred
>>
>> On 2/9/15, 7:29 PM, zhijun Ren wrote:
>>
>> Hi Manfred,
>>
>> Why do you think it is a signature change? it is overriding, the signature
>> is the same as it's parent class UIOutput:
>>
>> public Object getValue()
>>
>> and the code is now the same in 2.3.
>>
>> The issue complained by #2717 also exists in 2.1.20 which also have the
>> UIRepeat feature.
>>
>> Maybe I am not catching your real meaning, can you make it more clear?
>>
>> BR,
>> Zhijun
>>
>> On 2/9/15, 22:44, manfred riem wrote:
>>
>> Hi Zhijun,
>>
>> Unfortunately you cannot commit this as you are changing an API signature.
>>
>> As we discussed before for this issue you need to look to see if the test
>> can be made to pass without changing the API signature.
>>
>> If that is not possible then we need to make sure the test itself is
>> adjusted.
>>
>> Please verify the behavior against an older 2.1 release.
>>
>> Kind regards,
>> Manfred
>>
>>
>> On 2/9/15, 2:12 AM, zhijun Ren wrote:
>>
>> Hi Manfred and Ed,
>>
>> Please help to review the code attached on 20150209, please note that I have
>> all the Hudson test cases locally against WLS and didn't found any failed
>> test cases.
>>
>> attached the change here for your convenience.
>>
>> BR,
>> Zhijun
>>
>>