[
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
> [EMAIL PROTECTED]
--
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: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
