Bug 294774

Comment 1 Scott Lewis 2009-11-10 13:30:55 EST

Adding Holger, setting target milestone to 3.2 and setting to major importance.

Comment 2 Scott Lewis 2009-11-10 13:47:33 EST

My initial thoughts on simplification.  I would first like to focus on IRestCall, since that is the core interface for specifying a rest remote call on a client.

1) I suggest that IRestCall be made to extend IRemoteCall.  The three methods on IRemoteCall are currently duplicated on IRestCall (getMethod, getTimeout, getParameters), and making IRestCall extend IRemoteCall will open up some useful possibilities for the IRemoteService methods that take IRemoteCall instances.

2) Would it be possible to remove IRestCall.getRequestEntity() method?  The documentation for the method says that it is mutually exclusive with using parameters.  Is getRequestEntity present to support a specific REST post method use case? 

3) The IRestCall.getURI() method.  Perhaps this method should be modified in a couple of ways:
   a) It could return a String rather than URI.  I see by the implementation in RestService that this implementation simply converts the URI to a String in any event...so it seems that the URI type is not really needed.
   b) The method name could be changed from getURI() to (e.g.) getPath().  The reason for this is that it might be better to allow the rest container to remain responsible for the target protocol, hostname and port, and specify that the IRestCall.getPath() will only need to specify the path of the remote resource to access.  In most of the test cases, the URI is actually just a path anyway (e.g. "/statuses/user_timeline.xml".  It would then become unnecessary for a URI to be created.

More to come.  Thanks Holger.

Comment 3 Scott Lewis 2009-11-10 14:03:19 EST

Some more thoughts/ideas:

For IRestResourceRepresentationFactory:

The current signature is:

	public abstract Object createResourceRepresentation(HttpMethod method, IRestCall restCall) throws ParseException,
			IOException;


I suggest that the signature be changed to:

public Object createResourceRepresentation(String resourceIdentifier, String resourceContent) throws IOException;

The reason for this is that the only information needed is the resourceIdentifier (better name possible? e.g. resourceMarshallerId?), and a String representation of the resource content itself (after previously being retrieved as response body).

This, along with the removal of RequestEntity will remove dependence specificall on the Apache HttpClient 3.1 implementation...which would be desirable (sometime in future it might be useful to allow/support alternative http impls).  Actually, one idea here might be to use the ECF file transfer API rather than httpclient directly, so as to allow arbitrary file transfer methods to be used.  But in any event, it would be good to eliminate direct class-level dependencies on Apache httpclient in the API.

Comment 4 Scott Lewis 2009-11-16 19:41:40 EST

Created attachment 152360 [details]
zip with new versions of bundles:  org.eclipse.ecf.remoteservice.rest and org.eclipse.ecf.tests.remoteservice.rest

This attachment is a zip file with new versions of two bundles:

org.eclipse.ecf.remoteservice.rest 
org.eclipse.ecf.tests.remoteservice.rest

The changes to these two bundles were large, and so I've added this as a zip rather than a patch.

The changes are many, but the highlights

IRestCall extends IRemoteService

IRestCall and IRestCallable are responsible for the actual call with the following approach...the service provider is expected to create IRestCallable instances to associate a fully qualified method name (i.e. RestCallable(fqMethod param) with the information needed to actually complete a rest call...i.e. the IRestCallable.getResourcePath(), IRestCallable.getRequestType, IRestCallable.getDefaultTimeout, etc).  When an IRestCall is provided to a remote service, the method name (i.e. IRemoteCall.getMethod()) is used to *lookup* the associated IRestCallable instance, and then the parameters given by IRemoteCall.getParameters() along with the information defined in IRestCallable are used to actually make the rest call.

This makes much simpler the data needed from the client to make a rest call, and puts more of the burden of specifying the rest information on the provider...as the provider has to specify the IRestCallable.

See particularly the following tests as examples:

RestRemoteServiceTest
TwitterRemoteServiceTest

Comment 5 Scott Lewis 2009-11-17 11:58:26 EST

Note that for the attachment 152360 [details], that two new dependent projects are needed:

(in /cvsroot/rt)

org.eclipse.ecf/tests/bundles/org.eclipse.ecf.tests
org.eclipse.ecf/incubation/bundles/org.json

These dependencies were added to the test code (org.eclipse.ecf.tests.remoteservice.rest)

Comment 6 Scott Lewis 2009-11-17 17:38:09 EST

Comment on attachment 152360 [details]
zip with new versions of bundles:  org.eclipse.ecf.remoteservice.rest and org.eclipse.ecf.tests.remoteservice.rest

Marking as obsolete

Comment 7 Scott Lewis 2009-11-17 17:44:23 EST

After consulting with Holger, I've applied most of the changes previously in the attachment and released to HEAD.

The REST api is now significantly different than it was.  See the org.eclipse.ecf.tests.remoteservice.rest for examples.  We (and hopefully others) will be adding example to the REST wiki page(s): 

http://wiki.eclipse.org/REST_abstraction_for_ECF

Resolving this bug.