fabriciocolombo / delphi-rest-client-api

A Delphi REST client API to consume REST services written in any programming language.
Apache License 2.0
380 stars 182 forks source link

Changes the behavior for the 404 error and allows the use of Super Object in Delphi XE or up #32

Closed RobertoSchneiders closed 9 years ago

RobertoSchneiders commented 9 years ago

About the 404 error, I started a conversation in PR https://github.com/fabriciocolombo/delphi-rest-client-api/pull/24. This change was introduced in this PR and caused some problems for me (I updated last week, was using an old version of the lib).

I think the error handling should be the same for all HTTP errors. If will not raise an exception, has to be the same for other errors.

I don't like exceptions too, but I think is too late to change this right now. I'm using this lib in a bunch of places and I will have to refactor a lot of code if we decide to remove this "exception flow".

What do you think?

RobertoSchneiders commented 9 years ago

I fixed the HttpConnectionIndy @thomaserlang

thomaserlang commented 9 years ago

Cool, i reverted my commit.

I would like the exception from Indy to pass through to the DoRequest method as the other HTTP modules does. But if I recall correctly the body was empty in Indy after the exception had been raised. Any ideas?

RobertoSchneiders commented 9 years ago

Apparently, the objective of the exception handling on HttpConnectionIndy is to make the behavior be more like the others. AFAIK, the WinHTTP not raises exceptions. We can simply ignore the exceptions in the Indy adapter?

I made a simple test. I commented all the exception treatment in the HttpConnectionIndy and works fine (Tests OK). The response is blank, but anyway, you should not trust or use this information when you get an HTTP ERROR.

thomaserlang commented 9 years ago

I trust the info i get from the HTTP error since I wrote it. In my REST API I return a JSON object containing information about the error, used to generate a nice error message for the user in the language they now prefer.

Therefore it's a must for me that the body of the HTTP Error is returned.

RobertoSchneiders commented 9 years ago

Right. We can simply write the EIdHTTPProtocolException ErrorMessage in the response.

I don't know exactly how this will affect the json serialization of complex objects. I think this is a problem actually. Am I wrong or the library tries to deserialize the response into objects even with HTTP errors? The response handler is executed before the verification of the response code. It make sense to you?

thomaserlang commented 9 years ago

If it's an HTTP error it will raise the EHTTPError exception before the JSON serialization.

esasse commented 9 years ago

:+1:

RobertoSchneiders commented 9 years ago

Yeah, you are right @thomaserlang.

So, I think this will solve your problem (exception from Indy to pass through to the DoRequest).

procedure THttpConnectionIndy.Get(AUrl: string; AResponse: TStream);
var
  retryMode: THTTPRetryMode;
begin
  try
    FIdHttp.Get(AUrl, AResponse);
  except
    on E: EIdHTTPProtocolException do
    begin
      if Length(E.ErrorMessage) > 0 then
        AResponse.Write(E.ErrorMessage[1], Length(E.ErrorMessage));
    end;
    on E: EIdSocketError do
    begin
      ...
    end;
  end;
end;

AFAIK, I can't remove the EIdSocketError exception handling because OnConnectionLost event is a connection specific implementation (not handled in DoRequest). Seems that the OnConnectionLost is working only for Indy. The WinHttp and WinInet implementations not handles this event.

thomaserlang commented 9 years ago

Cool.

Yes, I only implemented the OnConnectionLost for Indy.

How does WinHTTP and WinInet handle "ConnectionLost"? Do they just return a timeout code? If so, do they use the same? Like 599? We should normalize this.

I don't know if raising the ConnectionLost exception is better than using the EHTTPError exception. As long as they use the same timeout code the EHTTPError could work.

RobertoSchneiders commented 9 years ago

In the WinHtttp case, I think it will generate some error, like these:

But I am not sure about that. http://msdn.microsoft.com/en-us/library/windows/desktop/aa383770(v=vs.85).aspx

RobertoSchneiders commented 9 years ago

You can review this @fabriciocolombo?

RobertoSchneiders commented 9 years ago

I added proxy authentication support.