Conversion framework is not 'most specific' compatible

[Adam Heath-2]

When Converters.getConverter doesn't find an exact match for a
conversion request, it loops over all registered converts, and asks
then if they can handle it.  However, this looping is not in any
particular order, as it is based on Map.values(), which is mostly random.

This means if there was a Converter registered for Map, and another
registered for LinkedHashMap, that if code tried to convert
org.ofbiz.base.util.collection.LRUMap, it's unknown which Converter
would be returned.  I would say LinkedHashMap is more correct.

Then, if there was a Map and MyNewInterface Converter register, and a
conversion request comes in that implements both those interfaces,
then which one should be returned?

Java compilers(janino is one I am semi-familiar with it's internals),
are designed to handle this.  But the algorithm isn't the simplest.

I suggest that if multiple Converters are found for a request, that an
exception gets thrown, and a warning logged.

[Adrian Crum-2]

--- On Sat, 2/13/10, Adam Heath <[hidden email]> wrote:

> From: Adam Heath <[hidden email]>
> Subject: Conversion framework is not 'most specific' compatible
> To: [hidden email]
> Date: Saturday, February 13, 2010, 1:09 PM
> When Converters.getConverter doesn't
> find an exact match for a
> conversion request, it loops over all registered converts,
> and asks
> then if they can handle it.  However, this looping is
> not in any
> particular order, as it is based on Map.values(), which is
> mostly random.
>
> This means if there was a Converter registered for Map, and
> another
> registered for LinkedHashMap, that if code tried to
> convert
> org.ofbiz.base.util.collection.LRUMap, it's unknown which
> Converter
> would be returned.  I would say LinkedHashMap is more
> correct.
>
> Then, if there was a Map and MyNewInterface Converter
> register, and a
> conversion request comes in that implements both those
> interfaces,
> then which one should be returned?
>
> Java compilers(janino is one I am semi-familiar with it's
> internals),
> are designed to handle this.  But the algorithm isn't
> the simplest.
>
> I suggest that if multiple Converters are found for a
> request, that an
> exception gets thrown, and a warning logged.

Why not add code that finds the closest match (or most specific) in the case of more than one match?

-Adrian

[Adam Heath-2]

Adrian Crum wrote:

> --- On Sat, 2/13/10, Adam Heath <[hidden email]> wrote:
>> From: Adam Heath <[hidden email]>
>> Subject: Conversion framework is not 'most specific' compatible
>> To: [hidden email]
>> Date: Saturday, February 13, 2010, 1:09 PM
>> When Converters.getConverter doesn't
>> find an exact match for a
>> conversion request, it loops over all registered converts,
>> and asks
>> then if they can handle it.  However, this looping is
>> not in any
>> particular order, as it is based on Map.values(), which is
>> mostly random.
>>
>> This means if there was a Converter registered for Map, and
>> another
>> registered for LinkedHashMap, that if code tried to
>> convert
>> org.ofbiz.base.util.collection.LRUMap, it's unknown which
>> Converter
>> would be returned.  I would say LinkedHashMap is more
>> correct.
>>
>> Then, if there was a Map and MyNewInterface Converter
>> register, and a
>> conversion request comes in that implements both those
>> interfaces,
>> then which one should be returned?
>>
>> Java compilers(janino is one I am semi-familiar with it's
>> internals),
>> are designed to handle this.  But the algorithm isn't
>> the simplest.
>>
>> I suggest that if multiple Converters are found for a
>> request, that an
>> exception gets thrown, and a warning logged.
>
> Why not add code that finds the closest match (or most specific) in the case of more than one match?

Because such code is complex.  Let's say there are converters for Map,
FooInterface, BarInterface, BazInterface, BarAndBazInterface, and
AbstractMapAndFooInterface.  Then, and object that extends
AbstractMapAndFooInterface, and implements both BarInterface and
BazInterface.  Picking the correct converter is a little difficult in
this case.

[Adrian Crum-2]

--- On Sat, 2/13/10, Adam Heath <[hidden email]> wrote:

> From: Adam Heath <[hidden email]>
> Subject: Re: Conversion framework is not 'most specific' compatible
> To: [hidden email]
> Date: Saturday, February 13, 2010, 4:58 PM
> Adrian Crum wrote:
> > --- On Sat, 2/13/10, Adam Heath <[hidden email]>
> wrote:
> >> From: Adam Heath <[hidden email]>
> >> Subject: Conversion framework is not 'most
> specific' compatible
> >> To: [hidden email]
> >> Date: Saturday, February 13, 2010, 1:09 PM
> >> When Converters.getConverter doesn't
> >> find an exact match for a
> >> conversion request, it loops over all registered
> converts,
> >> and asks
> >> then if they can handle it.  However, this
> looping is
> >> not in any
> >> particular order, as it is based on Map.values(),
> which is
> >> mostly random.
> >>
> >> This means if there was a Converter registered for
> Map, and
> >> another
> >> registered for LinkedHashMap, that if code tried
> to
> >> convert
> >> org.ofbiz.base.util.collection.LRUMap, it's
> unknown which
> >> Converter
> >> would be returned.  I would say LinkedHashMap
> is more
> >> correct.
> >>
> >> Then, if there was a Map and MyNewInterface
> Converter
> >> register, and a
> >> conversion request comes in that implements both
> those
> >> interfaces,
> >> then which one should be returned?
> >>
> >> Java compilers(janino is one I am semi-familiar
> with it's
> >> internals),
> >> are designed to handle this.  But the
> algorithm isn't
> >> the simplest.
> >>
> >> I suggest that if multiple Converters are found
> for a
> >> request, that an
> >> exception gets thrown, and a warning logged.
> >
> > Why not add code that finds the closest match (or most
> specific) in the case of more than one match?
>
> Because such code is complex.  Let's say there are
> converters for Map,
> FooInterface, BarInterface, BazInterface,
> BarAndBazInterface, and
> AbstractMapAndFooInterface.  Then, and object that
> extends
> AbstractMapAndFooInterface, and implements both
> BarInterface and
> BazInterface.  Picking the correct converter is a
> little difficult in
> this case.

Good point, but couldn't we at least try to come up with a set of rules or a strategy instead of throwing an exception? It seems to me that throwing an exception is an easy way out for the designer, and a big pain in the butt for the user.

[Adam Heath-2]

Adrian Crum wrote:

> --- On Sat, 2/13/10, Adam Heath <[hidden email]> wrote:
>> From: Adam Heath <[hidden email]>
>> Subject: Re: Conversion framework is not 'most specific' compatible
>> To: [hidden email]
>> Date: Saturday, February 13, 2010, 4:58 PM
>> Adrian Crum wrote:
>>> --- On Sat, 2/13/10, Adam Heath <[hidden email]>
>> wrote:
>>>> From: Adam Heath <[hidden email]>
>>>> Subject: Conversion framework is not 'most
>> specific' compatible
>>>> To: [hidden email]
>>>> Date: Saturday, February 13, 2010, 1:09 PM
>>>> When Converters.getConverter doesn't
>>>> find an exact match for a
>>>> conversion request, it loops over all registered
>> converts,
>>>> and asks
>>>> then if they can handle it.  However, this
>> looping is
>>>> not in any
>>>> particular order, as it is based on Map.values(),
>> which is
>>>> mostly random.
>>>>
>>>> This means if there was a Converter registered for
>> Map, and
>>>> another
>>>> registered for LinkedHashMap, that if code tried
>> to
>>>> convert
>>>> org.ofbiz.base.util.collection.LRUMap, it's
>> unknown which
>>>> Converter
>>>> would be returned.  I would say LinkedHashMap
>> is more
>>>> correct.
>>>>
>>>> Then, if there was a Map and MyNewInterface
>> Converter
>>>> register, and a
>>>> conversion request comes in that implements both
>> those
>>>> interfaces,
>>>> then which one should be returned?
>>>>
>>>> Java compilers(janino is one I am semi-familiar
>> with it's
>>>> internals),
>>>> are designed to handle this.  But the
>> algorithm isn't
>>>> the simplest.
>>>>
>>>> I suggest that if multiple Converters are found
>> for a
>>>> request, that an
>>>> exception gets thrown, and a warning logged.
>>> Why not add code that finds the closest match (or most
>> specific) in the case of more than one match?
>>
>> Because such code is complex.  Let's say there are
>> converters for Map,
>> FooInterface, BarInterface, BazInterface,
>> BarAndBazInterface, and
>> AbstractMapAndFooInterface.  Then, and object that
>> extends
>> AbstractMapAndFooInterface, and implements both
>> BarInterface and
>> BazInterface.  Picking the correct converter is a
>> little difficult in
>> this case.
>
> Good point, but couldn't we at least try to come up with a set of rules or a strategy instead of throwing an exception? It seems to me that throwing an exception is an easy way out for the designer, and a big pain in the butt for the user.

If you agree that we shouldn't throw an exception, then I'll implement
the code.  I agree that the exception way sucks.

[Adrian Crum-2]

--- On Sat, 2/13/10, Adam Heath <[hidden email]> wrote:

> From: Adam Heath <[hidden email]>
> Subject: Re: Conversion framework is not 'most specific' compatible
> To: [hidden email]
> Date: Saturday, February 13, 2010, 5:10 PM
> Adrian Crum wrote:
> > --- On Sat, 2/13/10, Adam Heath <[hidden email]>
> wrote:
> >> From: Adam Heath <[hidden email]>
> >> Subject: Re: Conversion framework is not 'most
> specific' compatible
> >> To: [hidden email]
> >> Date: Saturday, February 13, 2010, 4:58 PM
> >> Adrian Crum wrote:
> >>> --- On Sat, 2/13/10, Adam Heath <[hidden email]>
> >> wrote:
> >>>> From: Adam Heath <[hidden email]>
> >>>> Subject: Conversion framework is not
> 'most
> >> specific' compatible
> >>>> To: [hidden email]
> >>>> Date: Saturday, February 13, 2010, 1:09
> PM
> >>>> When Converters.getConverter doesn't
> >>>> find an exact match for a
> >>>> conversion request, it loops over all
> registered
> >> converts,
> >>>> and asks
> >>>> then if they can handle it.  However,
> this
> >> looping is
> >>>> not in any
> >>>> particular order, as it is based on
> Map.values(),
> >> which is
> >>>> mostly random.
> >>>>
> >>>> This means if there was a Converter
> registered for
> >> Map, and
> >>>> another
> >>>> registered for LinkedHashMap, that if code
> tried
> >> to
> >>>> convert
> >>>> org.ofbiz.base.util.collection.LRUMap,
> it's
> >> unknown which
> >>>> Converter
> >>>> would be returned.  I would say
> LinkedHashMap
> >> is more
> >>>> correct.
> >>>>
> >>>> Then, if there was a Map and
> MyNewInterface
> >> Converter
> >>>> register, and a
> >>>> conversion request comes in that
> implements both
> >> those
> >>>> interfaces,
> >>>> then which one should be returned?
> >>>>
> >>>> Java compilers(janino is one I am
> semi-familiar
> >> with it's
> >>>> internals),
> >>>> are designed to handle this.  But
> the
> >> algorithm isn't
> >>>> the simplest.
> >>>>
> >>>> I suggest that if multiple Converters are
> found
> >> for a
> >>>> request, that an
> >>>> exception gets thrown, and a warning
> logged.
> >>> Why not add code that finds the closest match
> (or most
> >> specific) in the case of more than one match?
> >>
> >> Because such code is complex.  Let's say
> there are
> >> converters for Map,
> >> FooInterface, BarInterface, BazInterface,
> >> BarAndBazInterface, and
> >> AbstractMapAndFooInterface.  Then, and object
> that
> >> extends
> >> AbstractMapAndFooInterface, and implements both
> >> BarInterface and
> >> BazInterface.  Picking the correct converter
> is a
> >> little difficult in
> >> this case.
> >
> > Good point, but couldn't we at least try to come up
> with a set of rules or a strategy instead of throwing an
> exception? It seems to me that throwing an exception is an
> easy way out for the designer, and a big pain in the butt
> for the user.
>
> If you agree that we shouldn't throw an exception, then
> I'll implement
> the code.  I agree that the exception way sucks.

As you can see, the method of cataloging the converters is extremely simple. There is no doubt it could be made more sophisticated. So, yes - I agree.

I'm 100% on board with your efforts, so don't expect a lot of push back from me.

[Adrian Crum-2]

--- On Sat, 2/13/10, Adam Heath <[hidden email]> wrote:
> If you agree that we shouldn't throw an exception, then
> I'll implement
> the code.  I agree that the exception way sucks.

The only real cost of a detailed class/interface analysis is the first time an unknown pair of classes request a converter. Once a close match is found, then the converter is cataloged for that class pair and subsequent requests are returned quickly.