Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

404 error returned on getUser api call #185

Closed
kahuwi opened this issue Nov 30, 2016 · 21 comments
Closed

404 error returned on getUser api call #185

kahuwi opened this issue Nov 30, 2016 · 21 comments

Comments

@kahuwi
Copy link

kahuwi commented Nov 30, 2016

Version info

  • intercom-php version: 2.0.0
  • PHP version: 5.6

Expected behavior

when user is not in intercom and getUser is called, the api should return with status with the user not found in the result

Actual behavior

api never returns, after further investigation guzzle dies on the 404 error returned by intercom api call. Page exists, has response code, but the http status of 404 causes guzzle to die.

Fatal error: Uncaught exception 'GuzzleHttp\Exception\ClientException' with message 'Client error: GET https://api.intercom.io/users?email=expiration%40test.com resulted in a 404 Not Found response: {"type":"error.list","request_id":"aomm6mt35nhsscst6fhg","errors":[{"code":"not_found","message":"User Not Found"}]} ' in /datadisk/vhosts/blueprintprep.com/myrest/vendor/guzzlehttp/guzzle/src/Exception/RequestException.php:107 Stack trace: #0 /datadisk/vhosts/blueprintprep.com/myrest/vendor/guzzlehttp/guzzle/src/Middleware.php(65): GuzzleHttp\Exception\RequestException::create(Object(GuzzleHttp\Psr7\Request), Object(GuzzleHttp\Psr7\Response)) #1 /datadisk/vhosts/blueprintprep.com/myrest/vendor/guzzlehttp/promises/src/Promise.php(203): GuzzleHttp\Middleware::GuzzleHttp{closure}(Object(GuzzleHttp\Psr7\Response)) #2 /datadisk/vhosts/blueprintprep.com/myrest/vendor/guzzlehttp/promises/src/Promise.php(156): GuzzleHttp\Promise\Promise::callHandler(1, Object(GuzzleHttp\Psr7\Response), Array) #3 /datadisk/vhosts/blueprintprep.co in /datadisk/vhosts/blueprintprep.com/myrest/vendor/guzzlehttp/guzzle/src/Exception/RequestException.php on line 107

Steps to reproduce (as granular as possible, including screenshots where appropriate)

  1. use this code:
require_once 'vendor/autoload.php';
use Intercom\IntercomClient;
$client = new IntercomClient('YOUR TOKEN GOES HERE',null);
echo 'Client Defined<br>';
$user = $client->users->getUsers(["email" => "bogus@noserver.com"]);
echo $user->id;
echo '<br>';
$delete_user = $user->id;
echo $user->name;
echo '<br>';
$client->users->deleteUser($delete_user);
  1. Run code from 1 and you will see the Client Defined, and the error. However none of the other echo are seen since the getUsers call never returned.

  2. Intercom is overloading the 404 to mean user not found. The page is found and has the proper response data to indicate user not found. However, since the http status is 404 and guzzle sees as a page not found guzzle errors out. Intercom needs to return 200 or intercom api needs to handle the 404 and return the response back up the line so I can handle it in my codebase.

Logs

@travega
Copy link
Contributor

travega commented Dec 1, 2016

@Skaelv @mmartinic ^

@kahuwi
Copy link
Author

kahuwi commented Jan 10, 2017

Been over a month on my issue, Is there any status?
Thanks,
Karl

@kant01ne
Copy link
Contributor

@kahuwi Thanks for bringing this up and sorry for the delay in our answer.

You can catch Guzzle Exceptions in your code if you don't want to throw exceptions when a user is not found.
We are not going to modify our API status code neither are we going to make breaking change in the SDK (more context on #198).

I'm adding a section in the readme to explain how to catch exceptions from Guzzle properly.

@kahuwi
Copy link
Author

kahuwi commented Feb 11, 2017

Bottom line in this our response is that instead of you the provider I the consumer have to add code to detect bad behavior within your api. This is unacceptable. You are violating the web standards with your API behavior. Your php API call never returns, if it came back I could catch it and deal with the off standard error return. Maybe you can give a way when making a web call via your API that allows me to handle it?

@kant01ne
Copy link
Contributor

Returning 404 when a resource is not found is a standard in REST APIs.

As discussed in #198 catching the exception on your end is probably the best way for you to handle the User not found Exception. Even if we had merged #198 (which would have been a breaking change) you would have had to handle the response with an if/else statement.

@kahuwi
Copy link
Author

kahuwi commented Feb 13, 2017 via email

@kahuwi
Copy link
Author

kahuwi commented Feb 13, 2017 via email

@atlkuk
Copy link

atlkuk commented Apr 4, 2017

Hi, I have the same issue, even if the user exists in my intercom users list. The code is:

require 'intercom/vendor/autoload.php';

use Intercom\IntercomClient;

$client = new IntercomClient(INTERCOM_APP_ID, INTERCOM_APP_SECRET);

$user = $client->users->getUsers(["user_id" => ID]);

I tried with email too but the method doesn't return any user

is there a possible solution?

Thanks

@kahuwi
Copy link
Author

kahuwi commented Apr 4, 2017

If you send an intercom getUsers call to a non-existent user they use 404 causing their php api to blow up. What you need to do is try{}catch{} the get user call so that the 404 will then be seen in your php client. simple example:
try {
$user = $client->users->getUsers(["email" => "test@test.com"]);
$return_data['data']['user_id'] = $user->id;
$return_data['data']['user_name'] = $user->name;
} catch (Exception $e) {
print_r(get_class_methods($e));
$error_code = $e->getCode();
if ($error_code == 404):
echo "ERROR : " . $error_code;
else:
endif;
$return_data['data']['error'] = $error_code . ' - User Not Found';
$return_data['data']['user_id'] = 0;
}
This concept works for me.

@kahuwi
Copy link
Author

kahuwi commented Apr 4, 2017

One other thing:
My code excerpt previously is in a class method that has the following:

require_once 'vendor/autoload.php';
use Intercom\IntercomClient;

class Intercom_Model 
{
	var $intercom;

function __construct() {
$this->intercom = new IntercomClient('YOUR API KEY',null);
}// end of constructor

function test(){
// code from previous comment
} // end of test

} // end of class

make sure you are using the new API as intercom has changed KEY use in the API
Sorry that I forgot this info earlier

@atlkuk
Copy link

atlkuk commented Apr 5, 2017

Thanks for the reply, but i don't understand why it returns me a 404 error even if the user exists, i'm sure of that because I see it in my users list

@kahuwi
Copy link
Author

kahuwi commented Apr 5, 2017

If you run the above and are getting a 404, double check your getUsers parameter and use email and not the ID. In the catch the print_r will lay it all out and you can see what Intercom is returning if all looks good, then you will need to chat into developer.intercom.io and talk to them.

@atlkuk
Copy link

atlkuk commented Apr 5, 2017

ok thx, I'll try :)

@joshtorres
Copy link

joshtorres commented Apr 21, 2017

@kahuwi Unfortunately, that wiki article you shared isn't correct. 404 definitely doesn't mean it couldn't connect to the server. The entire 4xx series of codes is for "Client Errors", not server errors. A 5xx error would mean it was the server, and more specifically 598 and 599 are connection timeout codes. Also, 404 means that a "resource" cannot be found, not just a "page". A "page" is one of many resources that could be searched for. 404 is commonly and correctly used when a resource cannot be found in a REST API. How a HTTP Client library handles a 404 error is a different issue. Some choose to throw an Exception, while others simply have a response code with empty data. With this library, the appropriate action seems to be catching ClientException and then checking the status code (as specified in the readme of this repo). Here are some resources for more info on HTTP codes. As you will see, some very popular libraries such as Twitter and Yahoo refer to 404 as "resource" not found and even provide an example of it being a "user not found". Hope this helps.
https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#4xx_Client_errors
http://www.restapitutorial.com/httpstatuscodes.html
https://dev.twitter.com/overview/api/response-codes
https://developer.yahoo.com/social/rest_api_guide/http-response-codes.html

@kahuwi
Copy link
Author

kahuwi commented Apr 21, 2017

That is all well and fine, if you choose to operate in this fashion you need to do several things.
First, you need to update your php api documentation reflecting your usage of the 404 and how to detect it.
Second, you need to provide within your php api downloads a solution that does not throw errors when the 404 comes into guzzle (guzzle uses the 404 as an error of page not found, the norm).

It is unfortunate that the 404 error was not clearly defined in standard leaving the it open to this kind of interpretation, I will agree to disagree on your usage and move on.

@joshtorres
Copy link

joshtorres commented Apr 21, 2017

Well, the throwing of errors is a Guzzle thing, not an intercom issue. Intercom is simply responding with 404 status code, which is the correct response.
Also, I'm not sure what you mean by documentation, but like I said the README.md of this very repo that we are replying on says near the bottom exactly how to handle these exceptions.
screen shot 2017-04-21 at 9 27 26 am

And with all due respect, I don't think this is an issue of interpretation. This is an industry standard for HTTP status codes for REST APIs. If you have official documentation that says otherwise please share. In the meantime I will provide some more examples of companies like Mozilla, Google, and the official HTTP statuses website that all say "resource" not found. Obviously things of this nature are not set in stone, which is why adhering to standards is very important, as well as discussions such as this one. Sorry if I'm acting like a tight-wad about this, but I think it's important to understand the difference between a library doing something that isn't industry standard compared to something that we haven't used or experienced yet. It can have a trickle effect down to newer developers who are looking for answers to these very common questions.

Overall, it sounds like your main issue is with the Guzzle library, not Intercom, so you may want to reach out to them for further information if you still feel it's necessary.

https://httpstatuses.com/404
https://cloud.google.com/storage/docs/json_api/v1/status-codes#404_Not_Found
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status

@kahuwi
Copy link
Author

kahuwi commented Apr 22, 2017

You include guzzle in your php api codebase, so if you don't want to handle their 404 usage, then stop including it in your release and provide something that does.
Your Intercom developer api does not address handling this condition (php api 404 on user not found).

From Wikipedia, the free encyclopedia
The HTTP 404, 404 Not Found and 404 (pronounced "four oh four") error message is a Hypertext Transfer Protocol (HTTP) standard response code, in computer network communications, to indicate that the client was able to communicate with a given server, but the server could not find what was requested.

The web site hosting server will typically generate a "404 Not Found" web page when a user attempts to follow a broken or dead link; hence the 404 error is one of the most recognizable errors encountered on the World Wide Web."

The key term "typically" is where I am coming from in that my interpretation while different from yours is also correct.

Where I am coming from towards your side is that you need to document your interpretation so that when developers run into issues like this you have clearly defined how to handle it.

The standard:
https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.5

says "This status code is commonly used when the server does not wish to reveal exactly why the request has been refused, or when no other response is applicable."

So there is still room for interpretation.

My main issue is with Intercom and not Guzzle.
Update your php api doc for how you are handling user not found and all this would have been avoided.

@joshtorres
Copy link

Well I want to be clear I have nothing to do with Intercom. I was simply responding to your post to try to give further clarification. Also, I definitely see what you me and I'm not trying to discredit the fact that there has been some confusion over this issue. But I was trying to communicate on what most of the community agrees on as well as many http rest API standards. The difference to note here is what you are referring to is when someone is trying to access a webpage and what I'm trying to point out is these are standards for REST apis. But we can leave it at that. Sorry to take up so much of your time.

@theEricDevelops
Copy link

I would actually tend to disagree with you, @joshtorres that a 404 error is the proper error. However, @kahuwi, I also don't think 200 would be the best error.

Instead, per the standards found at https://tools.ietf.org/html/rfc7231#page-53, I would assert a 204: No Content would be the best way to handle that. There are several reasons for this, but mainly you're not wondering if your request was bad as you would in the case of a 404 code, and it's not stating that everything was fine and grand in the case of a 200 status code.

@kahuwi
Copy link
Author

kahuwi commented May 5, 2017

I agree that 200, like 404 are extreme case indicators and that for the user not found instance that 204 would be a better response. What needs to happen is for intercom to fully document their usage so that developers are not caught chasing errors that are really intercom "working as designed". Thanks for the input

@kant01ne
Copy link
Contributor

kant01ne commented Jun 7, 2017

Doc has been updated with guidelines to catch Guzzle issues.
Even if the conversation was super interesting I'm going to close this one :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants