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.

Sign up for GitHub

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

Introduce new Idempotency error class #436

Merged
merged 1 commit into from
Feb 12, 2018
Merged

Introduce new Idempotency error class #436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions init.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
require(dirname(__FILE__) . '/lib/Error/ApiConnection.php');
require(dirname(__FILE__) . '/lib/Error/Authentication.php');
require(dirname(__FILE__) . '/lib/Error/Card.php');
require(dirname(__FILE__) . '/lib/Error/Idempotency.php');
require(dirname(__FILE__) . '/lib/Error/InvalidRequest.php');
require(dirname(__FILE__) . '/lib/Error/Permission.php');
require(dirname(__FILE__) . '/lib/Error/RateLimit.php');
Expand Down
5 changes: 5 additions & 0 deletions lib/ApiRequestor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ public function request($method, $url, $params = null, $headers = null)
* @param array $resp
*
* @throws Error\InvalidRequest if the error is caused by the user.
* @throws Error\Idempotency if the error is caused by an idempotency key.
* @throws Error\Authentication if the error is caused by a lack of
* permissions.
* @throws Error\Permission if the error is caused by insufficient
Expand Down Expand Up @@ -106,6 +107,7 @@ private static function _specificAPIError($rbody, $rcode, $rheaders, $resp, $err
$msg = isset($errorData['message']) ? $errorData['message'] : null;
$param = isset($errorData['param']) ? $errorData['param'] : null;
$code = isset($errorData['code']) ? $errorData['code'] : null;
$type = isset($errorData['type']) ? $errorData['type'] : null;

switch ($rcode) {
case 400:
Expand All @@ -114,6 +116,9 @@ private static function _specificAPIError($rbody, $rcode, $rheaders, $resp, $err
if ($code == 'rate_limit') {
return new Error\RateLimit($msg, $param, $rcode, $rbody, $resp, $rheaders);
}
if ($type == 'idempotency_error') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the usual pedantry, but it kind of looks like the other conditional ladders for error interpretation in this file are alphabetized, and it might be worth giving this the same treatment. (Also, might want to change this to a switch like the others? It doesn't matter that much though.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean grouping rate_limit and idempotency_error in a switch statement? The problem with that is that the two values are in different variables ($code vs $type).

I've taken a look at how we handle this in other libraries: stripe-ruby completely dropped support for 400 rate_limit errors, which maybe we should do in other libraries as well, but that would be a breaking change and we just released 6.0.0 :/

stripe-python uses a series of semi-complex if statements: https://github.com/stripe/stripe-python/blob/master/stripe/api_requestor.py#L190-L214, but then again Python doesn't have switch.

I'm tempted to update the logic to something like stripe-python anyway, wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, my mistake — I wasn't paying enough attention to notice the $code vs. $type distinction there. Feel free to leave it.

I'm tempted to update the logic to something like stripe-python anyway, wdyt?

IMO the logic ladders are at least pretty similar, and honestly PHP's is quite a bit more readable, so it might be okay to just leave as is. Up to you :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's leave it as is then. Thanks!

return new Error\Idempotency($msg, $rcode, $rbody, $resp, $rheaders);
}

// intentional fall-through
case 404:
Expand Down
7 changes: 7 additions & 0 deletions lib/Error/Idempotency.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

namespace Stripe\Error;

class Idempotency extends Base
{
}
29 changes: 29 additions & 0 deletions tests/Stripe/ApiRequestorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,35 @@ public function testRaisesInvalidRequestErrorOn400()
}
}

public function testRaisesIdempotencyErrorOn400AndTypeIdempotencyError()
{
$this->stubRequest(
'POST',
'/v1/charges',
array(),
null,
false,
array(
'error' => array(
'type' => 'idempotency_error',
'message' => "Keys for idempotent requests can only be used with the same parameters they were first used with. Try using a key other than 'abc' if you meant to execute a different request.",
),
),
400
);

try {
Charge::create();
$this->fail("Did not raise error");
} catch (Error\Idempotency $e) {
$this->assertSame(400, $e->getHttpStatus());
$this->assertTrue(is_array($e->getJsonBody()));
$this->assertSame("Keys for idempotent requests can only be used with the same parameters they were first used with. Try using a key other than 'abc' if you meant to execute a different request.", $e->getMessage());
} catch (\Exception $e) {
$this->fail("Unexpected exception: " . get_class($e));
}
}

public function testRaisesAuthenticationErrorOn401()
{
$this->stubRequest(
Expand Down