Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 349023

Summary:

provide support for Gerrit 2.2

Product:

z_Archived

Reporter:

Martin Long <martin>

Component:

Mylyn

Assignee:

Steffen Pingel <steffen.pingel>

Status:

RESOLVED FIXED

QA Contact:

Severity:

enhancement

Priority:

CC:

alex.blewitt, b.muskalla, christian.halstrick, greensopinion, jeffl8n, patnotz, steffen.pingel

Version:

0.8

Keywords:

noteworthy, plan

Target Milestone:

0.9

Hardware:

All

OS:

All

See Also:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=347405

Whiteboard:

Bug Depends on:

357322

Bug Blocks:

Attachments:

Description	Flags
mylyn/context/zip	none

Description Martin Long

2011-06-10 09:18:14 EDT

Build Identifier: 20110301-1815

The code in getPatchSetDetail(final PatchSet.Id id, IProgressMonitor monitor) assumes that if an exception is thrown from getChangeDetailService().patchSetDetail(id, this); that this must mean that the Gerrit version is < 2.1.7. It then goes on to try the fallback of getChangeDetailService().patchSetDetail(id, this);

For me, the second call throws an exception, because I'm trying to make the connector work with Gerrit 2.2.1. I'm clearly getting an "Error parsing request", but I just get exactly what I would expect from this code (given I'm using the compiled jars from 2.2.1):

 java.lang.NoSuchMethodError: org.eclipse.mylyn.internal.gerrit.core.client.compat.ChangeDetailService.patchSetDetail(Lcom/google/gerrit/reviewdb/PatchSet$Id;Lcom/google/gwt/user/client/rpc/AsyncCallback;)V I

Because there is no further catch, this exception is propogated, and so I've lost the original exception that happened on: patchSetDetail(id, null, null, this).

I'm not sure what the solution is. Maybe catch NoSuchMethodError and throw the original exception (e) instead.

Reproducible: Always

Steps to Reproduce:
Details of code examination given. 

Otherwise: run against Gerrit 2.2.1, using the compiled prettify, common and reviewdb jars from 2.2.1

Comment 1 Steffen Pingel

2011-06-10 09:42:10 EDT

Sascha, do you have an idea how to improve the error handling? Would be nice to make the connector work with 2.2.1 as well :).

Comment 2 Sascha Scholz

2011-06-10 15:29:28 EDT

If I understood you correctly you're trying to get the connector working with Gerrit 2.2.1 and also compile against the Gerrit 2.2.1 jars. Is there any reason why you do the latter, doesn't it work with the 2.1.5 jars? Would be nice if you could share your plans with us. Anything you would like to contribute?

Currently, the connector doesn't rely on functionality from Gerrit > 2.1.5. The Gerrit people know about our "misuse" of the JSON-RPC communication layer (it's actually not an official API) and try to avoid incompatible changes in the future.

Actually, if we changed our policy to support only 2.2.1 or greater we could remove the compatibility interface from our coding again. But I wouldn't like do that unless there's a good reason.

Comment 3 Martin Long

2011-06-10 16:31:14 EDT

Thanks for the quick response.

The reason I tried to compile against the 2.2.1 jars is that there has been a change between 2.1.7 and 2.2.1 in the JSON "API". In PatchSetPublishDetail, "given" has been changed from a Map to a List (plus some other changes). This causes a parse error when parsing into the objects in the 2.1.5 libraries.

I don't have a huge amount of time to spend working on this, but I wouldn't hesitate to contribute my work back. I just wanted to get it running on Gerrit 2.2.1.

I can see the difficulties writing against and unofficial, moving API. I did actually notice some of Shawn Pearce's commit comments referred to maintaining backwards compatibility for you guys.

I don't really know the details of the compatibility layer you describe - from what I see you've overridden some of the classes, providing overloads of some of the methods. I'm not sure if that works in all cases (I'm not very familiar with JSON rpc), such as the example I give above.

I don't know how you feel about abandoning 2.1.5 support in favour of 2.2.1. 2.2.1 is not yet in "stable", and I'm not sure what Shawn's plans are for release (though I'm using it comfortably, and it seems 'ready'). That said, the Gerrit Connector is still in incubation (in fact, is the whole of Mylyn 3.6 in incubation?), I could see Gerrit 2.2.1 being release before the connector.

I suppose the other option is to branch the connector. I don't know how Eclipse handles branches - would the updater support two concurrent branches? I don't know, just throwing that in the pot.

Anyway, don't want to step on toes. I just though this connector (and the Hudson/Jenkins connector) were really cool, but to my dismay, only of limited use with Gerrit 2.2.1. Serves me right for upgrading too quickly.

Thanks again.

Martin

Comment 4 Steffen Pingel

2011-06-14 12:32:49 EDT

As long as the 2.1.x branch is officially supported by the Gerrit project I would like to maintain backwards compatibility. Still, it would be nice to also support the 2.2.x stream. Do you know if that's possible through overloading additional methods?

Comment 5 Martin Long

2011-06-14 16:06:37 EDT

I personally suspect it can't be done by overloading methods, as it's the deserialisation of a class that is failing. Existing fields in a class have been changed from a Map to a List - the parser baulks at this. 

It might need a branch :(

Comment 6 Steffen Pingel

2011-06-21 06:13:11 EDT

*** Bug 349899 has been marked as a duplicate of this bug. ***

Comment 7 Steffen Pingel

2011-06-21 08:28:26 EDT

Martin, which version were you trying? I can't actually reproduce the error with  http://egit.eclipse.org/r/ which is supposedly "Powered by Gerrit Code Review (2.2.1-67-g17ca55f)".

Comment 8 Martin Long

2011-06-21 09:48:33 EDT

Given the comments above, and the linked report that has been closed, I think it's probably best if I give the details for the original issue, while using the 2.1.5 in Eclipse. 

It looks like eclipse must be on a VERY recent version. Mine is: v2.2.1-1-gec40063 which was committed on 2011-06-08. 

I think 2.2.1-67-g17ca55f must be on an eclipse feature branch, as I can't find it in the google repository (probably branched for branching etc). 

To reproduce:

1. create a review. 
2. open the review in mylyn
3. add yourself as a reviewer, either in Mylyn or Gerrit
4. Save, and resync the review in Mylyn

Result: 

"Unexpected error while communicating with Gerrit"

org.eclipse.mylyn.internal.gerrit.core.client.GerritException
	at org.eclipse.mylyn.internal.gerrit.core.client.GerritClient.execute(GerritClient.java:684)
	at org.eclipse.mylyn.internal.gerrit.core.client.GerritClient.getPatchSetPublishDetail(GerritClient.java:383)
	at org.eclipse.mylyn.internal.gerrit.core.client.GerritClient.getChange(GerritClient.java:400)
	at org.eclipse.mylyn.internal.gerrit.core.GerritTaskDataHandler.getTaskData(GerritTaskDataHandler.java:81)
	at org.eclipse.mylyn.internal.gerrit.core.GerritConnector.getTaskData(GerritConnector.java:123)
	at org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.synchronizeTask(SynchronizeTasksJob.java:245)
	at org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.runInternal(SynchronizeTasksJob.java:218)
	at org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.run(SynchronizeTasksJob.java:153)
	at org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.run(SynchronizeTasksJob.java:129)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54)
Caused by: com.google.gson.JsonParseException: The JsonDeserializer com.google.gwtjsonrpc.server.MapDeserializer@184b826 failed to deserialized json object [{"key":{"patchSetId":{"changeId":{"id":527},"patchSetId":1},"accountId":{"id":1000000},"categoryId":{"id":"CRVW"}},"value":0,"granted":"2011-06-14 14:34:37.930000000","changeOpen":true}] given the type com.google.gson.ParameterizedTypeImpl@1daced8
	at com.google.gson.JsonDeserializerExceptionWrapper.deserialize(JsonDeserializerExceptionWrapper.java:63)
	at com.google.gson.JsonDeserializationVisitor.invokeCustomDeserializer(JsonDeserializationVisitor.java:88)
	at com.google.gson.JsonObjectDeserializationVisitor.visitFieldUsingCustomHandler(JsonObjectDeserializationVisitor.java:116)
	at com.google.gson.ObjectNavigator.navigateClassFields(ObjectNavigator.java:158)
	at com.google.gson.ObjectNavigator.accept(ObjectNavigator.java:131)
	at com.google.gson.JsonDeserializationContextDefault.fromJsonObject(JsonDeserializationContextDefault.java:73)
	at com.google.gson.JsonDeserializationContextDefault.deserialize(JsonDeserializationContextDefault.java:51)
	at com.google.gson.Gson.fromJson(Gson.java:568)
	at org.eclipse.mylyn.internal.gerrit.core.client.JSonSupport.parseResponse(JSonSupport.java:157)
	at org.eclipse.mylyn.internal.gerrit.core.client.GerritService.invoke(GerritService.java:102)
	at $Proxy19.patchSetPublishDetail(Unknown Source)
	at org.eclipse.mylyn.internal.gerrit.core.client.GerritClient$9.execute(GerritClient.java:386)
	at org.eclipse.mylyn.internal.gerrit.core.client.GerritClient.execute(GerritClient.java:678)
	... 9 more
Caused by: java.lang.IndexOutOfBoundsException: Index: 1, Size: 1
	at java.util.ArrayList.RangeCheck(ArrayList.java:547)
	at java.util.ArrayList.get(ArrayList.java:322)
	at com.google.gson.JsonArray.get(JsonArray.java:101)
	at com.google.gwtjsonrpc.server.MapDeserializer.deserialize(MapDeserializer.java:69)
	at com.google.gwtjsonrpc.server.MapDeserializer.deserialize(MapDeserializer.java:33)
	at com.google.gson.JsonDeserializerExceptionWrapper.deserialize(JsonDeserializerExceptionWrapper.java:50)
	... 21 more


To save you trolling the code - this is basically an issue parsing the newer List used in the 2.2.1 version of PatchSetPublishDetail (and consequently in the JSON), into the 2.1.5 implementation, which uses Maps. 

I'm having trouble pulling from the Google repo at work, so I can't see if this has been changed in a more recent version. 

Martin

Comment 9 Martin Long

2011-06-21 10:12:37 EDT

Ok, managed to pull from google. That commit is definitely not in Google's repo. I can't see anything else that would effect it, so it should be reproducable on egit. 

Note that if you are not the reviewer, then it works ok. But you cannot actually complete a review.

Comment 10 Martin Long

2011-06-21 10:28:08 EDT

And I've just connected to http://egit.eclipse.org/r/ and reproduced this. 

Martin

Comment 11 Martin Long

2011-06-21 10:59:26 EDT

And I've just connected to http://egit.eclipse.org/r/ and reproduced this. 

Martin

Comment 12 Steffen Pingel

2011-07-04 05:25:08 EDT

*** Bug 351021 has been marked as a duplicate of this bug. ***

Comment 13 Steffen Pingel

2011-07-11 05:53:55 EDT

*** Bug 351666 has been marked as a duplicate of this bug. ***

Comment 14 Jeff Layton

2011-07-18 13:00:15 EDT

Is there a target completion for this issue?

Comment 15 Steffen Pingel

2011-07-18 14:34:47 EDT

We don't have a target release, yet, but I have the task to the backlog to consider it during planning.

Comment 16 Steffen Pingel

2011-09-09 09:25:00 EDT

I have added work-arounds to support parsing the change permission data fields in Gerrit 2.2: http://review.mylyn.org/#change,26.

Comment 17 Steffen Pingel

2011-09-09 09:25:05 EDT

Created attachment 203056 [details]
mylyn/context/zip

Comment 18 Steffen Pingel

2011-09-11 08:10:23 EDT

The parsing error is unfortunately not yet fixed. Updated stack trace when opening http://egit.eclipse.org/r/#change,2609:

!ENTRY org.eclipse.mylyn.gerrit.core 4 0 2011-09-11 14:07:35.829
!MESSAGE Unexpected error while communicating with Gerrit
!STACK 0
org.eclipse.mylyn.internal.gerrit.core.client.GerritException
	at org.eclipse.mylyn.internal.gerrit.core.client.GerritClient.execute(GerritClient.java:770)
	at org.eclipse.mylyn.internal.gerrit.core.client.GerritClient.getPatchSetPublishDetail(GerritClient.java:442)
	at org.eclipse.mylyn.internal.gerrit.core.client.GerritClient.getChange(GerritClient.java:473)
	at org.eclipse.mylyn.internal.gerrit.core.GerritTaskDataHandler.getTaskData(GerritTaskDataHandler.java:90)
	at org.eclipse.mylyn.internal.gerrit.core.GerritConnector.getTaskData(GerritConnector.java:126)
	at org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.synchronizeTask(SynchronizeTasksJob.java:245)
	at org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.runInternal(SynchronizeTasksJob.java:218)
	at org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.run(SynchronizeTasksJob.java:153)
	at org.eclipse.mylyn.internal.tasks.core.sync.SynchronizeTasksJob.run(SynchronizeTasksJob.java:129)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54)
Caused by: com.google.gson.JsonParseException: The JsonDeserializer com.google.gwtjsonrpc.server.MapDeserializer@56636a77 failed to deserialized json object [{"key":{"patchSetId":{"changeId":{"id":2609},"patchSetId":6},"accountId":{"id":83},"categoryId":{"id":"IPCL"}},"value":0,"granted":"2011-09-09 19:21:16.000000000","changeOpen":true}] given the type com.google.gson.ParameterizedTypeImpl@a0741aba
	at com.google.gson.JsonDeserializerExceptionWrapper.deserialize(JsonDeserializerExceptionWrapper.java:63)
	at com.google.gson.JsonDeserializationVisitor.invokeCustomDeserializer(JsonDeserializationVisitor.java:88)
	at com.google.gson.JsonObjectDeserializationVisitor.visitFieldUsingCustomHandler(JsonObjectDeserializationVisitor.java:116)
	at com.google.gson.ObjectNavigator.navigateClassFields(ObjectNavigator.java:158)
	at com.google.gson.ObjectNavigator.accept(ObjectNavigator.java:131)
	at com.google.gson.JsonDeserializationContextDefault.fromJsonObject(JsonDeserializationContextDefault.java:73)
	at com.google.gson.JsonDeserializationContextDefault.deserialize(JsonDeserializationContextDefault.java:51)
	at com.google.gson.Gson.fromJson(Gson.java:568)
	at org.eclipse.mylyn.internal.gerrit.core.client.JSonSupport.parseResponse(JSonSupport.java:176)
	at org.eclipse.mylyn.internal.gerrit.core.client.GerritService.invoke(GerritService.java:106)
	at $Proxy5.patchSetPublishDetailX(Unknown Source)
	at org.eclipse.mylyn.internal.gerrit.core.client.GerritClient$11.execute(GerritClient.java:446)
	at org.eclipse.mylyn.internal.gerrit.core.client.GerritClient.execute(GerritClient.java:764)
	... 9 more
Caused by: java.lang.IndexOutOfBoundsException: Index: 1, Size: 1
	at java.util.ArrayList.RangeCheck(ArrayList.java:547)
	at java.util.ArrayList.get(ArrayList.java:322)
	at com.google.gson.JsonArray.get(JsonArray.java:101)
	at com.google.gwtjsonrpc.server.MapDeserializer.deserialize(MapDeserializer.java:69)
	at com.google.gwtjsonrpc.server.MapDeserializer.deserialize(MapDeserializer.java:33)
	at com.google.gson.JsonDeserializerExceptionWrapper.deserialize(JsonDeserializerExceptionWrapper.java:50)
	... 21 more

Comment 19 Steffen Pingel

2011-09-12 07:48:12 EDT

Pushed 862a05dd760c9dc27d1231824ea00a8b5085caac. The connector now registers a custom deserializer to handle the changes in the JSon stream.

Comment 20 Steffen Pingel

2011-10-20 05:37:27 EDT

*** Bug 361485 has been marked as a duplicate of this bug. ***

Comment 21 Steffen Pingel

2011-10-20 05:38:53 EDT

*** Bug 361486 has been marked as a duplicate of this bug. ***