Mailing-List: contact users-help@myfaces.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "MyFaces Discussion" <users@myfaces.apache.org>
Received-SPF: pass (herse.apache.org: local policy)
Message-ID: <45E610F3.5040000@voller-ernst.de>
Date: Thu, 01 Mar 2007 00:32:03 +0100
From: Paul Iov <paul@voller-ernst.de>
User-Agent: Thunderbird 1.5.0.9 (Windows/20061207)
MIME-Version: 1.0
To: MyFaces Discussion <users@myfaces.apache.org>
Subject: Re: MYFACES-1545 / MYFACES-1532
References: <8f985b960702271204m47c800b6j3998c1546005e803@mail.gmail.com>
	 <45E4BD97.8010900@voller-ernst.de>
 <8f985b960702271528r76e85534h902e8ac6cddf455f@mail.gmail.com>
In-Reply-To: <8f985b960702271528r76e85534h902e8ac6cddf455f@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Well, I've reviewed the sources related to this issue again before 
posting some comments/code to JIRA and can state now, that it's really 
obscure. At least to me :( I think it's better to figure out my doubts 
about this converter stuff again. Perhaps someone can just point out my 
mistake or else confirm the global problem with converter handling in 
current code.

Few things, that are clear:
1. Description of value attribute says:
"The initial value of this component. This value is generally set as a 
value-binding in the form #{myBean.myProperty}, where myProperty can be 
any data-type of Java (also user-defined data-types), if a converter for 
this data-type exists. Special cases...[irrelevant]"
This is true for many components, but I'm referencing here and below the 
h:selectBooleanCheckbox since it was the start point for me.

2. In most cases it not works but produce *Expected submitted value of 
type* ... *for Component* ...if value attribute is bound to a property 
of any type excepts expected one (boolean in case of 
h:selectBooleanCheckbox). This happens during initial rendering of 
component, so there is no submitted value. The only value existing at 
this time is the value of bound bean property.

3. Neither provided custom converter nor standard one eventually 
registered for expected type is called to try to convert this bean value 
into expected type.

After hours studying of source code I have grave doubts about 
correctness of how the rendering of components is handled generally 
because the problem lays IMHO in two very common utility classes:

org.apache.myfaces.shared_impl.renderkit.RendererUtils
The static methods of this class are mostly used to accomplish the 
'standard' tasks such as retrieving of component's value, so if I'm 
right, this problem affects wide range of html components and 
automatically theirs descendants. At least the components dealing with 
Boolean and Date as value should be affected. I'm not sure if this 
utility method's hierarchy (since both of getBooleanValue() and 
getDateValue() relays on getObjectValue() ) is the right place to 
implement handling of custom converters, but I have a questions 
regarding this hierarchy too since it seems to be a part of problem.
Why should the getObjectValue() call the .getSubmittedValue() and not 
the .getValue() method of EditableValueHolder? This causes that all 
affected components becomes null/false/empty as value during initial 
rendering regardless of how the underlying bean's properties are 
initialized since there is no SUBMITTED values at this time and the 
method always returns null.

org.apache.myfaces.shared_impl.taglib.UIComponentTagUtils
Probably THE right place to implement converters... but why should the 
renderkit use own utility class above to accomplish all of GETs and the 
SETs are grouped here? IMHO it should be the common task, since about 
any component can provide custom converter and GETs as well SETs should 
use it in some consistent way.

I've tried first to implement usage of custom/standard converter only 
for getBoolean() but then I have realized that it should be done 
consistent for all standard types. That's why I'm not providing any path 
right now (and the code in past posting is just an idea of how it can be 
handled). The key questions that I'd like to ask first are:
Where it should be done? (RendererUtils or UIComponentTagUtils)
How exactly should it work?

The common definition from spec(?) is beautiful, but how to implement it 
really consistent? Let's say, X is the expected by component type and Y 
is the type effectively provided by underlying value binding property. 
If X and Y are not the same type, there are many different conversion cases.

a. component retrieves the value from value-binding ((Y)object or null -> X)

b. component sets submitted value to value binding (X -> Y)

c. component retrieves submitted value from request (String -> X)
d. component renders retrieved value (X -> String)

Which should be covered by custom converter and how, and which not?

First of all, for c and d component have to know itself how to render 
own type X = how to convert submitted String back to X. Let's name this 
knowledge 'own conversion [String <-> X]'. Generally it is not affected 
by any extra conversion, but I guess there are cases when it should be done!

If no custom converter provided, we can still try to obtain standard 
one, capable to convert [String <-> X] and to use
- a:  getAsObject( (String)value )
- b: getAsString(X)
It won't work, if underlying bean property is not String, but then we 
can pass throw the exception and the message of this exception will be 
absolutely clear to user.

If custom converter provided, but it's unable to cast type returned by 
getAsObject() to our X type, we have two possibility:
1) we can rely on our own 'knowledge' about how to convert [String <-> 
X] and suppose that getAsString() of custom converter returns something 
compatible. Then we can use:
- a: getAsString() + own conversion String -> X
- b: own conversion X->String + getAsObject()

2) we can try to obtain standard converter [String <-> X] and use it 
instead of own 'knowledge' like:
- a: customConv.getAsString() + standardConv.getAsObject()
- b: standardConv.getAsString() + customConv.getAsObject()

The case when custom converter provided and return type of getAsObject() 
is compatible with our X seems to be clear, but in fact it's a most 
complicated one. There are two reason to provide custom converter for 
standard type. Either the value should be rendered by non-textual 
component (i.e. Boolean + checkBox and user going to implement own 
tristate logic, like true|false|unknown) or some data manipulation 
should be done (i.e. Date parsing/formating). I'm not sure how this case 
should be handled to preserve maximum flexibility.

Just an idea: it should be different for both component kinds and 
probably, we must ignore own rendering behavior as well.
For textual components:
- a and *c*: getAsObject()
- b and *d*: getAsString()

For non-textual components (checkBox):
- a,b and *c*: getAsString() + getAsObject()
- *d*: getAsString()

Well, the problem seems to be really complex. I think that providing any 
'light' path for some little part of this problem is not a good idea. 
I'm also not familiar enough with JSF myself to state weather this 
contradict spec or is just a minor issue, which should be worked around 
some way until new better version i.e. 2.0 is released. I've just tried 
to use this framework's feature and discovered a totally inconsistence. 
I can provide test cases as well as check this against RI or figure it 
out clearly in JIRA etc.but that's exactly why I'd like to ask first, 
weather it makes any sense.

best regards,
paul


Mike Kienenberger schrieb:
> Paul, in my own case, I wasn't reading myfaces email in depth the week
> you posted this.   Sometimes things just fall through the cracks
> because people are busy with other things.
>
> Sometimes the issue is so obscure that few people really know how to
> respond to it.
>
> In both cases, the best thing to do is
>
> 1) prove there's a bug.   Either test the same code using the JSF RI
> or another version of MyFaces.   Or show that it contradicts the JSF
> Specs.   It's best to provide a simplified example demonstrating the
> problem -- this would be easy in your particular issue.   The easier
> you make it for someone else to look at the problem, the more likely
> someone will do so.
>
> 2) Provide a patch (like it looks like you're doing above) in JIRA.
> If no one has done anything with it, and the bug is proven and the
> patch is easily reviewed, feel free to ping the list after a week of
> no action.   Repeat weekly until someone either rejects or approves
> it, or otherwise responds :-)  Again, having a test case or example
> makes it easier to test your patch to verify it should be applied.
>
>
>
>
> On 2/27/07, Paul Iov <paul@voller-ernst.de> wrote:
>>
>>  Thank you for answer, Mike.
>>
>>  I'm still not sure, weather the second part is really an unrelated spec
>> issue because it could be just the consequence of the first one.
>>  So I've decide to describe both together thinking, it can help 
>> during code
>> review since it belongs to very common part of core implementation
>>  (org.apache.myfaces.shared.renderkit.RendererUtils). If my
>> suggestion is wrong, the second part can be just ignored. Please see 
>> also my
>> comment to MYFACES-1452, which was the start point of my 
>> 'investigation' :)
>>
>>  I've tried to solve the problem myself, but it was just a partially
>> solution for me (respectively to second part of MYFACES-1532 
>> description).
>> Since no one has answered in mailing list, I've thinking my 
>> suggestion is
>> just wrong and haven't provide this as path. But if I'm right, the same
>> should affect other standard types too (i.e. Date etc.)
>>
>>  Anyway, here's my code. This method is intended to replace existing
>> getBooleanValue() in
>> org.apache.myfaces.shared.renderkit.RendererUtils, which is
>> called from
>> org.apache.myfaces.shared.renderkit.html.HtmlCheckboxRendererBase.encodeEnd(). 
>>
>> public static Boolean
>> getBooleanValueForCheckbox(FacesContext facesContext,
>>  UIComponent component) {
>>  Object value = getObjectValue(component);
>>
>>  // This call relays on the implementation of getObjectValue()!!!
>>  // If uiComponent is not an instance of ValueHolder, the
>>  // IllegalArgumentException should be already thrown.
>>  Converter converter = ((ValueHolder) component).getConverter();
>>
>>  if (null == value || value instanceof Boolean) {
>>  return (Boolean) value;
>>  }
>>
>>  if (null == converter && value instanceof String) {
>>  // it's still correct to convert String into Boolean, because
>>  // Boolean provides constructor Boolean(String x)!
>>  return new Boolean(value.toString());
>>  }
>>
>>  // If component provides no custom converter, we
>>  // must try to obtain a standard one from Application
>>  if (null == converter) {
>>  try {
>>  converter = facesContext.getApplication().createConverter(
>>  value.getClass());
>>  } catch (FacesException ex) {
>>  throw new IllegalArgumentException("Component : "
>>  + getPathToComponent(component)
>>  + " expects value of type Boolean.\n"
>>  + "Neither standard converter for "
>>  + value.getClass().getName()
>>  + " nor custom converter provided.");
>>  }
>>  }
>>
>>  if (null != converter) {
>>  try {
>>
>>  /* Try to convert it into String. That is!
>>  * The getAsObject() provides the conversion of string value's
>>  * representation into custom user type. We are expecting a Boolean,
>>  * but the value itself is not a Boolean, so we need to get String 
>> first.
>>  */
>>  String strValue = converter.getAsString(facesContext,
>>  component, value);
>>
>>  boolean warn = (null == strValue);
>>  warn = warn
>>  || !(strValue.equalsIgnoreCase(Boolean.TRUE.toString()) || strValue
>>  .equalsIgnoreCase(Boolean.TRUE.toString()));
>>
>>  if (warn) {
>>  /* Note that this situation is not an error! We have some
>>  * converter and it got back some string, and no exception has been
>>  * thrown!
>>  */
>>  log.warn("Component "
>>  + getPathToComponent(component)
>>  + " expects value of type Boolean. The value was converted with "
>>  + converter.getClass().getName()
>>  + " into "
>>  + strValue
>>  + ", what is neither true nor false! false assumed.");
>>  }
>>
>>  return new Boolean(strValue);
>>
>>  } catch (ConverterException ex) {
>>  throw new IllegalArgumentException("Component : "
>>  + getPathToComponent(component)
>>  + " expects value of type Boolean.\n"
>>  + "Unable to convert " + value.getClass().getName()
>>  + " into Boolean.");
>>  }
>>
>>  } else {
>>  throw new IllegalArgumentException("Component "
>>  + getPathToComponent(component)
>>  + " expects value of type Boolean but "
>>  + value.getClass().getName() + " was found.");
>>  }
>>
>>  }
>>
>>
>>
>>
>>  regards,
>>  paul
>>
>>  Mike Kienenberger schrieb:
>> On 2/27/07, Paul Iov <paul@voller-ernst.de> wrote:
>>
>> Some other question to Mike. You have just closed MYFACES-1545. I think,
>>  it's something very similar to MYFACES-1532, I've created 13.02.2007. I
>>  have asked about this in both of DEV and USERS lists, but nobody has
>>  answered, so I'm still not sure, whether it's really a bug, or just a
>>  feature. (There are some other related issues too, however they aren't
>>  linked to this one.)  Would you please confirm this or close 1532 too?
>>
>>  Paul, please use a new thread when asking unrelated questions.
>>
>>  I closed MYFACES-1545 for the reasons listed in the issue, after
>>  searching the mailing lists for relevent posts by the reporter.  There
>>  were none.  Custom converters on UISelectOne items are quite common,
>>  and I've yet to have an issue with them.
>>
>>  It sounds like there could be a bug in h:selectBooleanCheckbox that
>>  causes the problem you described in MYFACES-1532.   I've never used a
>>  converter on this component, and it could very well be as you
>>  described, so I'm going to leave that open, especially since you
>>  provided example code.   I vaguely recall issues with the
>>  convertBoolean converter in the sandbox in the distant past, so it
>>  wouldn't be at all surprising.
>>
>>  Again, though, you  have an unrelated second issue described in the
>>  same issue.   I'd recommend voiding that part and opening another
>>  issue (although if it's a spec issue, it'll just be closed out of hand
>>  as those are outside of the scope of MyFaces to handle).
>>
>>
>>
>