DefaultConverter has many broken cases

[ctrueden]

The DefaultConverter was not being tested directly. With 4e40e04189cafcfad4c66357cbad17727dbf6325, now it is. But there are many cases that do not work, marked with commented out tests. (Also many missing tests, but at least it's a start!)

This issue exists to document some of the known problems, and thoughts on how best to resolve them.

Just to give an inkling of the problems highlighted by this issue:

--- a/src/main/java/org/scijava/convert/DefaultConverter.java
+++ b/src/main/java/org/scijava/convert/DefaultConverter.java
@@ -73,6 +73,7 @@ public class DefaultConverter extends AbstractConverter<Object, Object> {

        @Override
        public Object convert(final Object src, final Type dest) {
+               if (!canConvert(src, dest)) return null;

                // Handle array types, including generic array types.
                final Type componentType = Types.component(dest);
@@ -92,6 +93,8 @@ public class DefaultConverter extends AbstractConverter<Object, Object> {

        @Override
        public <T> T convert(final Object src, final Class<T> dest) {
+               if (!canConvert(src, dest)) return null;
+
                // ensure type is well-behaved, rather than a primitive type
                final Class<T> saneDest = Types.box(dest);

:point_up: This patch breaks three tests. :open_mouth:

So let's consider the following questions:

What would you expect to happen in the following circumstances? You ask SciJava Common to convert some data you have, to a particular type:

1. "{{0, 1}, {2, 3}}" → double[][]. Do you expect: A) [[0d, 1d], [2d, 3d]]? B) null?
2. "{{0, 1}, {curtis, 3}}" → Double[][]. Do you expect: A) [[0d, 1d], [null, 3d]]? B) null?
3. "{{0, 1}, {curtis, 3}}" → double[][]. Do you expect: A) [[0d, 1d], [NaN, 3d]]? B) null?
4. "5" → double[]. Do you expect: A) [5d]? B) null?
5. "5" → double[][]. Do you expect: A) [[5d]]? B) null?
6. "curtis" → double[]. Do you expect: A) [NaN]? B) null?
7. "curtis" → Double[]. Do you expect: A) [null]? B) null?
8. "curtis" → Double[][]. Do you expect: A) [[null]]? B) null?

I think the following behavior is what we should shoot for:

• When converting to an array/collection type, we first go to Object[], Object[][], etc., of correct dimensionality.
• If dimensionality of this initially converted source is less than dimensionality of target, we wrap it in extra dimensions till it's the same.
• If dimensionality of the initially converted source is more than the dimensionality of target, return null.
• Otherwise: we allocate the destination object of the requested type, then convert source's leaf elements one by one to destination elements.

E.g. for (8) above, we would go "curtis" → new Object[][] {{"curtis"}} → new Double[][] {{ convert("curtis", Double.class) }}.

The remaining question is: what to do if any/all of those leaves are not convertible?

After some discussion between myself, @hinerm, and @gselzer, we narrowed down the following potentially reasonable behavior:

1. If all of the leaf elements are convertible to the destination element type, then the converted object is populated. Otherwise, the whole collection is inconvertible, and we return null for the whole collection, and false for canConvert. This would answer the above list with null for questions 2, 3, 6, 7, 8.

2. If any of the leaf elements are convertible to the destination element type, then the converted object is populated, filling any inconvertible elements with null. This would answer the above list with null for questions 6, 7, 8—but 2 and 3 would work.

3. Regardless of whether any leaf elements are convertible to the destination element type, the converted object is populated, filling nulls as in (2). This would answer the above list with "As all the way down" i.e. no null results.

Part of this issue is deciding which of these three behaviors would be best. Then fixing the DefaultConverter to work that way—in both its canConvert and convert methods—and uncommenting the disabled tests (and adjusting them as needed) to validate it.

[gselzer]

It would be cool if, through solving this PR, we could formalize 1 or 2 or 3 as the intent of the ConvertService, although I admit that may be infeasible for SciJava Common if any consensus would be a breaking change.

[imagesc-bot]

This issue has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/change-of-behaviour-in-scijava-parameter-table-of-size-one-with-null-object-inside-instead-of-null-object/83625/3