Re: TAP5-2688: CoercionTuple lookup cost vs. quality

[Ben Weidig <be...@netzgut.net>]

Hi,

On Wed, Sep 29, 2021 at 3:35 PM Thiago H. de Paula Figueiredo <
thiagohp@gmail.com> wrote:

> On Sun, Sep 19, 2021 at 7:36 AM Ben Weidig <be...@netzgut.net> wrote:
>
> > Hi,
> >
>
> Hello!
>
> I'm sorry for the very late answer. I thought I had answered this already.
>
> > I'm working on TAP5-2688, which is about the TypeCoercer picking the
> > "wrong" CoercionTuple.
> > Because JSONArray became a real Collection, its String->JSONArray
> > CoercionTuple is picked over the Object->List CoercionTuple.
> >
> > https://issues.apache.org/jira/browse/TAP5-2688
> >
> > I've created a patch and added it to the ticket, but I wanted to have
> some
> > feedback before committing and pushing.
> >
>
> Nice!
>
> > The patch contains:
> >
> > - Preferring exact targetType matches over cheaper (less intermediate)
> > coercions.
> >
>
> Awesome!
>
> > - Add an explicit String->Collection CoercionTuple in tapestry-json
> >
>
> Cool! But why in tapestry-json instead of commons?
>


Because the String->JSONArray CoercionTuple added in tapestry-json is the
root of the problem in TAP5-2688.
Without including tapestry-json, everything's fine.
That's why the coercion tests in tapestry-core didn't reveal the problem,
and the tapestry-json tests didn't include the special-case.


> What's up to discussion IMO is the general question about cost vs.
> quality.
> > Previously "cost" was favored over quality because no quality checks
> > existed except the exact source and target type checks.
> >
> > The patch I've added to the ticket now prefers quality over cost.
> > That means that the first coercion of a value will check all coercion
> > tuples if a better match with an exact targetType is found.
> > The only problem I see with this approach is costlier first checks (it's
> > cached afterward), and maybe a more costly way is chosen even if a
> cheaper
> > "good enough" CoercionTuple would suffice.
> >
>
> I believe this is exactly the way to go. Calculating the best coercion is
> something very quick and
> the result is permanently cached.
>
> > Possible changes:
> >
> > - Restrict intermediate lookups if a "good enough" CoercionTuple is
> found.
> >   A one or two-step coercion should be preferable over a 5+ coercion.
> >   But what would be a good number?
> >   A maximum of one or two additional level checks sounds good to me.
> >
>
> I was wondering whether we could check for direct coercions (i.e. one step)
> first, then
> 2-step ones, then 3-step ones, and so on. Is this what you're proposing,
> Ben? Since
> this is a very quick computation that will only be done once, I think we
> could stop at
> level 5.
>

Well, I think that's how it's doing it right now, but it ignores "exact"
matches if a "good enough" is found earlier.

That can lead to problems... For example, I've added "Object --> ArrayList"
for testing purposes:

Coercion wanted: String --> List (not availabler in one step)
Good enough: Object --> ArrayList (isAssignableFrom)
Better match: Object --> List (exact target type)

My patch prefers targetType matches over just "isAssignable", and is trying
to find one regardless of the intermediate steps.
Maybe going deeper for lookups isn't necessary.
But checking all the available tuples at the current level to find a
targetType match and not just the first "isAssignableFrom".

That, of course, leads to the problem of detecting the number of steps in
compound coercions.
Right now, I don't see a way to see how many intermediate coercions are
involved.
But it could be added to Coercion, with the default being "1", and
CompoundCoercion to actually calculate it.

Or a coercion tuple could be marked as "exact target types only".
So JSONArray won't match for Collection.

Looks like I have to investigate further and write more tests, but I think
I'm on the right track to improve coercions.


>
> >
> > What do you think?
> >
> > Cheers,
> > Ben
> >
>
>
> --
> Thiago
>


Cheers,
Ben

Re: TAP5-2688: CoercionTuple lookup cost vs. quality

[Ben Weidig <be...@netzgut.net>]

The more I'm trying to work on it, the harder it gets, and I find more
issues...

* CoercionTuple doesn't implement hashCode/equals, but it used to keep
track of already considered tuples.
  I fixed it by using the CoercionTuple.Key instead:
https://github.com/apache/tapestry-5/commit/be972fa5ef31eb5e256fae6bbb5ec29245883e57


* Source/Target match vs. Target-only Match: More coercions might lead to
an exact source and target type match. But the result will most likely be
wrong, thanks to Object-->String is always an intermediate for
non-contributed coercions.


* Inverse Coercions: Right now the following coercion path is valid:
String --> Long, Long --> java.time.Instant, java.time.Instant -->
java.util.Date, java.util.Date --> java.time.Instant

Inverse coercion paths should not be considered. But it's hard to keep
track of it due to the CompoundCoercions.


* How deep to look for a better match: after testing different
combinations, I believe it makes no sense to look for a costlier
target-type match if a "targetType.isAssignableFrom(tupleTargetType)" is
found. We should still check the other coercers with the same cost.
  If the cost is higher for another coercion tuple, it's most likely a
coercion with too many intermediate types to actually do what we want.
  And IMO it's better to be forced to provide an exact Coercion instead of
relying on many intermediate ones.

I have to play around with it some more before I'm confident enough to
commit any code.
Just wanted to present my current findings, so others might interject or
provide feedback if I missed something.


On Fri, Oct 1, 2021 at 8:05 PM Thiago H. de Paula Figueiredo <
thiagohp@gmail.com> wrote:

> On Fri, Oct 1, 2021 at 5:36 AM Ben Weidig <be...@netzgut.net> wrote:
>
> > Hi,
> >
>
> Hi!
>
>
> > Because the String->JSONArray CoercionTuple added in tapestry-json is the
> > root of the problem in TAP5-2688.
> > Without including tapestry-json, everything's fine.
> > That's why the coercion tests in tapestry-core didn't reveal the problem,
> > and the tapestry-json tests didn't include the special-case.
> >
>
> So couldn't we have the test in tapestry-json but the coercion in commons?
>
>
> > Well, I think that's how it's doing it right now, but it ignores "exact"
> > matches if a "good enough" is found earlier.
> >
>
> Great catch! We should definitely favor exact matches when they're
> available.
>
> My patch prefers targetType matches over just "isAssignable", and is trying
> > to find one regardless of the intermediate steps.
> > Maybe going deeper for lookups isn't necessary.
> > But checking all the available tuples at the current level to find a
> > targetType match and not just the first "isAssignableFrom".
> >
>
> Agreed.
>
>
> > That, of course, leads to the problem of detecting the number of steps in
> > compound coercions.
> > Right now, I don't see a way to see how many intermediate coercions are
> > involved.
> > But it could be added to Coercion, with the default being "1", and
> > CompoundCoercion to actually calculate it.
> >
>
> Good idea.
>
>
> > Or a coercion tuple could be marked as "exact target types only".
> > So JSONArray won't match for Collection.
> >
>
> I'd implement this only if preferring exact matches doesn't work.
>
>
> > Looks like I have to investigate further and write more tests, but I
> think
> > I'm on the right track to improve coercions.
> >
>
> I agree. Keep up the good work!
>
>
> >
> >
> > >
> > > >
> > > > What do you think?
> > > >
> > > > Cheers,
> > > > Ben
> > > >
> > >
> > >
> > > --
> > > Thiago
> > >
> >
> >
> > Cheers,
> > Ben
> >
>
>
> --
> Thiago
>

Re: TAP5-2688: CoercionTuple lookup cost vs. quality

["Thiago H. de Paula Figueiredo" <th...@gmail.com>]

On Fri, Oct 1, 2021 at 5:36 AM Ben Weidig <be...@netzgut.net> wrote:

> Hi,
>

Hi!


> Because the String->JSONArray CoercionTuple added in tapestry-json is the
> root of the problem in TAP5-2688.
> Without including tapestry-json, everything's fine.
> That's why the coercion tests in tapestry-core didn't reveal the problem,
> and the tapestry-json tests didn't include the special-case.
>

So couldn't we have the test in tapestry-json but the coercion in commons?


> Well, I think that's how it's doing it right now, but it ignores "exact"
> matches if a "good enough" is found earlier.
>

Great catch! We should definitely favor exact matches when they're
available.

My patch prefers targetType matches over just "isAssignable", and is trying
> to find one regardless of the intermediate steps.
> Maybe going deeper for lookups isn't necessary.
> But checking all the available tuples at the current level to find a
> targetType match and not just the first "isAssignableFrom".
>

Agreed.


> That, of course, leads to the problem of detecting the number of steps in
> compound coercions.
> Right now, I don't see a way to see how many intermediate coercions are
> involved.
> But it could be added to Coercion, with the default being "1", and
> CompoundCoercion to actually calculate it.
>

Good idea.


> Or a coercion tuple could be marked as "exact target types only".
> So JSONArray won't match for Collection.
>

I'd implement this only if preferring exact matches doesn't work.


> Looks like I have to investigate further and write more tests, but I think
> I'm on the right track to improve coercions.
>

I agree. Keep up the good work!


>
>
> >
> > >
> > > What do you think?
> > >
> > > Cheers,
> > > Ben
> > >
> >
> >
> > --
> > Thiago
> >
>
>
> Cheers,
> Ben
>


-- 
Thiago