Skip to content

LB paginator fix #521

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

Merged
merged 9 commits into from
Jan 15, 2015
Merged

LB paginator fix #521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d08b30b
Merge pull request #506 from rgbkrk/containerized_travis
ycombinator Dec 26, 2014
249d0e3
Merge branch 'working' of https://github.com/rackspace/php-opencloud …
Jan 8, 2015
f965348
Add mock HTTP responses
Jan 8, 2015
e5f57b5
Decouple the process of retrieving last element from setting marker
Jan 8, 2015
9cd0db1
Add new iterator to handle differences
Jan 8, 2015
3f69dfd
Implement into service and refactor tests
Jan 8, 2015
622d76d
Appease PSR-2 gods
Jan 9, 2015
6895f84
Attempt to fix 5.3 segfault
Jan 9, 2015
48e1df4
Tweaks based on code review
Jan 13, 2015
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
4 changes: 4 additions & 0 deletions lib/OpenCloud/Common/Collection/PaginatedIterator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,11 @@ public function updateMarkerToCurrent()
}

$element = $this->elements[$this->position];
$this->setMarkerFromElement($element);
}

protected function setMarkerFromElement($element)
{
$key = $this->getOption('key.marker');

if (isset($element->$key)) {
Expand Down
39 changes: 39 additions & 0 deletions lib/OpenCloud/LoadBalancer/Collection/LoadBalancerIterator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<?php

namespace OpenCloud\LoadBalancer\Collection;

use OpenCloud\Common\Collection\PaginatedIterator;

class LoadBalancerIterator extends PaginatedIterator
{
private $nextElement;

public function constructNextUrl()
{
$url = parent::constructNextUrl();

// We need to return n+1 items in order to grab the relevant marker value
$query = $url->getQuery();
$query['limit'] = $query['limit'] + 1;
$url->setQuery($query);

return $url;
}

public function updateMarkerToCurrent()
{
$this->setMarkerFromElement($this->nextElement);
}

public function parseResponseBody($body)
{
$response = parent::parseResponseBody($body);

if (count($response) >= $this->getOption('limit.page')) {
// Pop last element and save (we will need it for the next marker)
$this->nextElement = array_pop($response);
}

return $response;
}
}
12 changes: 11 additions & 1 deletion lib/OpenCloud/LoadBalancer/Service.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

use OpenCloud\Common\Log\Logger;
use OpenCloud\Common\Service\NovaService;
use OpenCloud\LoadBalancer\Collection\LoadBalancerIterator;

/**
* Class that encapsulates the Rackspace Cloud Load Balancers service
Expand Down Expand Up @@ -51,11 +52,20 @@ public function loadBalancer($id = null)
*/
public function loadBalancerList($detail = true, array $filter = array())
{
$options = $this->makeResourceIteratorOptions($this->resolveResourceClass('LoadBalancer'));

if (isset($filter['limit'])) {
$options['limit.page'] = $filter['limit'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment here explaining why this the next 2 statements are necessary? I'm particularly curious about the unset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess they aren't needed. I was thinking of these lines and wanted to have the iterator set the query value rather than couple it to the user's input. But thinking about it, it doesn't matter: either way the query is set and the URL will have the appropriate value.

Copy link
Contributor

Choose a reason for hiding this comment

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

either way the query is set and the URL will have the appropriate value.

How will the URL have the appropriate value without you explicitly setting $options['limit.page']? Is it part of the paginated "next page" URL returned in the API response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The base URL that is passed in as an option to the iterator is subsequently used to construct next URLs. If the query value is set on the initial URL, then it should still be set when the iterator constructs the next one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood. Thanks.

unset($filter['limit']);
}

$url = $this->getUrl();
$url->addPath(Resource\LoadBalancer::resourceName());
$url->setQuery($filter);

return $this->resourceList('LoadBalancer', $url);
$options = array_merge($options, array('baseUrl' => $url, 'key.marker' => 'id'));

return LoadBalancerIterator::factory($this, $options);
}

/**
Expand Down
5 changes: 5 additions & 0 deletions phpunit.xml.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,9 @@
<log type="coverage-clover" target="./build/logs/clover.xml" />
</logging>

<php>
<ini name="memory_limit" value="512M" />
<ini name="zend.enable_gc" value="0" />
</php>

</phpunit>
48 changes: 35 additions & 13 deletions tests/OpenCloud/Tests/LoadBalancer/ServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,13 @@

namespace OpenCloud\Tests\LoadBalancer;

class ServiceTest extends \OpenCloud\Tests\OpenCloudTestCase
{
private $service;

public function setupObjects()
{
$this->service = $this->getClient()->loadBalancerService('cloudLoadBalancers', 'DFW', 'publicURL');
}
use Guzzle\Http\Message\Response;
use Guzzle\Plugin\Mock\MockPlugin;
use OpenCloud\Rackspace;
use OpenCloud\Tests\MockSubscriber;

class ServiceTest extends LoadBalancerTestCase
{
public function test__construct()
{
$this->assertInstanceOf(
Expand All @@ -52,12 +50,36 @@ public function testLoadBalancer()
);
}

public function testLoadBalancerList()
public function test_Listing_Load_Balancers()
{
$this->assertInstanceOf(
self::COLLECTION_CLASS,
$this->service->loadBalancerList()
);
// Load JSON HTTP data
$authData = file_get_contents($this->getTestFilePath('Auth', './'));
$data1 = file_get_contents($this->getTestFilePath('LoadBalancers1'));
$data2 = file_get_contents($this->getTestFilePath('LoadBalancers2'));
$data3 = file_get_contents($this->getTestFilePath('LoadBalancers3'));

// Populate mock response queue
$mock = new MockPlugin();
$mock->addResponse(Response::fromMessage($authData))
->addResponse(Response::fromMessage($data1))
->addResponse(Response::fromMessage($data2))
->addResponse(Response::fromMessage($data3));

// We need to define our own setup because *jazz hands*
$client = $this->newClient();
$client->addSubscriber($mock);
$service = $client->loadBalancerService(null, 'IAD');

// Ensure that a series of paginated calls return a holistic collection
$lbs = $service->loadBalancerList(false, array('limit' => 2));
$ids = array();
foreach ($lbs as $lb) {
$ids[] = $lb->id;
}

// Check our assumptions
$this->isCollection($lbs);
$this->assertEquals($ids, array(1, 2, 3, 4, 5));
}

public function testBillableLoadBalancer()
Expand Down
80 changes: 80 additions & 0 deletions tests/OpenCloud/Tests/LoadBalancer/_response/LoadBalancers1.resp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
HTTP/1.1 200 OK
Status: 200 OK
Date: Thu, 28 Jul 2011 21:54:21 GMT
X-API-VERSION: 1.0.17
Content-Type: application/json
Content-Length: 1975

{
"loadBalancers":[
{
"name":"lb-site1",
"id":1,
"protocol":"HTTP",
"port":80,
"algorithm":"RANDOM",
"status":"ACTIVE",
"nodeCount":3,
"virtualIps":[
{
"id":403,
"address":"206.55.130.1",
"type":"PUBLIC",
"ipVersion":"IPV4"
}
],
"created":{
"time":"2010-11-30T03:23:42Z"
},
"updated":{
"time":"2010-11-30T03:23:44Z"
}
},
{
"name":"lb-site2",
"id":2,
"protocol":"HTTP",
"port":80,
"algorithm":"RANDOM",
"status":"ACTIVE",
"nodeCount":4,
"virtualIps":[
{
"id":401,
"address":"206.55.130.2",
"type":"PUBLIC",
"ipVersion":"IPV4"
}
],
"created":{
"time":"2010-11-30T03:23:42Z"
},
"updated":{
"time":"2010-11-30T03:23:44Z"
}
},
{
"name":"lb-site3",
"id":3,
"protocol":"HTTP",
"port":80,
"algorithm":"RANDOM",
"status":"ACTIVE",
"nodeCount":4,
"virtualIps":[
{
"id":401,
"address":"206.55.130.2",
"type":"PUBLIC",
"ipVersion":"IPV4"
}
],
"created":{
"time":"2010-11-30T03:23:42Z"
},
"updated":{
"time":"2010-11-30T03:23:44Z"
}
}
]
}
80 changes: 80 additions & 0 deletions tests/OpenCloud/Tests/LoadBalancer/_response/LoadBalancers2.resp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
HTTP/1.1 200 OK
Status: 200 OK
Date: Thu, 28 Jul 2011 21:54:21 GMT
X-API-VERSION: 1.0.17
Content-Type: application/json
Content-Length: 1975

{
"loadBalancers":[
{
"name":"lb-site3",
"id":3,
"protocol":"HTTP",
"port":80,
"algorithm":"RANDOM",
"status":"ACTIVE",
"nodeCount":4,
"virtualIps":[
{
"id":401,
"address":"206.55.130.2",
"type":"PUBLIC",
"ipVersion":"IPV4"
}
],
"created":{
"time":"2010-11-30T03:23:42Z"
},
"updated":{
"time":"2010-11-30T03:23:44Z"
}
},
{
"name":"lb-site4",
"id":4,
"protocol":"HTTP",
"port":80,
"algorithm":"RANDOM",
"status":"ACTIVE",
"nodeCount":4,
"virtualIps":[
{
"id":401,
"address":"206.55.130.2",
"type":"PUBLIC",
"ipVersion":"IPV4"
}
],
"created":{
"time":"2010-11-30T03:23:42Z"
},
"updated":{
"time":"2010-11-30T03:23:44Z"
}
},
{
"name":"lb-site5",
"id":5,
"protocol":"HTTP",
"port":80,
"algorithm":"RANDOM",
"status":"ACTIVE",
"nodeCount":4,
"virtualIps":[
{
"id":401,
"address":"206.55.130.2",
"type":"PUBLIC",
"ipVersion":"IPV4"
}
],
"created":{
"time":"2010-11-30T03:23:42Z"
},
"updated":{
"time":"2010-11-30T03:23:44Z"
}
}
]
}
34 changes: 34 additions & 0 deletions tests/OpenCloud/Tests/LoadBalancer/_response/LoadBalancers3.resp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
HTTP/1.1 200 OK
Status: 200 OK
Date: Thu, 28 Jul 2011 21:54:21 GMT
X-API-VERSION: 1.0.17
Content-Type: application/json
Content-Length: 1975

{
"loadBalancers":[
{
"name":"lb-site5",
"id":5,
"protocol":"HTTP",
"port":80,
"algorithm":"RANDOM",
"status":"ACTIVE",
"nodeCount":4,
"virtualIps":[
{
"id":401,
"address":"206.55.130.2",
"type":"PUBLIC",
"ipVersion":"IPV4"
}
],
"created":{
"time":"2010-11-30T03:23:42Z"
},
"updated":{
"time":"2010-11-30T03:23:44Z"
}
}
]
}
13 changes: 5 additions & 8 deletions tests/OpenCloud/Tests/OpenCloudTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,10 @@ abstract class OpenCloudTestCase extends \PHPUnit_Framework_TestCase

public function newClient()
{
$client = new Rackspace(Rackspace::US_IDENTITY_ENDPOINT, array(
return new Rackspace(Rackspace::US_IDENTITY_ENDPOINT, array(
'username' => 'foo',
'apiKey' => 'bar'
));

$client->addSubscriber(new MockSubscriber());
//$client->addSubscriber(LogPlugin::getDebugPlugin());

$client->authenticate();

return $client;
}

public function getClient()
Expand All @@ -59,6 +52,10 @@ public function getClient()
public function setUp()
{
$this->client = $this->newClient();

$this->client->addSubscriber(new MockSubscriber());
$this->client->authenticate();

$this->setupObjects();
$this->handleMockSubscribers();
}
Expand Down