Issue1285

classification
Title: [2.5b3] respectJavaAccessibility = false makes some Java class method signatures inaccessible from Jython
Type: behaviour Severity: normal
Components: Core Versions: 2.5b1
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: cgroves Nosy List: cgroves, fwierzbicki, kfitch42, marcdownie, zyasoft
Priority: high Keywords: patch

Created on 2009-03-23.03:02:45 by marcdownie, last changed 2009-04-27.05:07:57 by cgroves.

Files
File name Uploaded Description Edit Remove
fix-for-accessibility-bug.patch kfitch42, 2009-04-23.03:37:03
fix-for-accessibility-bug-redo.patch kfitch42, 2009-04-24.02:37:19
test-for-javaAccessibility.tar.gz kfitch42, 2009-04-24.02:38:46
Messages
msg4337 (view) Author: Marc Downie (marcdownie) Date: 2009-03-23.03:02:44
A simple java class hierarchy: 

package somepackage;

public class Banana {
	public void amethod() {}

	public void amethod(int x, int y) {}
}

---
and:

package somepackage;

public class Pear extends Banana {
	public void amethod(int x, int y)
	{
	}		
}

---
Inside Jython as it ships:

Jython 2.5b3 (Release_2_5beta3:6092, Mar 10 2009, 15:34:57) 
[Java HotSpot(TM) Client VM (Apple Inc.)] on java1.5.0_16
Type "help", "copyright", "credits" or "license" for more information.
>>> from somepackage import *
>>> x = Pear()
>>> x.amethod()

All ok.

with python.security.respectJavaAccessibility = false

Jython 2.5b3 (Release_2_5beta3:6092, Mar 10 2009, 15:34:57) 
[Java HotSpot(TM) Client VM (Apple Inc.)] on java1.5.0_16
Type "help", "copyright", "credits" or "license" for more information.
>>> from somepackage import *
>>> x = Pear()
>>> x.amethod()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: amethod(): expected 2 args; got 0

It's broken at least this far:

>>> print x.amethod.argslist
array(org.python.core.ReflectedArgs, [class somepackage.Pear, false, 0, public void 
somepackage.Pear.amethod(int,int)
	(int, int, )])

----

I can't say for sure how long this behavior has been in place (at least as far back as beta1, but maybe 
I've gotten lucky for a lot longer than that). Since every method that's implicated here is public I 
find it very odd (and very inconvenient) that respectJavaAccessibility = false appears to reduce the 
visibility of the method. I'm stepping through PyJavaType#init but I haven't found anything obviously 
wrong yet.
msg4521 (view) Author: Kevin Fitch (kfitch42) Date: 2009-04-16.17:34:59
I think I know where the issue is.
PyJavaType.java line 232
When respectJavaAccessibility==False it uses getDeclaredMethods, which
the java docs say does not find inherited methods.
When respectJavaAccessibility==True it uses getMethods, which does find
inherited methods (but only public ones) ... so we need to walk up the
heirarchy when respectJavaAccessibility==False (possibly making use of
the mro that was just computed) and collect ALL the methods.
msg4526 (view) Author: Marc Downie (marcdownie) Date: 2009-04-17.15:47:05
I fear it's more complex than this. 

If I change my example to:

package somepackage;

public class Banana {
       public void amethod() {}

       public void amethod(int x, int y) {}

       // added this method
       public void bmethod(){}
}

---
and:

package somepackage;

public class Pear extends Banana {
       public void amethod(int x, int y)
       {
       }
}

---

then Pear().bmethod() works as expected (even though 
Pear.class.getDeclaredMethods() will not contain a bmethod). Had it not 
worked I would have seen this bug immediately (I have a lot of code that 
runs with respectJavaAccessibility=false).
msg4575 (view) Author: Kevin Fitch (kfitch42) Date: 2009-04-23.03:37:03
Well, even though you said it wouldn't work I tried tweaking things 
with respect to getMethods and getDeclaredMethods. And, I got something 
that seems to resolve the bug. patch attached.

Basically I turned the ordering around. I first make all the methods 
public, then call getMethods in both cases. I have checked that it 
makes the given example work, nothing else.
msg4577 (view) Author: Marc Downie (marcdownie) Date: 2009-04-23.04:15:32
I'm afraid I don't understand your patch at all --- it seems to remove the 
else clause for !respectJavaAccessiblity (which is the clause that is 
suspect) completely, and the last line makes the one remaining if clause 
irrelevant as far as I can tell.

With your patch applied on r6255 respectJavaAccessibility = false behaves 
just like respectJavaAccessibility = true and we can't access non-public 
methods. Yes, this makes this particular bug here go away, but ...

If this bug is still active by then, I'll be able to take a close look at 
it this weekend.
msg4582 (view) Author: Kevin Fitch (kfitch42) Date: 2009-04-23.10:45:58
Doh, that will teach me to write code that late at night.

Yeah, the logic in the conditional was backward from my intent, and it
just didn't work. My hope was that when respectJavaAccessibility=false 
I could call setAccessible(true) on every method in the hierarchy, then
getMethods would treat them as public, so that we could use getMethods
in both cases. Turns out I this hope had nothing to do with reality :(

I would like to retract the patch then.
msg4585 (view) Author: Kevin Fitch (kfitch42) Date: 2009-04-24.02:37:19
OK, I really fixed it this time (well mostly ... my new patch almost 
definitely needs some tweaks to be more robust). This time I actualy 
implemented a real test case that tests both sides of the issue.
msg4586 (view) Author: Kevin Fitch (kfitch42) Date: 2009-04-24.02:38:46
And here is my test case. Unzip in a sibling directory to where the 
jython code is checked out and the included shell script should work 
for you, if not it should be pretty trivial to fix up.
msg4600 (view) Author: Charlie Groves (cgroves) Date: 2009-04-27.05:07:41
I've committed a fix for this in r6266.  Rather than trying to build up a set of method signatures just for the 
respectJavaAccessibility portion of the code, the commit grabs all the methods, stuffs them into a list, and lets 
PyReflectedFunction sort out the argument conflicts.

Both the patch and the committed version will lead to a circumstance where if a superclass declares a private or 
package protected method with more restrictive types than a subclass, the subclass method won't be callable from Jython 
with those types.  The alternative is doing more type analysis for respectJavaAccessibility and not including 
superclass methods if they're more specific than subclass methods, but that means the superclass method isn't directly 
accessible on the subclass.  I believe this is how respectJavaAccessibility has always worked, so I'm going this way to  
keep from rocking the boat.

In any case, I advise getting away from respectJavaAccessibility if possible.  Its effects on code can be non-obvious, 
it's far less solid than the regular Java integration, and it may go away in a future version.
History
Date User Action Args
2009-04-27 05:07:58cgrovessetstatus: open -> closed
resolution: fixed
messages: + msg4600
2009-04-24 14:21:34cgrovessetassignee: cgroves
2009-04-24 02:38:46kfitch42setfiles: + test-for-javaAccessibility.tar.gz
messages: + msg4586
2009-04-24 02:37:20kfitch42setfiles: + fix-for-accessibility-bug-redo.patch
messages: + msg4585
2009-04-23 10:45:58kfitch42setmessages: + msg4582
2009-04-23 04:15:33marcdowniesetmessages: + msg4577
2009-04-23 03:37:04kfitch42setfiles: + fix-for-accessibility-bug.patch
keywords: + patch
messages: + msg4575
2009-04-17 15:47:05marcdowniesetmessages: + msg4526
2009-04-16 17:34:59kfitch42setnosy: + kfitch42
messages: + msg4521
2009-04-04 02:08:37zyasoftsetnosy: + cgroves, fwierzbicki, zyasoft
2009-04-04 02:07:12zyasoftsetpriority: high
2009-03-23 03:02:45marcdowniecreate