#55 CustomConverter matching too loose

closed
None
5
2007-02-09
2006-05-03
Matt Benson
No

Here are the current criteria for custom converter
matching:

// we check to see if the destination class is the same
as classA defined in the converter mapping xml.
// we next check if the source class is the same as
classA defined in the converter mapping xml.
// we also to check to see if it is assignable to
either. We then perform these checks in the other
direction for
// classB

//following reformatted here:

(
(
classA.getName().equals(dest.getName())
|| classA.getName().equals(src.getName())
|| classA.isAssignableFrom(dest)
|| classA.isAssignableFrom(src)
)
&&
(
classB.getName().equals(dest.getName())
|| classB.getName().equals(dest.getName())
|| classB.isAssignableFrom(dest)
|| classB.isAssignableFrom(src)
)
)

Problems: since Dozer works without weird classloader
stuff (i.e. it assumes a single classloader) I see no
reason to compare classnames. That would change the
above to:

(
(
classA.isAssignableFrom(dest)
|| classA.isAssignableFrom(src)
)
&&
(
classB.isAssignableFrom(dest)
|| classB.isAssignableFrom(src)
)
)

Assume you have a converter that can convert between
different implementation types of a single interface:

<converter type="FooConverter">
<class-a>my.IF</class-a>
<class-b>my.IF</class-b>
</converter>

You have another converter that can convert
implementations of the interface to e.g. String:

<converter type="BarConverter">
<class-a>java.lang.String</class-a>
<class-b>my.IF</class-b>
</converter>

Your String converter cannot be picked up when looking
for a converter matching (String -> my.IF) because
classA is assignable from dest and classB is assignable
from dest. The only workaround is to put all such
conversions in one converter. I suggest the code become:

(
(
classA.isAssignableFrom(dest)
&& classB.isAssignableFrom(src)
)
||
(
classA.isAssignableFrom(src)
&& classB.isAssignableFrom(dest)
)
)

I am using this with 21Xbranch code; all tests pass,
performance is fine, and I was able to add a decoy
mapping to dozerBeanMapping.xml that breaks testcases
against current code but works as expected with my
modification. Can I commit it?

Discussion

  • Franz Garsombke

    Franz Garsombke - 2006-05-03

    Logged In: YES
    user_id=550744

    Thanks Matt, looks nice. I have commited that change to
    v21XBranch.

    Franz

     
  • Franz Garsombke

    Franz Garsombke - 2006-05-03
    • assigned_to: nobody --> orangeherbert
     
  • Matt Benson

    Matt Benson - 2006-05-03

    Logged In: YES
    user_id=120761

    Hmm... looks like what you committed was the intermediate
    version here. This removed the name tests but still has the
    same basic result as before. My final version reversed the
    &&s and ||s from what you have committed. I tested it by
    creating a ThrowExceptionConverter that did what it said,
    then mapping that converter to HintedOnly on both sides.
    Simply having this "decoy mapping" around breaks the
    existing test case; switching && and || cleans it right up. ;)

     
  • Franz Garsombke

    Franz Garsombke - 2006-05-03

    Logged In: YES
    user_id=550744

    Can you attach your file and I will just take from there.

    Thanks,

    franz

     
  • Matt Benson

    Matt Benson - 2006-05-05

    Logged In: YES
    user_id=120761

    adding changes to dozerbeanmapping.xml and
    CustomConverterContainer.java as patches.

     
  • Matt Tierney

    Matt Tierney - 2006-11-13
    • assigned_to: orangeherbert --> mhtierney
     
  • Matt Tierney

    Matt Tierney - 2007-01-28

    Logged In: YES
    user_id=1236069
    Originator: NO

    Applied suggested code changes to 2.5. Thanks for the code submission

     
  • Matt Tierney

    Matt Tierney - 2007-01-28
    • status: open --> pending
     
  • Matt Tierney

    Matt Tierney - 2007-02-09
    • status: pending --> closed
     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks