Hi, On Wed, Sep 29, 2021 at 3:35 PM Thiago H. de Paula Figueiredo < thiag...@gmail.com> wrote:
> On Sun, Sep 19, 2021 at 7:36 AM Ben Weidig <b...@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
