commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gabriel Belingueres (JIRA)" <j...@apache.org>
Subject [jira] Commented: (BEANUTILS-185) [beanutils] Try to align BeanUtils with JSTL standard tags
Date Sat, 10 Jun 2006 06:51:32 GMT
    [ http://issues.apache.org/jira/browse/BEANUTILS-185?page=comments#action_12415646 ] 

Gabriel Belingueres commented on BEANUTILS-185:
-----------------------------------------------

Hi Niall (answers starting with *)

Thanks for the patches, however I'm not that keen on implementing what 
you propose.

1) In regards to RowSetDynaClass and the createDynaBean() method, the 
contract of that method is IMO clear since the return type is a DynaBean 
- if a specific implementation was required then the return type should 
have been the specific  implementation (such as BasicDynaBean). 
Therefore I'm against introducing your new AbstractRowSetDynaClass as IMO its 
unnecessary.

* I introduced the AbstractRowSetDynaClass class for two reasons I can remember of now:
a) To tell with the code by itself that there is a Factory Method createDynaBean() and it
is OK to create your own type of RowSetDynaClass which create the kind of row beans you want.
b) The introduction of the AbstractRowSetDynaClass class made it really easy to introduce
both the DirectRowSetDynaClass and IndirectRowSetDynaClass (we only implement the createDynaBean()
method)


Looking at RowSetDynaClass I am surprised there is a createDynaBean() 
method at all, since DynaClass defines a perfectly good public 
newInstance() method for that very purpose. IMO we should deprecate the 
createDynaBean() method and switch to using newInstance() instead.

* If I understood it well, I think newInstance() would not be an adequate method to use in
this situation. From the DynaClass javadoc: "A DynaClass is a simulation of the functionality
of java.lang.Class for classes implementing the DynaBean interface".
If in a regular Java program I call newInstance() on the java.util.ArrayList class, I would
not expect to be returned a new Object instance because it is the type of objects an ArrayList
can hold, but I would expect to be returned an instance of a new ArrayList.
With RowSetDynaClass I think it happens the same: calling newInstance() one would expect I
get returned a new RowSetDynaBean (which it is a concept that doesn't exist!), not a BasicDynaBean
instance (which is the type of the elements in the RowSet. I think that it is why when called
it throws an UnsupportedOperationException.

May be we need to replace RowSetDynaClass with something more meaningful? RowSetDynaClass
is like an abstract class: We have the class available but we can't instantiate it. Maybe
we need to align the definition with the standard JDBC RowSet implementation?


2) DirectAccessDynaBean gives me some concern since it is both a Map 
and a DynaBean and I'm unsure whether this may confuse other BeanUtils 
functionality or cause it to act inconsistently. Even if that didn't turn 
out to be a problem I'm against this "super" interface as I don't see 
how this provides any advantage/benefit over an object that implements 
the two interfaces separtely rather than this combined one.

* I agree that this is dangerous. I wanted to use JSTL tags like <c:out value="${dynabean.firstname}"/>
with my dynabeans, so the combination of DynaBean and Map seemed a good idea. The BasicDirectAccessDynaBean
implementation I provided treats the dynabean (when accessed from a Map method) as immutable.
In the one hand, read access was the only thing I needed since I use JSTL with a model 2 mvc
framework (so no writing is performed in the JSP pages), and in the other hand, I wrapped
the map in a Collections.unmodifiableMap to protect the dynabean integrity from writing.
I tend to agree that this super interface may be opening the door for bugs. However, I think
that before dropping it we could consult with a JSF implementer to see if it would simplify
the implementation of expressions like <h:inputText value="#{dynabean.property}"/>,
which will read and WRITE on the dynabean.


An alternative thought I had to this is to provide a Map implementation 
to decorate DynaBeans - that way any DynaBean could be decorated to 
look like a Map?

* This can be a good idea, however if you wrap a DynaBean with a Map interface, then it is
no longer a DynaBean. Don't know if this loss of functionality can be counter productive or
not, but would cover the requirement of being JSTL friendly.
Also, this will require to either change the RowSetDynaClass.copy() implementation to wrap
the dynabean with an optional Map, or (easier I think) add a getRowsAsMap() method which would
create a list of Map-wrapped dynabeans.

3) On the IndirectAccessDynaBean I'm afraid I don't see any benefit in 
JSTL terms since simply adding a getMap() method to a DynaBean 
implementation will enable it to be used by JSTL. Also perhaps it would have 
wider benefit if it was simply an interface with that one method (rather 
than extending DynaBean) - that way any object could provide a Map 
representation of itself.

* I agree in both.

The reasons I added this interface were:
a) To have the alternate method <c:out value="${dynabean.map.firstname}"/> to access
a dynabean in JSTL.
b) Added the exact getMap() method name to align the implementation with the current DynaActionForm
class in Struts (see http://svn.apache.org/viewvc/struts/action/trunk/core/src/main/java/org/apache/struts/action/DynaActionForm.java?view=markup)
so as it can work as a foundation dynabean for other developments (however the BasicIndirectAccessDynaBean
return the values in an un modifiable map, and Struts' implementation does not (don't know
if it is required to do so)
c) For completeness with DirectAccessDynaBean.

Another argument in favour of the Map decorator for a DynaBean is that 
it would make it very easy for DynaBean implementations to implement a 
getMap() method. LazyDynaBean currently has a getMap() - but it exposes 
the internal Map used to store properties - allowing the DynaBean 
methods such as get/set to be circumvented and possible corruption of the 
contents (illegal values for a property). This way the Map returned would 
under the covers still delegate to the proper DynaBean methods.

* Never used LazyDynaBeans before, but it seems to me that they allow to add properties dynamically
to the dynabean. Don't know how good they are implemented but if the only requirement you
have is to operate on a dynabean as if it were a map, then you should not pay for the performance
penalty that may be in the LazyDynaBean class. I think that both requirements (showing as
a Map and adding properties dynamically) are orthogonal and should be best implemented as
wrapped dynabeans (much like the io streams library).

Anyway don't know if all this make any sense since it is almost 4am...

Gabriel

> [beanutils] Try to align BeanUtils with JSTL standard tags
> ----------------------------------------------------------
>
>          Key: BEANUTILS-185
>          URL: http://issues.apache.org/jira/browse/BEANUTILS-185
>      Project: Commons BeanUtils
>         Type: Improvement

>  Environment: Operating System: other
> Platform: Other
>     Reporter: Gabriel Belingueres
>     Priority: Minor
>  Attachments: AbstractRowSetDynaClass.java, BasicDirectAccessDynaBean.java, BasicDirectAccessDynaBeanTestCase.java,
BasicIndirectAccessDynaBean.java, DirectAccessDynaBean.java, DirectRowSetDynaClass.java, IndirectAccessDynaBean.java,
IndirectRowSetDynaClass.java, beanutil-diff.txt
>
> Hi,
> I've done some modifications to the beanutils package to better support the use 
> of DynaBeans with the JSTL tags. Some of the changes are discussed in this 
> thread of the commons-user mailing list:
> http://marc.theaimsgroup.com/?l=jakarta-commons-user&m=114669123403779&w=2
> I attach the diff file that comprises changes to the RowSetDynaClass.java file 
> and build.xml file (since I added a TestCase.) Note: Please try to filter 
> carefully the diffs in the build.xml file since they include some local 
> settings I have for compilation on my machine. :-(
> Together with the diff file, I attach the new java files added to the package.
> Regards,
> Gabriel Belingueres
> gaby@ieee.org

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira


---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message