arangodb / arangodb-php

PHP ODM for ArangoDB
https://www.arangodb.com
Apache License 2.0
183 stars 43 forks source link

More descriptive Exception messages #180

Closed razvanphp closed 9 years ago

razvanphp commented 9 years ago

Currently if we set ArangoDb\ConnectionOptions::OPTION_TIMEOUT, and the request times out, we get the message:

triagens\ArangoDb\ClientException: Got an invalid response from the server after request to PATCH /_db/bi_live/_api/document/posts/5847702094515?policy=error

that is very deceptive, since this is simply not true. I don't know if you can distinguish between other errors too, but at least this one should be personalized, maybe also including "after x seconds" and response status in the text.

frankmayer commented 9 years ago

@razvanphp you were right, it was deceptive. I fixed this and you're getting a correct ClientException in case of a timeout. The "after x seconds" though, is not provided by the status provider atm. I would have to look into that. However, I believe you could look into the slow queries log to get more info on such queries.

@jsteemann I blatantly used 408 (same as the http code for request timeout for servers) for the exception code. :smile: I am not sure if this is the right way to go with client error codes, however I think it does not hurt anyone, since it regards the ClientException and not the ServerException. Any thoughts?

@jsteemann There are some inconsistencies between devel and master from some of your commits. I am not sure which ones you'd like to keep. Could you take a look at them and merge any outstanding things? Thanks :smile:

jsteemann commented 9 years ago

@frankmayer : I think using error code 408 is fine.

Regarding the differences between the master and devel branches: those were intentional, and I would like to keep both. The master branchis currently matching the ArangoDB master/2.5 server version whereas the devel branch is matching the 2.6/devel server versions. The devel branch contains additional features that are not usable in master/2.5. This is mainly the export API plus the associated tests.

The other changes I made in devel but not in master are fixes for comments in the driver code plus probably a few changes in the docs or examples, but nothing major.

From my point of view, the only change that could be moved from the devel to the master branch is this one:

diff --git a/lib/triagens/ArangoDb/HttpHelper.php b/lib/triagens/ArangoDb/HttpHelper.php
index 3e28a10..0820bf4 100644
--- a/lib/triagens/ArangoDb/HttpHelper.php
+++ b/lib/triagens/ArangoDb/HttpHelper.php
@@ -286,10 +286,6 @@ class HttpHelper

         if (!isset($parts[1]) or $parts[1] === null) {
             if ($originUrl !== null && $originMethod !== null) {
+                if ($httpMessage === '') {
+                    throw new ClientException('Got no response from the server after request to '
+                        . $originMethod . ' ' . $originUrl . ' - Note: this may be a timeout issue');
+                }
                 throw new ClientException('Got an invalid response from the server after request to '
                     . $originMethod . ' ' . $originUrl);
             }

But this has been address probably even better by your change.

frankmayer commented 9 years ago

OK, didn't dive into your commits as much. Great! Then we'll just take it from there...