Issue2391

[DESCRIPTION]

Consider this Java interface:

package attributeTest;
public interface NumberHolder {
    Number getNumber();
}

And this class implementing it:

package attributeTest;
public class DoubleHolder implements NumberHolder {
    private Double number = 0.0;
    @Override public Double getNumber() {
        return number;
    }
    public void setNumber(Double number) {
        this.number = number;
    }
}

Now try to use it from Jython:

> from attributeTest import DoubleHolder
> d = DoubleHolder()
> print d.number
0.0
> d.number = 3.0
<type 'exceptions.AttributeError'>: read-only attr: number

The reported error is wrong, as the DoubleHolder class has actually a setNumber method matching the attribute.


[ANALYSIS]

Digging into the code (Jython 2.5 codebase), this comes from the init method of org.python.core.PyJavaType.

When building the bean properties map, all DoubleHolder's methods are got by means of java.lang.Class.getMethods (line 273).

This returns all methods for the type and its parent classes and interfaces. Thus it gets:

[NumberHolder] Number getNumber()
[DoubleHolder] Double getNumber()
[DoubleHolder] void setNumber(Double)

Class.getMethods does not guarantee any order. What happens is that the getNumber() method of DoubleHolder is processed, then getNumber() of NumberHolder, which overwrites the former, then setNumber().

There is a subsequent check on org.python.core.PyJavaType.init() (lines 488-490) where the getMethod is expected to return the type of the setMethod's argument.

As this is not the case (getNumber returns Number because the interface's method overwrote the one in the class, but setNumber receives a Double), then prop.setMethod is set to null. Which provokes later the mentioned error

<type 'exceptions.AttributeError'>: read-only attr: number


[SOLUTION]

A solution is to sort the props map so that the methods belonging to the class are processed after the methods belonging to super classes or interfaces.

This could be implemented by means of a TreeMap with a comparator ensuring the desired order.

Aside note: this ticket might be related with bug #1610.

Platform details:

* Java 1.8.0_60
* Jython 2.5.2
* Linux Red Hat 6.7

Maybe a problem specifically with 2.5.2?

$ jython27
Jython 2.7.0 (default:9987c746f838, Apr 29 2015, 02:25:11)
[Java HotSpot(TM) 64-Bit Server VM (Oracle Corporation)] on java1.8.0_45
Type "help", "copyright", "credits" or "license" for more information.
>>> from attributeTest import DoubleHolder
>>> d = DoubleHolder()
>>> d.number
0.0
>>> d.number = 3.0
>>> d.number
3.0

$ jython25
Jython 2.5.4 (2.5:5ce837b1a1d8+, Dec 30 2014, 09:01:23)
[Java HotSpot(TM) 64-Bit Server VM (Oracle Corporation)] on java1.8.0_45
Type "help", "copyright", "credits" or "license" for more information.
>>> from attributeTest import DoubleHolder
>>> d = DoubleHolder()
>>> d.number
0.0
>>> d.number = 3.0
>>> d.number
3.0

Thanks for the prompt reply.

In fact, it works OK with the jython command with version 2.5.2 as well:

$ jython
Jython 2.5.2 (Release_2_5_2:7206, Mar 2 2011, 23:12:06) 
[Java HotSpot(TM) 64-Bit Server VM (Oracle Corporation)] on java1.8.0_60
Type "help", "copyright", "credits" or "license" for more information.
>>> from attributeTest import DoubleHolder
>>> d = DoubleHolder()
>>> print d.number
0.0
>>> d.number = 3.0
>>> print d.number
3.0

However, we use Jython in an interactive GUI built in Java. The PythonInterpreter that we instantiate there is giving this error.

As the order of the methods is not guaranteed, it might sometimes work but do not work other times. The map mentioned in the analysis section should be sorted as explained there.

CORRECTION: In the proposed solution, what needs to be sorted is the methods array (not the props map). By means of Arrays.sort.

Inserting this in the line 292 of PyJavaType.java solves it:

// === START PATCH FOR BUG 2391 ===

Comparator<Method> methodComparator = new Comparator<Method>() {
    @Override public int compare(Method m1, Method m2) {
        Class<?> c1 = m1.getClass();
        Class<?> c2 = m2.getClass();
        return c1.isAssignableFrom(c2)? -1 : c2.isAssignableFrom(c1)? 1 : 0;
    }
};
Arrays.sort(methods, methodComparator);

// ==== END PATCH FOR BUG 2391 ====

The previous comparator is wrong. This one apparently solves it:

// === START PATCH FOR BUG 2391 ===

Comparator<Class<?>> classComparator = new Comparator<Class<?>>() {
    @Override public int compare(Class<?> c1, Class<?> c2) {
        if (c1 != c2) {
            if (c1.isAssignableFrom(c2)) { return -1; }
            if (c2.isAssignableFrom(c1)) { return  1; }
        }
        return 0;
    }
};
Comparator<Method> methodComparator = new Comparator<Method>() {
    @Override public int compare(Method m1, Method m2) {
        int nameComparison = m1.getName().compareTo(m2.getName());
        if (nameComparison != 0) { return nameComparison; }
        int typeComparison = classComparator.compare(m1.getDeclaringClass(), m2.getDeclaringClass());
        if (typeComparison != 0) { return typeComparison; }
        int returnComparison = classComparator.compare(m1.getReturnType(), m2.getReturnType());
        if (returnComparison != 0) { return returnComparison; }
        return 0;
    }
};
Arrays.sort(methods, methodComparator);

// ==== END PATCH FOR BUG 2391 ====

The following code also considers overloaded setter methods:

// === START PATCH FOR BUG 2391 ===

Comparator<Class<?>> classComparator = new Comparator<Class<?>>() {
    @Override public int compare(Class<?> c1, Class<?> c2) {
        if (c1 != c2) {
            if (c1.isAssignableFrom(c2)) { return -1; }
            if (c2.isAssignableFrom(c1)) { return  1; }
        }
        return 0;
    }
};
Comparator<Method> methodComparator = new Comparator<Method>() {
    @Override public int compare(Method m1, Method m2) {
        int result = m1.getName().compareTo(m2.getName());
        if (result != 0) {
            return result;
        }
        Parameter[] p1 = m1.getParameters();
        Parameter[] p2 = m2.getParameters();
        int n1 = p1.length;
        int n2 = p2.length;
        result = n1 - n2;
        if (result != 0) {
            return result;
        }
        result = classComparator.compare(m1.getDeclaringClass(), m2.getDeclaringClass());
        if (result != 0) {
            return result;
        }
        if (n1 == 0) {
            result = classComparator.compare(m1.getReturnType(), m2.getReturnType());
        } else if (n1 == 1) {
            result = classComparator.compare(p1[0].getType(), p2[0].getType());
        }
        return result;
    }
};
Arrays.sort(methods, methodComparator);

// ==== END PATCH FOR BUG 2391 ====

Attached file that solves the issue for us.

To complete this fix, we will need the following:

* unit tests - possibly tricky because of ordering, but doing this repeatedly is generally fine for flushing out nondeterminism issues
* PSF contributor agreement - https://www.python.org/psf/contrib/contrib-form/
* preferred email for the commit

Thanks for your bug report and work on this issue!

Attached a jar file with classes for testing this issue.

With the old implementation, it fails about half of the times.

With the patch, it systematically succeeds.

Feel free to modify it, including changing the package name.

I've signed the PSF. Preferred email for the commit: jsaiz@sciops.esa.int

Thanks.

New version of the patch that solves random exception in comparator for large hierarchies of unrelated classes.

Need to review and get in. Still can make 2.7.1, otherwise 2.7.2 for sure.

Also see #2445

Committed in https://github.com/jythontools/jython/commit/504fb0e605303fb1da06236a441589cac3124e2b

I've made a small change to hierarchyName to use stack instead of inserting into strigbuilder at position 0 as it's O(n^2). Otherwise seems to be passing all tests.

Thank you for the contribution Jaime!