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 < thiag...@gmail.com> wrote: > On Fri, Oct 1, 2021 at 5:36 AM Ben Weidig <b...@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 >
