myfaces-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul Iov <p...@voller-ernst.de>
Subject Re: MYFACES-1545 / MYFACES-1532
Date Thu, 01 Mar 2007 12:14:22 GMT
Hi Martin,

I've checked this with the latest nightly build 1.1.6.
1. getValue() behavior seems to be correct now (at least without 
conversion) :)
2. conversion (and validation?) issue is still present :(

my test case:
bean
private boolean boolBoolean = true;
private String strBoolean = "true";
private Date datDate = new Date();
private String strDate = "01.01.2007";
(with all correct get/set)

page
    <h:selectBooleanCheckbox id="myBoolBoolean" 
value="#{MyBean.boolBoolean}"/>
--OK

  <h:selectBooleanCheckbox id="myStrBoolean" value="#{MyBean.strBoolean}"/>
--fails with Expected submitted value of type Boolean for Component: 
...myStrBoolean

  <h:selectBooleanCheckbox id="myStrBoolean" value="#{MyBean.strBoolean}"
         converter="MyBooleanConverter"/>
--fails with Expected submitted value..., converter not called

  <t:inputCalendar value="#{MyBean.datDate}"/>
--OK

  <t:inputCalendar value="#{MyBean.strDate}"/>
--fails with Expected submitted value...

  <t:inputCalendar value="#{MyBean.strDate}"
                    converter="MyDateConverter" />
--fails with Expected submitted value..., converter not called

"Expected submitted value..." is thrown in all cases during initial 
rendering, so it's not possible to check behavior on submit etc.

Regards,
paul


Martin Marinschek schrieb:
> Hi Paul,
>
> I couldn't follow all your arguments in this rather long mail, but
> here a short thought for you to consider - it might help you
> determining if you've found a bug or not:
>
> the submitted value should always be used for rendering, if it is not
> 'null'. If it is null, the getValue() method of the component should
> be called (I've just found an issue with that, posted to the
> dev-list). getValue() should in fact return a local value, if it is
> set (a submitted value gets to be a local value after conversion and
> validation), if it is not set, the value-binding should be evaluated
> (and the backing-bean value used). Does any of the behaviour you see
> in this component contradict this?
>
> regards,
>
> Martin
>
> On 3/1/07, Paul Iov <paul@voller-ernst.de> wrote:
>> 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).
>> >>
>> >>
>> >>
>> >
>>
>>
>
>


Mime
View raw message