batched friends_get in the json client returns nothing (with suggested fix!)

[GoogleCodeExporter]

What steps will reproduce the problem?
1.  call friends_get() thru the json client
2.  get the results

What is the expected output? What do you see instead?
-i should see the list of publicly available friends.
-what i see is an empty list.  inspection reveals the return type of the 
method is "void"

fix:
whats happening is you are overriding friends_get in the json client to 
return JSONArray instead of <T> (which will bind to Object).  but you are 
not accounting for this return type in the static block up at the top when 
you fill out the RETURN_TYPE map.  all i did was add a condition where if 
the type of the method candidate is jsonarray, then make the return type 
"default".  im not really sure if this is a hacky fix, but you guys will 
probably be able to come up with the real fix.

thanks for all the hard work, btw!  client is great!

Original issue reported on code.google.com by thien.p....@gmail.com on 14 Nov 2008 at 8:47

[GoogleCodeExporter]

I've added a JUnit test for this along with my testing framework in Issue 141. I
can't get batching to work at all; is the problem really restricted to the
friends_get method? The method documentation says that once client.beginBatch() 
is
called, all methods should return null and be queued up until executeBatch() is 
run.
That's not the behaviour I'm seeing from the client.

Original comment by david.j....@googlemail.com on 16 Nov 2008 at 7:37

[GoogleCodeExporter]

so.. i can run the test, but the batch returns null for some reason..

please continue to explore.

Original comment by fern...@gmail.com on 18 Nov 2008 at 3:19

• Changed state: Accepted

[GoogleCodeExporter]

just putting to high.. because it breaks tests.. and we just need to figure out 
if
batchRuns are broken, or friends_get do not work with batches..

Original comment by fern...@gmail.com on 18 Nov 2008 at 3:32

• Added labels: Priority-High
• Removed labels: Priority-Medium

[GoogleCodeExporter]

so, it is true that friends_get shouldn't return anything when its called 
within a 
batch begin/end block.  what im talking about is the piece of code within 
FacebookJSONRestClient:executeBatch method, theres this line that gets the 
return 
type of the method:

String type = RETURN_TYPES.get( buffer.get( count ).getMethod() );

the type that gets returned for friends_get is "void" when it should be 
"default", in 
which case it should get dealt with in the IF clause that handles it as a 
JSONArray.  

i traced the static block at the top that handles filling that RETURN_TYPES 
map, and 
thats where i believe the bug is, at least from my basic knowledge of the code. 
 in 
that code, theres some type of java reflection code that inspects what the 
return 
type of the method is, but it doesn't test for "JSONArray", which is the return 
type 
of the friends_get() method thats overridden in FacebookJSONRestClient.  

hope that makes sense

Original comment by thien.p....@gmail.com on 18 Nov 2008 at 6:05

[GoogleCodeExporter]

The JUnit test is now passing as expected. The client returns null for the 
method
calls *during* the batch, and returns the correct results (twice) when the
executeBatch method is called.

Marking this issue as fixed.

Original comment by david.j....@googlemail.com on 18 Nov 2008 at 11:06

• Changed state: Fixed

[GoogleCodeExporter]

wait, it's failing for me.

still returning null after batchRun..

any ideas?

did you forget to check something in?

Original comment by fern...@gmail.com on 19 Nov 2008 at 7:06

• Changed state: Accepted

[GoogleCodeExporter]

Something very strange going on which I'll investigate further. The test is 
working
for me when I *run* it, but wasn't working when I used Eclipse to *debug* it. 
There
must be some other thing going on which I'll check. In the meantime, here's the 
patch
I used to get it working in Debug mode:

### Eclipse Workspace Patch 1.0
#P facebook-api
Index:
facebook-java-api/src/main/java/com/google/code/facebookapi/FacebookJsonRestClie
nt.java
===================================================================
---
facebook-java-api/src/main/java/com/google/code/facebookapi/FacebookJsonRestClie
nt.java
(revision 410)
+++
facebook-java-api/src/main/java/com/google/code/facebookapi/FacebookJsonRestClie
nt.java
(working copy)
@@ -58,7 +58,10 @@
                            || ( typeName.indexOf( "map" ) != -1 ) || ( typeName.indexOf( "object" ) !=
-1 ) ) {
                        // we don't autobox these for now, the user can parse them on their own
                        RETURN_TYPES.put( method, "default" );
-                   } else {
+                   } else if ((typeName.indexOf("jsonarray") != -1)) {
+                       RETURN_TYPES.put( method, "default" );
+                   }
+                   else {
                        RETURN_TYPES.put( method, "void" );
                    }
                    break;

Original comment by david.j....@googlemail.com on 20 Nov 2008 at 10:18

[GoogleCodeExporter]

see, that's weird because that change is not committed in the repository.  i 
have svn
up and don't see that change, that is why it fails for me! :(

remember when I said that your old working directory was read-only and you had 
to
check it out again with commit privlieges?  Maybe your command-line and your 
eclipse
are using different working directories??

you might have to double check everything outside and inside your eclipse 
working
directory to make sure this isn't the case.

Original comment by fern...@gmail.com on 21 Nov 2008 at 6:09

[GoogleCodeExporter]

Right, I haven't committed that change while I investigate why I see the 
different
behaviour in the Run and Debug environments. As I'm sure you know, the quickest 
way
to just get this bug fixed is to check in the change I suggested. Feel free to 
check
it in if you are happy with the patch.

Cheers,
Dave

Original comment by david.j....@googlemail.com on 21 Nov 2008 at 8:35

[GoogleCodeExporter]

OK, I've finally worked out why it's behaving differently in Run and Debug and
believe it or not it's a bug, not some setup problem.

This line in FacebookJsonRestClient fetches the list of methods defined on
FacebookJsonRestClient and its superclasses:

Method[] candidates = FacebookJsonRestClient.class.getMethods();

There are two versions of friends_get() present in this array when Java returns 
it.
One is declared by:
FacebookJsonRestClient - public ****JSONArray**** friends_get()
The other is declared by superclass:
ExtensibleClient - public T friends_get()
which actually gets reported in the array as
ExtensibleClient - public ****Object**** friends_get()

Now, there is a loop within a loop in the static {} block at the top of
FacebookJsonRestClient. The outer loop iterates through all of the 
FacebookMethod
values. The inner loop tries to find a matching candidate Java Method. When it 
finds
one, it break;s the inner loop and goes onto the next FacebookMethod.

So, when there are 2 corresponding Methods in the Method[] candidates array, 
which
one wins? The first one. And there's the rub. Class.getMethods() offers no 
guaranteed
ordering (nor should it really) for the methods.
From the Javadoc [The elements in the array returned are not sorted and are not 
in
any particular order.]

I've found that in my Java 1.5 environment, the Class.getMethods() call is 
returning
the same list but in a different order depending on whether I'm using Run or 
Debug.

So, mystery solved and hopefully this points us towards the correct fix. The 
first
thing to do is to make the code *always* match the
FacebookJsonRestClient.friends_get() method. This is the behaviour you'd expect.
Then, we probably want to apply the patch suggested in Comment 7 anyway. But 
without
fixing this underlying problem of having multiple methods, we run the risk of 
having
unpredictable code.

Original comment by david.j....@googlemail.com on 21 Nov 2008 at 3:48

[GoogleCodeExporter]

wow. ok, so yes, please check in that change, it looks to make sense anyhow.  
and
we'll think about what that method is doing, maybe it's just too complicated 
for its
own good..

also, the project has a standard eclipse formatting checked in. so feel free to 
run
Ctrl-Shift-F anytime to have eclipse format code.. that's what I do all the 
time. :)

Original comment by fern...@gmail.com on 21 Nov 2008 at 4:33

[GoogleCodeExporter]

yup. checked it in.  unit test passes now! :)

thank you for setting the unit tests!!!!!

Original comment by fern...@gmail.com on 22 Nov 2008 at 2:40

• Changed state: Fixed

[GoogleCodeExporter]

Reopened this issue. I've added a JUnit to ensure that this works for JAXB as 
well.
Unfortunately it doesn't at this stage :o(

Looks like the JAXB code doesn't know anything about the "batch_run_response" 
element.

Original comment by david.j....@googlemail.com on 24 Nov 2008 at 9:12

• Changed state: Accepted

[GoogleCodeExporter]

I'm considering raising a Facebook bug about this one. The batch_run_response 
isn't
mentioned in:

http://api.facebook.com/1.0/facebook.xsd

Original comment by david.j....@googlemail.com on 2 Dec 2008 at 12:35

[GoogleCodeExporter]

Adding this to schema.xsd makes it work, but I'm not going to check this in as 
it's a
bodge not a fix:

<xsd:element name="batch_run_response">
  <xsd:complexType>
    <xsd:sequence minOccurs="1" maxOccurs="unbounded">
      <xsd:element name="friends_get_response">
        <xsd:complexType>
          <xsd:sequence minOccurs="0" maxOccurs="unbounded">
            <xsd:element name="uid" type="uid" minOccurs="0" maxOccurs="unbounded" />
          </xsd:sequence>
          <xsd:attribute name="list" type="xsd:boolean" />
        </xsd:complexType>
    </xsd:element>
    </xsd:sequence>
  </xsd:complexType>
</xsd:element>

I think we need to wait for facebook.xsd to be reworked to include the batching 
API
which, after all, is Beta. I'm going to mark the JUnit test for JAXB as @Ignore 
and
wait for facebook.xsd to catch up. The original issue for this thread is 
resolved, so
marking it so.

Original comment by david.j....@googlemail.com on 2 Dec 2008 at 12:57

• Changed state: Fixed

[GoogleCodeExporter]

I'm going to reopen this and add this into the schema.xsd.  We are already
maintaining our own schema.xsd with lots of changes, what's one more :)

Original comment by fern...@gmail.com on 2 Dec 2008 at 2:04

• Changed state: Accepted

[GoogleCodeExporter]

there.  applied the changes to the xsd.

(and facebook.xsd.diff is tracking the changes that we are making to the 
'official'
yet broken facebook.xsd, just to be anal)

Original comment by fern...@gmail.com on 2 Dec 2008 at 3:53

• Changed state: Fixed

[GoogleCodeExporter]

not sure what's going on, but the unit test keeps failing for me...

Original comment by fern...@gmail.com on 27 Dec 2008 at 11:59

• Changed state: Accepted

Attachments:

• com.google.code.facebookapi.Issue140FriendsGetJSONTest.txt

[GoogleCodeExporter]

Original comment by fern...@gmail.com on 28 Dec 2008 at 12:01

[GoogleCodeExporter]

Unit test passing for me on HEAD.

Original comment by david.j....@googlemail.com on 28 Dec 2008 at 1:23

[GoogleCodeExporter]

Original comment by david.j....@googlemail.com on 13 Jan 2009 at 11:34

• Changed state: Fixed