Bug 361933

Comment 1 Scott Lewis 2011-10-25 17:16:26 EDT

Created attachment 205952 [details]
revised RestClientService

Hi Vaselin,

I propose this new version of RestClientService to resolve this bug.  This revision introduces some new protected methods to RestClientService, as well as a new 'response' field to RestException class...to support the retrieval of the entire response body for a non-200 response code.

Please let me know what you think as a comment on this bug...and if it meets your needs then I'll release it for ECF 3.5.3.

Comment 2 Veselin Vasilev 2011-10-26 04:17:11 EDT

Hi Scott,
this will do the job although it forces me to extend RestClientService and override retrieveErrorResponseBody method. To do this I would also need to extend RestClientContainer and return my implementation of RestClientService in createRemoteService method. In this case I am not able to use ContainerFactory but have to create new instance by myself.
Considering the fact that RestException will have field 'response', isn't it better to always read response so the code looks like this : 
    	responseCode = httpClient.executeMethod(httpMethod);
    	responseBody = getResponseAsString(httpMethod);
		if (!isResponseOk(responseCode)) {
			handleException("Http response not OK.  URL=" + uri + " responseCode=" + new Integer(responseCode), null, responseCode, responseBody); //$NON-NLS-1$ //$NON-NLS-2$
		}
This way API client will receive RestException where there are fields 'errorCode' and 'response'
Based on Exception's errorCode client can decide if 'response' is needed
What do you think ?

Thanks,
Veselin


(In reply to comment #1)
> Created attachment 205952 [details]
> revised RestClientService
> 
> Hi Vaselin,
> 
> I propose this new version of RestClientService to resolve this bug.  This
> revision introduces some new protected methods to RestClientService, as well as
> a new 'response' field to RestException class...to support the retrieval of the
> entire response body for a non-200 response code.
> 
> Please let me know what you think as a comment on this bug...and if it meets
> your needs then I'll release it for ECF 3.5.3.

Comment 3 Scott Lewis 2011-10-26 10:46:21 EDT

Hi Vaselin,

(In reply to comment #2)
> Hi Scott,
> this will do the job although it forces me to extend RestClientService and
> override retrieveErrorResponseBody method. To do this I would also need to
> extend RestClientContainer and return my implementation of RestClientService in
> createRemoteService method. In this case I am not able to use ContainerFactory
> but have to create new instance by myself.
> Considering the fact that RestException will have field 'response', isn't it
> better to always read response so the code looks like this : 
>         responseCode = httpClient.executeMethod(httpMethod);
>         responseBody = getResponseAsString(httpMethod);
>         if (!isResponseOk(responseCode)) {
>             handleException("Http response not OK.  URL=" + uri + "
> responseCode=" + new Integer(responseCode), null, responseCode, responseBody);
> //$NON-NLS-1$ //$NON-NLS-2$
>         }
> This way API client will receive RestException where there are fields
> 'errorCode' and 'response'
> Based on Exception's errorCode client can decide if 'response' is needed
> What do you think ?

Without consulting the spec more thoroughly, I'm hesitant to read the response body for every response...as I have no idea whether that's allowed by the spec...not to mention how existing web servers behave under those conditions.  If you could research this a little and report on this bug, I would appreciate it...as I don't have a lot of time these days.

But given that you want to modify the handling of the response code and response anyway (i.e. different from spec)...don't you need to override some method in RestClientService anyway?

And I'm not sure I understand the use case that would prevent you from using the ContainerFactory (or rather IContainerFactory service) to create your own container.  It's not a difficult thing to setup a container factory for your new type...and will happily explain this if you wish.

Comment 4 Veselin Vasilev 2011-10-26 15:18:17 EDT

Hi Scott,
I am fine with the solution you propose. It will allow for API client more freedom to only expect response for specific error codes.
My only concern is that instead of using :
IContainer originalContainer = ContainerFactory.getDefault().createContainer("ecf.rest.client", url);
I should do something like :
IContainer container = new MyRestClientContainer(restID); and do some tricks to create restID but I guess we can discuss usage tech question in the mailing list.

Thanks,
Veselin

> Without consulting the spec more thoroughly, I'm hesitant to read the response
> body for every response...as I have no idea whether that's allowed by the
> spec...not to mention how existing web servers behave under those conditions. 
> If you could research this a little and report on this bug, I would appreciate
> it...as I don't have a lot of time these days.
> 
> But given that you want to modify the handling of the response code and
> response anyway (i.e. different from spec)...don't you need to override some
> method in RestClientService anyway?
> 
> And I'm not sure I understand the use case that would prevent you from using
> the ContainerFactory (or rather IContainerFactory service) to create your own
> container.  It's not a difficult thing to setup a container factory for your
> new type...and will happily explain this if you wish.

Comment 5 Scott Lewis 2011-10-26 20:02:06 EDT

Release described changes to master:

http://git.eclipse.org/c/ecf/org.eclipse.ecf.git/commit/?id=da1c6969562414700956a65a363d4dca0708f2cf

Resolving as fixed.

Comment 6 Scott Lewis 2011-10-26 20:07:20 EDT

(In reply to comment #4)
> Hi Scott,
> I am fine with the solution you propose. It will allow for API client more
> freedom to only expect response for specific error codes.
> My only concern is that instead of using :
> IContainer originalContainer =
> ContainerFactory.getDefault().createContainer("ecf.rest.client", url);
> I should do something like :
> IContainer container = new MyRestClientContainer(restID); and do some tricks to
> create restID but I guess we can discuss usage tech question in the mailing
> list.
> 

This class should probably have been made external rather than internal:

org.eclipse.ecf.internal.remoteservice.rest.RestClientContainerInstantiator

but you could use the source as a model for your MyRestClientContainer (which presumably can extend RestClientContainer).  This RestClientContainerInstantiator has the code necessary to specify a RestID upon container instance creation (i.e. via a call to:

IContainer container = containerFactory.createContainer("my.client.container",new Object[] { restID });

Again...I now regret that I didn't make this class external...so that folks like you could reuse it...perhaps an enhancement request should be created to do that (for the next minor or major release).

Please let me know if this isn't clear how you would do this...also see org.eclipse.ecf.provider.rest/plugin.xml...containerFactory extension.