[betwixt] AddDefaultsRule bug in 0.7RC2

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[betwixt] AddDefaultsRule bug in 0.7RC2

Glenn Goldenberg
I think there may be a bug in the org.apache.commons.betwixt.digester.AddDefaultsRule class.  I was testing the "add-adders" and "add-properties" flags on the <addDefaults> element in the .betwixt file.  The flags didn't seem to work correctly, so I debugged into AddDefaultsRule and saw on line 69:

        addProperties = Boolean.valueOf(addAddersAttributeValue).booleanValue();

I changed this to:

        addAdders = Boolean.valueOf(addAddersAttributeValue).booleanValue();

and things worked as expected.

thanks,
glenn

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [betwixt] AddDefaultsRule bug in 0.7RC2

robert burrell donkin
hi glenn

add-adder is indeed buggy. the unit tests seemed half written for this
feature :-/

i'm a little unsure how to proceed. either, the fix put into head or the
current release vote could be cancelled and this fix rolled into a fresh
release candidate.

opinions?

- robert

On Thu, 2005-07-07 at 14:52 -0500, Glenn Goldenberg wrote:

> I think there may be a bug in the org.apache.commons.betwixt.digester.AddDefaultsRule class.  I was testing the "add-adders" and "add-properties" flags on the <addDefaults> element in the .betwixt file.  The flags didn't seem to work correctly, so I debugged into AddDefaultsRule and saw on line 69:
>
> addProperties = Boolean.valueOf(addAddersAttributeValue).booleanValue();
>
> I changed this to:
>
> addAdders = Boolean.valueOf(addAddersAttributeValue).booleanValue();
>
> and things worked as expected.
>
> thanks,
> glenn
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: [betwixt] AddDefaultsRule bug in 0.7RC2

Glenn Goldenberg
In reply to this post by Glenn Goldenberg
Hey robert:

After fixing the "add-adder" bug, I ran into an issue using "updater" in the ElementRule for a property of type Map and another issue with "add-adder" and mixed Collection property types.

The first issue with a Map property is illustrated by this TestBetwixtMapUpdater test case that follows.  The issue is that if "add-adder" is false then the "updater" that is specified for a Map property is ignored.  This issue can't been seen without applying the previous fix for honoring the "add-adder" attribute.  I debugged into the ElementRule class and there was no logic to handle adding a Map "updater".  I was able to path this with the following changes:

in XMLIntrospector, line 959, change visibility of setMapAdder() to public:
        public void setMapAdder(

in ElementRule, line 237, update configureProperty() in two places to handle Map adders:
        ...
                if ( updateMethodName.equals( method.getName() ) ) {
                    // we have a matching name
                   
                    // updater should have one parameter unless type is Map
                    int numParams = 1;
                    if (Map.class.isAssignableFrom(type)) {
                    // updater for Map should have two parameters
                    numParams = 2;
                    }
                   
                    if (methods[i].getParameterTypes().length == numParams) {
                        // we'll use first match
                        updateMethod = methods[i];
                        if ( log.isTraceEnabled() ) {
                            log.trace("Matched method:" + updateMethod);
                        }
                        // done since we're using the first match
                        break;
                    }
                }
        ...
                if (updateMethod == null) {
                if ( log.isInfoEnabled() ) {
                   
                    log.info("No method with name '" + updateMethodName + "' found for update");
                }
            } else {
    // assign updater to elementDescriptor
                                if (Map.class.isAssignableFrom(type)) {
                                        Map elementsByPropertyName = new HashMap();
                                        elementsByPropertyName.put(propertyDescriptor.getName(), elementDescriptor);
                                       
                                        getXMLIntrospector().setMapAdder(elementsByPropertyName, updateMethod);
                                       
                                } else {
                        elementDescriptor.setUpdater( new MethodUpdater( updateMethod ) );
                        elementDescriptor.setSingularPropertyType( updateMethod.getParameterTypes()[0] );
                        if ( log.isTraceEnabled() ) {
                            log.trace( "Set custom updater on " + elementDescriptor);
                        }
                    }
            }

With these changes, the TestBetwixtMapUpdater test passes (along with all other tests).  It seemed easy to change the visibility of setMapAdder() and reuse that logic in the ElementRule, but I don't know if that is the best solution...

The other issue is that using addDefaults in a class where you want to display a mixed collection causes the mixed collection elements to not display properly.  It seems that if an ElementRule is specified with no name attribute and a mixed Collection property, than the output XML only has element tags based on the collection object class type if "add-adders" is false.  It looks like when adding default Adders, betwixt overwrites the settings for mixed collection properties somehow.  If you comment out the addChildBean() method, then "add-adders='true'" works, but the beanReader won't be able to convert the xml back into the bean.  Here is the result from TestBetwixtMixedCollection which is included below as well:

with <addDefaults add-properties='true' add-adders='true'/>:
 <?xml version="1.0"?>
  <parentBean stuff="stuff" id="1">
    <childBeans>
      <childBean/>
      <childBean/>
    </childBeans>
  </parentBean>

with <addDefaults add-properties='true' add-adders='false'/>:
 <?xml version="1.0"?>
  <parentBean id="1">
    <childBeans>
      <childBean1/>
      <childBean2/>
    </childBeans>
  </parentBean>

Please let me know if I'm missing something around these two issues...

Thanks,
glenn

*********************************************************************************************

package random;

import java.io.StringReader;
import java.io.StringWriter;
import java.util.HashMap;
import java.util.Map;

import org.apache.commons.betwixt.io.BeanReader;
import org.apache.commons.betwixt.io.BeanWriter;
import org.xml.sax.InputSource;

import junit.framework.Test;
import junit.framework.TestCase;
import junit.framework.TestSuite;

public class TestBetwixtMapUpdater extends TestCase
{

        public TestBetwixtMapUpdater(String name)
        {
        super(name);
    }
   
    public static Test suite()
    {
        TestSuite suite = new TestSuite(TestBetwixtMapUpdater.class);
       
        return suite;
    }
   
    public void testMapUpdater()
    {
    String config =
                "<?xml version='1.0'?>" +
                "<betwixt-config primitiveTypes='attribute'>" +
                "    <class name='random.TestBetwixtMapUpdater$ParentBean'>" +
                "        <element name='parentBean'>" +
                "            <element name='pairs'>" +
                "                <element property='pairs' updater='addPair'/>" +
                "            </element>" +
                "            <addDefaults add-properties='true' add-adders='false'/>" +
                "        </element>" +
                "    </class>" +
                "</betwixt-config>";
   
    String result =
            "<?xml version=\"1.0\"?>\n" +
            "  <parentBean id=\"1\">\n" +
            "    <pairs>\n" +
            "      <entry id=\"2\">\n" +
            "        <key>key</key>\n" +
            "        <value>value</value>\n" +
            "      </entry>\n" +
            "    </pairs>\n" +
            "  </parentBean>\n";
   
    try
    {
    ParentBean pb = new ParentBean();;
    pb.getPairs().put("key", "value");
   
    StringWriter writer = new StringWriter();
    BeanWriter beanWriter = new BeanWriter(writer);
            beanWriter.enablePrettyPrint();
        beanWriter.getXMLIntrospector().register(new InputSource(new StringReader(config)));
        beanWriter.writeXmlDeclaration("<?xml version=\"1.0\"?>");
                beanWriter.write(pb);
               
                System.out.println(writer.toString());
               
                assertEquals(result, writer.toString());
               
                BeanReader beanReader  = new BeanReader();
                        beanReader.registerMultiMapping(new InputSource(new StringReader(config)));
                       
                       
                        ParentBean pbRead = (ParentBean)beanReader.parse(new StringReader(writer.toString()));
                               
                        StringWriter writer2 = new StringWriter();
    BeanWriter beanWriter2 = new BeanWriter(writer2);
            beanWriter2.enablePrettyPrint();
        beanWriter2.getXMLIntrospector().register(new InputSource(new StringReader(config)));
        beanWriter2.writeXmlDeclaration("<?xml version=\"1.0\"?>");
                beanWriter2.write(pbRead);
               
                System.out.println(writer2.toString());
               
                assertEquals(result, writer2.toString());
    }
    catch (Exception e)
    {
    fail(e.getMessage());
    }
    }
   
    public static class ParentBean
    {
    private Map pairs = new HashMap();

                public Map getPairs()
                {
                        return pairs;
                }

                public void setPairs(Map pairs)
                {
                        this.pairs = pairs;
                }
               
                public void addPair(String key, String value)
                {
                        pairs.put(key, value);
                }
   
    }
   
}

*********************************************************************************************

package random;

import java.io.StringReader;
import java.io.StringWriter;
import java.util.ArrayList;
import java.util.List;

import org.apache.commons.betwixt.io.BeanWriter;
import org.xml.sax.InputSource;

import junit.framework.Test;
import junit.framework.TestCase;
import junit.framework.TestSuite;

public class TestBetwixtMixedCollection extends TestCase
{

        public TestBetwixtMixedCollection(String name)
        {
        super(name);
    }
   
    public static Test suite()
    {
        TestSuite suite = new TestSuite(TestBetwixtMixedCollection.class);
       
        return suite;
    }
   
    public void testWithDefaults()
    {
    toXml(true);
    }
   
    public void testWithoutDefaults()
    {
    toXml(false);
    }
   
    protected void toXml(boolean addAdders)
    {    
    StringReader configReader = new StringReader(
                "<?xml version='1.0' ?>" +
                "<betwixt-config primitiveTypes='attribute'>" +
                "    <class name='random.TestBetwixtMixedCollection$ParentBean'>" +
                "        <element name='parentBean'>" +
                "            <element name='childBeans'>" +
                "                <element property='childBeans'/>" +
                "            </element>" +
                "            <addDefaults add-properties='true' add-adders='" + addAdders + "'/>" +
                "        </element>" +
                "    </class>" +
                "    <class name='random.TestBetwixtMixedCollection$ChildBean1'>" +
                "        <element name='childBean1'>" +
                "            <addDefaults/>"  +
                "        </element>" +
                "    </class>" +
                "    <class name='random.TestBetwixtMixedCollection$ChildBean2'>" +
                "        <element name='childBean2'>" +
                "            <addDefaults/>"  +
                "        </element>" +
                "    </class>" +
                "</betwixt-config>");
   
    try
    {
    ParentBean pb = new ParentBean();
    pb.setStuff("stuff");
    ChildBean1 cb1 = new ChildBean1();
    pb.getChildBeans().add(cb1);
    ChildBean2 cb2 = new ChildBean2();
    pb.getChildBeans().add(cb2);
   
    StringWriter writer = new StringWriter();
    BeanWriter beanWriter = new BeanWriter(writer);
            beanWriter.enablePrettyPrint();
        beanWriter.getXMLIntrospector().register(new InputSource(configReader));
        beanWriter.writeXmlDeclaration("<?xml version=\"1.0\"?>");
                beanWriter.write(pb);
               
                System.out.println(writer.toString());
   
    }
    catch (Exception e)
    {
    fail(e.getMessage());
    }
    }
   
    public static class ParentBean
    {
    private List childBeans = new ArrayList();
    private String stuff = null;
   
    public List getChildBeans()
    {
    return childBeans;
    }
   
    public void setChildBeans(List childBeans)
    {
    this.childBeans = childBeans;
    }
   
    public void addChildBean(ChildBean childBean)
    {
    getChildBeans().add(childBean);
    }

                public String getStuff()
                {
                        return stuff;
                }

                public void setStuff(String stuff)
                {
                        this.stuff = stuff;
                }
    }
   
    public static abstract class ChildBean
    {
    }
   
    public static class ChildBean1 extends ChildBean
    {
    }
   
    public static class ChildBean2 extends ChildBean
    {
    }
   
}

*********************************************************************************************

-----Original Message-----
From: robert burrell donkin
[mailto:[hidden email]]
Sent: Monday, July 11, 2005 1:30 PM
To: Jakarta Commons Users List
Subject: Re: [betwixt] AddDefaultsRule bug in 0.7RC2


hi glenn

add-adder is indeed buggy. the unit tests seemed half written for this
feature :-/

i'm a little unsure how to proceed. either, the fix put into head or the
current release vote could be cancelled and this fix rolled into a fresh
release candidate.

opinions?

- robert

On Thu, 2005-07-07 at 14:52 -0500, Glenn Goldenberg wrote:

> I think there may be a bug in the org.apache.commons.betwixt.digester.AddDefaultsRule class.  I was testing the "add-adders" and "add-properties" flags on the <addDefaults> element in the .betwixt file.  The flags didn't seem to work correctly, so I debugged into AddDefaultsRule and saw on line 69:
>
> addProperties = Boolean.valueOf(addAddersAttributeValue).booleanValue();
>
> I changed this to:
>
> addAdders = Boolean.valueOf(addAddersAttributeValue).booleanValue();
>
> and things worked as expected.
>
> thanks,
> glenn
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: [betwixt] AddDefaultsRule bug in 0.7RC2

Simon Kitching
On Mon, 2005-07-11 at 16:22 -0500, Glenn Goldenberg wrote:
> Hey robert:
>
> After fixing the "add-adder" bug, I ran into an issue using "updater" in the ElementRule
>  for a property of type Map and another issue with "add-adder" and mixed Collection property types.

Hmm..this looks more complex to fix. Looks like some unit tests are
badly needed for this stuff too (as Robert said earlier).

Robert: If you think this is becoming too much to add at the last minute
and you want to release 0.7 before tacking this stuff for 0.7.1 then my
+1 to release the current RC2 still stands..


Regards,

Simon


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: [betwixt] AddDefaultsRule bug in 0.7RC2

robert burrell donkin
On Tue, 2005-07-12 at 19:02 +1200, Simon Kitching wrote:
> On Mon, 2005-07-11 at 16:22 -0500, Glenn Goldenberg wrote:
> > Hey robert:
> >
> > After fixing the "add-adder" bug, I ran into an issue using "updater" in the ElementRule
> >  for a property of type Map and another issue with "add-adder" and mixed Collection property types.
>
> Hmm..this looks more complex to fix.

that's probable

i did suspect that there'd be a few more issues to sort out. not sure
how many are actually just code limitations (addDefaults is more than a
little peculiar) which would mean added extra features rather than
coding bugs.

> Looks like some unit tests are
> badly needed for this stuff too (as Robert said earlier).

yep. not sure why they were never completed

> Robert: If you think this is becoming too much to add at the last minute

it's the drip-drip that worries me

there are already another couple of issues raised by users which lie
somewhere between bugs and features. this release has already been
ongoing for a month and that seems like a long time to me...

> and you want to release 0.7 before tacking this stuff for 0.7.1 then my
> +1 to release the current RC2 still stands..

that vote's dead

i think that the best course of action is to accept that RC2 is good
enough for now and add something to the release notes about the know
add-adder bug. i'll then start a new VOTE thread.

- robert


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: [betwixt] AddDefaultsRule bug in 0.7RC2

robert burrell donkin
In reply to this post by Glenn Goldenberg
On Mon, 2005-07-11 at 16:22 -0500, Glenn Goldenberg wrote:
> Hey robert:

hi glenn

> After fixing the "add-adder" bug, I ran into an issue using "updater" in the ElementRule for a property of type Map
> and another issue with "add-adder" and mixed Collection property types.

i've committed the fix (together with a test case) to HEAD. (needed some
time to discuss whether the fix belongs in HEAD or in the 0.7 release.)

you help's appreciated. if (in future) you want to help to speed the
process of application, please read
http://jakarta.apache.org/site/getinvolved.html and consider creating
some patches (once you get the hang of them, you'll find them easier
than describing things in words). if you find your emails are getting a
little long, then you could open a report in
http://issues.apache.org/bugzilla/ (if you like) and then follow up on
list .  

> The first issue with a Map property is illustrated by this TestBetwixtMapUpdater test case that follows.  
> The issue is that if "add-adder" is false then the "updater" that is specified for a Map property is ignored.  
> This issue can't been seen without applying the previous fix for honoring the "add-adder" attribute.

yep

> I debugged into the ElementRule class and there was no logic to handle adding a Map "updater".

it's probably on the (very long) to do list...

> I was able to path this with the following changes:
>
> in XMLIntrospector, line 959, change visibility of setMapAdder() to public:
> public void setMapAdder(
>
> in ElementRule, line 237, update configureProperty() in two places to handle Map adders:
> ...
> if ( updateMethodName.equals( method.getName() ) ) {
>                     // we have a matching name
>                    
>                     // updater should have one parameter unless type is Map
>                     int numParams = 1;
>                     if (Map.class.isAssignableFrom(type)) {
>                     // updater for Map should have two parameters
>                     numParams = 2;
>                     }
>                    
>                     if (methods[i].getParameterTypes().length == numParams) {
>                         // we'll use first match
>                         updateMethod = methods[i];
>                         if ( log.isTraceEnabled() ) {
>                             log.trace("Matched method:" + updateMethod);
>                         }
>                         // done since we're using the first match
>                         break;
>                     }
>                 }
> ...
> if (updateMethod == null) {
>                 if ( log.isInfoEnabled() ) {
>                    
>                     log.info("No method with name '" + updateMethodName + "' found for update");
>                 }
>             } else {
>     // assign updater to elementDescriptor
> if (Map.class.isAssignableFrom(type)) {
> Map elementsByPropertyName = new HashMap();
> elementsByPropertyName.put(propertyDescriptor.getName(), elementDescriptor);
>
> getXMLIntrospector().setMapAdder(elementsByPropertyName, updateMethod);
>
> } else {
>                elementDescriptor.setUpdater( new MethodUpdater( updateMethod ) );
>                elementDescriptor.setSingularPropertyType( updateMethod.getParameterTypes()[0] );
>                if ( log.isTraceEnabled() ) {
>                    log.trace( "Set custom updater on " + elementDescriptor);
>                }
>            }
>             }
>
> With these changes, the TestBetwixtMapUpdater test passes (along with all other tests).  
> It seemed easy to change the visibility of setMapAdder() and reuse that logic in the ElementRule,
> but I don't know if that is the best solution...

reusing the logic is important but so is keeping a compact API. thanks
for pinpointing the issue. i'll probably have a think and maybe post
something on dev...

> The other issue is that using addDefaults in a class where you want to display a mixed collection
> causes the mixed collection elements to not display properly.  It seems that if an ElementRule is
> specified with no name attribute and a mixed Collection property, than the output XML only has
> element tags based on the collection object class type if "add-adders" is false.  It looks like
> when adding default Adders, betwixt overwrites the settings for mixed collection properties somehow.

sounds about right. one of the basic design issues is that adding
defaults also performs normalization but it's not easy to fix whilst
retaining good backwards compatibility.

> If you comment out the addChildBean() method, then "add-adders='true'" works,
> but the beanReader won't be able to convert the xml back into the bean.  
> Here is the result from TestBetwixtMixedCollection which is included below as well:
>
> with <addDefaults add-properties='true' add-adders='true'/>:
>  <?xml version="1.0"?>
>   <parentBean stuff="stuff" id="1">
>     <childBeans>
>       <childBean/>
>       <childBean/>
>     </childBeans>
>   </parentBean>
>
> with <addDefaults add-properties='true' add-adders='false'/>:
>  <?xml version="1.0"?>
>   <parentBean id="1">
>     <childBeans>
>       <childBean1/>
>       <childBean2/>
>     </childBeans>
>   </parentBean>
>
> Please let me know if I'm missing something around these two issues...

i'll need a little time to think on this (and maybe discuss on dev).

BTW are you happy to contribute your test cases (which i've deleted from
the bottom of this mail) to apache? (saves me having to create fresh
ones.) just FYI if you attach the apache software license 2.0 to the top
of any new classes you post then i won't have to ask ;)

- robert



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: [betwixt] AddDefaultsRule bug in 0.7RC2

Glenn Goldenberg
In reply to this post by Glenn Goldenberg
hi robert:

> you help's appreciated. if (in future) you want to help to speed the
> process of application, please read
> http://jakarta.apache.org/site/getinvolved.html and consider creating
> some patches (once you get the hang of them, you'll find them easier
> than describing things in words). if you find your emails are getting a
> little long, then you could open a report in
> http://issues.apache.org/bugzilla/ (if you like) and then follow up on
> list .

Thanks, I will take a look at this for contributing in the future...

> BTW are you happy to contribute your test cases (which i've deleted from
> the bottom of this mail) to apache? (saves me having to create fresh
> ones.) just FYI if you attach the apache software license 2.0 to the top
> of any new classes you post then i won't have to ask ;)

Sure, no problem.  Glad you can use the tests.

thanks,
glenn


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: [betwixt] AddDefaultsRule bug in 0.7RC2

robert burrell donkin
On Thu, 2005-07-14 at 16:25 -0500, Glenn Goldenberg wrote:

> hi robert:
>
> > you help's appreciated. if (in future) you want to help to speed the
> > process of application, please read
> > http://jakarta.apache.org/site/getinvolved.html and consider creating
> > some patches (once you get the hang of them, you'll find them easier
> > than describing things in words). if you find your emails are getting a
> > little long, then you could open a report in
> > http://issues.apache.org/bugzilla/ (if you like) and then follow up on
> > list .
>
> Thanks, I will take a look at this for contributing in the future...
>
> > BTW are you happy to contribute your test cases (which i've deleted from
> > the bottom of this mail) to apache? (saves me having to create fresh
> > ones.) just FYI if you attach the apache software license 2.0 to the top
> > of any new classes you post then i won't have to ask ;)
>
> Sure, no problem.  Glad you can use the tests.

(back from apachecon now which was very cool BTW)

great. thanks.

- robert


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: [betwixt] AddDefaultsRule bug in 0.7RC2

robert burrell donkin
In reply to this post by Glenn Goldenberg
hi glenn

it's been a bit longer than i'd intended. sorry.

i've committed solutions to both the issues you raised. for the problem
with the mixed collection you'll need to set the new guess-names
property to false. there's some information in the documentation about
this.

- robert

On Thu, 2005-07-14 at 16:25 -0500, Glenn Goldenberg wrote:

> hi robert:
>
> > you help's appreciated. if (in future) you want to help to speed the
> > process of application, please read
> > http://jakarta.apache.org/site/getinvolved.html and consider creating
> > some patches (once you get the hang of them, you'll find them easier
> > than describing things in words). if you find your emails are getting a
> > little long, then you could open a report in
> > http://issues.apache.org/bugzilla/ (if you like) and then follow up on
> > list .
>
> Thanks, I will take a look at this for contributing in the future...
>
> > BTW are you happy to contribute your test cases (which i've deleted from
> > the bottom of this mail) to apache? (saves me having to create fresh
> > ones.) just FYI if you attach the apache software license 2.0 to the top
> > of any new classes you post then i won't have to ask ;)
>
> Sure, no problem.  Glad you can use the tests.
>
> thanks,
> glenn
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]