Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Feature/rest delete replace collection #3482

Merged
merged 8 commits into from

5 participants

@weierophinney

In reviewing the AbstractRestfulController implementation against various docs, I discovered two capabilities we currently not only do not support, but raise exceptions for:

  • DELETE to a collection (i.e., to "/users" vs "/users/1"). While rarely used, this method is allowed, and is used to indicate that the entire collection should be deleted.
  • PUT to a collection (i.e., to "/users" vs "/users/1"). While rarely used, this method is allowed, and is used to indicate that the entire collection should be replaced with what is provided in the request.

This PR adds support for both operations. It adds two new methods, deleteList() and replaceList(), which were chosen to mirror the existing getList() method while semantically indicating purpose.

weierophinney added some commits
@weierophinney weierophinney Allow deletion of collections via AbstractRestfulController
- Implemented deleteList() method, and updated switch for DELETE method
  to delegateto this method when no identifier is present.
531d019
@weierophinney weierophinney Added ability to replace a collection
- PUT on a collection (i.e., without an identifier) now invokes
  replaceList()
- Rewrote logic in DELETE case to mirror logic in PUT, GET, and DELETE
  cases.
7ee4e60
@weierophinney weierophinney Removed no longer used method
- Removed processPutData() as it is no longer necessary, nor used. Since
  this was only introduced for 2.1, it's safe to remove at this time.
f4b3d73
...ary/Zend/Mvc/Controller/AbstractRestfulController.php
@@ -229,14 +262,15 @@ public function onDispatch(MvcEvent $e)
break;
// DELETE
case 'delete':
- if (null === $id = $routeMatch->getParam('id')) {
- if (! ($id = $request->getQuery()->get('id', false))) {
- throw new Exception\DomainException(
- 'Missing identifier');
- }
+ $id = $this->getIdentifier($routeMatch, $request);
+ if ($id) {
@Freeaqingme Collaborator

What happens if $id is int(0), I think it would delete the list, rather than the resource identified by id=0 ? Same goes for PUT.

@weierophinney Owner

Good point -- I'll update to if (false !== $id), as getIdentifier() returns a boolean false or the identifier.

@weierophinney Owner

Interestingly, we used this construct previously, and nobody caught it! Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ary/Zend/Mvc/Controller/AbstractRestfulController.php
@@ -252,7 +286,7 @@ public function onDispatch(MvcEvent $e)
// HEAD
case 'head':
$id = $this->getIdentifier($routeMatch, $request);
- if (!$id) {
+ if (!$id !== false) {
@Maks3w Collaborator
Maks3w added a note

Double negation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ary/Zend/Mvc/Controller/AbstractRestfulController.php
@@ -270,7 +304,7 @@ public function onDispatch(MvcEvent $e)
// PATCH
case 'patch':
$id = $this->getIdentifier($routeMatch, $request);
- if (!$id) {
+ if (!$id !== false) {
@Maks3w Collaborator
Maks3w added a note

same here

@weierophinney Owner

crap, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Maks3w

This change need a specific test

...st/Mvc/Controller/TestAsset/RestfulTestController.php
@@ -99,6 +112,17 @@ public function patch($id, $data)
}
/**
+ * Replace the entire resource collection
+ *
+ * @param array|Traversable $items
+ * @return array|Traversable
+ */

Traversable should be \Traversable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Freeaqingme Freeaqingme merged commit 21f4850 into zendframework:develop
@Freeaqingme
Collaborator

Thank you for contributing! ;)

@davidwindell

Ping @weierophinney this is a BC break, we've been extending this method since way before 2.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 18, 2013
  1. @weierophinney

    Allow deletion of collections via AbstractRestfulController

    weierophinney authored
    - Implemented deleteList() method, and updated switch for DELETE method
      to delegateto this method when no identifier is present.
  2. @weierophinney

    Added ability to replace a collection

    weierophinney authored
    - PUT on a collection (i.e., without an identifier) now invokes
      replaceList()
    - Rewrote logic in DELETE case to mirror logic in PUT, GET, and DELETE
      cases.
  3. @weierophinney

    Removed no longer used method

    weierophinney authored
    - Removed processPutData() as it is no longer necessary, nor used. Since
      this was only introduced for 2.1, it's safe to remove at this time.
  4. @weierophinney
  5. @weierophinney
  6. @weierophinney

    Added unit test for patch

    weierophinney authored
    - PATCH without an identifier should raise an exception
  7. @weierophinney
  8. @weierophinney
This page is out of date. Refresh to see the latest.
View
95 library/Zend/Mvc/Controller/AbstractRestfulController.php
@@ -72,6 +72,22 @@
abstract public function delete($id);
/**
+ * Delete the entire resource collection
+ *
+ * Not marked as abstract, as that would introduce a BC break
+ * (introduced in 2.1.0); instead, raises an exception if not implemented.
+ *
+ * @return mixed
+ * @throws Exception\RuntimeException
+ */
+ public function deleteList()
+ {
+ throw new Exception\RuntimeException(sprintf(
+ '%s is unimplemented', __METHOD__
+ ));
+ }
+
+ /**
* Return single resource
*
* @param mixed $id
@@ -139,6 +155,23 @@ public function patch($id, $data)
}
/**
+ * Replace an entire resource collection
+ *
+ * Not marked as abstract, as that would introduce a BC break
+ * (introduced in 2.1.0); instead, raises an exception if not implemented.
+ *
+ * @param mixed $data
+ * @return mixed
+ * @throws Exception\RuntimeException
+ */
+ public function replaceList($data)
+ {
+ throw new Exception\RuntimeException(sprintf(
+ '%s is unimplemented', __METHOD__
+ ));
+ }
+
+ /**
* Update an existing resource
*
* @param mixed $id
@@ -229,19 +262,20 @@ public function onDispatch(MvcEvent $e)
break;
// DELETE
case 'delete':
- if (null === $id = $routeMatch->getParam('id')) {
- if (! ($id = $request->getQuery()->get('id', false))) {
- throw new Exception\DomainException(
- 'Missing identifier');
- }
+ $id = $this->getIdentifier($routeMatch, $request);
+ if ($id !== false) {
+ $action = 'delete';
+ $return = $this->delete($id);
+ break;
}
- $action = 'delete';
- $return = $this->delete($id);
+
+ $action = 'deleteList';
+ $return = $this->deleteList();
break;
// GET
case 'get':
$id = $this->getIdentifier($routeMatch, $request);
- if ($id) {
+ if ($id !== false) {
$action = 'get';
$return = $this->get($id);
break;
@@ -252,7 +286,7 @@ public function onDispatch(MvcEvent $e)
// HEAD
case 'head':
$id = $this->getIdentifier($routeMatch, $request);
- if (!$id) {
+ if ($id !== false) {
$id = null;
}
$action = 'head';
@@ -270,8 +304,10 @@ public function onDispatch(MvcEvent $e)
// PATCH
case 'patch':
$id = $this->getIdentifier($routeMatch, $request);
- if (!$id) {
- throw new Exception\DomainException('Missing identifier');
+ if ($id === false) {
+ $response = $e->getResponse();
+ $response->setStatusCode(405);
+ return $response;
}
$data = $this->processBodyContent($request);
$action = 'patch';
@@ -284,12 +320,23 @@ public function onDispatch(MvcEvent $e)
break;
// PUT
case 'put':
- $action = 'update';
- $return = $this->processPutData($request, $routeMatch);
+ $id = $this->getIdentifier($routeMatch, $request);
+ $data = $this->processBodyContent($request);
+
+ if ($id !== false) {
+ $action = 'update';
+ $return = $this->update($id, $data);
+ break;
+ }
+
+ $action = 'replaceList';
+ $return = $this->replaceList($data);
break;
// All others...
default:
- throw new Exception\DomainException('Invalid HTTP method!');
+ $response = $e->getResponse();
+ $response->setStatusCode(405);
+ return $response;
}
$routeMatch->setParam('action', $action);
@@ -315,26 +362,6 @@ public function processPostData(Request $request)
}
/**
- * Process put data and call update
- *
- * @param Request $request
- * @param $routeMatch
- * @return mixed
- * @throws Exception\DomainException
- */
- public function processPutData(Request $request, $routeMatch)
- {
- $id = $this->getIdentifier($routeMatch, $request);
- if (!$id) {
- throw new Exception\DomainException('Missing identifier');
- }
-
- $data = $this->processBodyContent($request);
-
- return $this->update($id, $data);
- }
-
- /**
* Check if request has certain content type
*
* @return boolean
View
48 tests/ZendTest/Mvc/Controller/RestfulControllerTest.php
@@ -90,6 +90,21 @@ public function testDispatchInvokesUpdateMethodWhenNoActionPresentAndPutInvokedW
$this->assertEquals('update', $this->routeMatch->getParam('action'));
}
+ public function testDispatchInvokesReplaceListMethodWhenNoActionPresentAndPutInvokedWithoutIdentifier()
+ {
+ $entities = array(
+ array('id' => uniqid(), 'name' => __FUNCTION__),
+ array('id' => uniqid(), 'name' => __FUNCTION__),
+ array('id' => uniqid(), 'name' => __FUNCTION__),
+ );
+ $string = http_build_query($entities);
+ $this->request->setMethod('PUT')
+ ->setContent($string);
+ $result = $this->controller->dispatch($this->request, $this->response);
+ $this->assertEquals($entities, $result);
+ $this->assertEquals('replaceList', $this->routeMatch->getParam('action'));
+ }
+
public function testDispatchInvokesDeleteMethodWhenNoActionPresentAndDeleteInvokedWithIdentifier()
{
$entity = array('id' => 1, 'name' => __FUNCTION__);
@@ -102,6 +117,16 @@ public function testDispatchInvokesDeleteMethodWhenNoActionPresentAndDeleteInvok
$this->assertEquals('delete', $this->routeMatch->getParam('action'));
}
+ public function testDispatchInvokesDeleteListMethodWhenNoActionPresentAndDeleteInvokedWithoutIdentifier()
+ {
+ $this->request->setMethod('DELETE');
+ $result = $this->controller->dispatch($this->request, $this->response);
+ $this->assertSame($this->response, $result);
+ $this->assertEquals(204, $result->getStatusCode());
+ $this->assertTrue($result->getHeaders()->has('X-Deleted'));
+ $this->assertEquals('deleteList', $this->routeMatch->getParam('action'));
+ }
+
public function testDispatchInvokesOptionsMethodWhenNoActionPresentAndOptionsInvoked()
{
$this->request->setMethod('OPTIONS');
@@ -341,4 +366,27 @@ public function testRequestingContentTypeReturnsFalseForInvalidMatches($contentT
$this->request->getHeaders()->addHeaderLine('Content-Type', $contentType);
$this->assertFalse($this->controller->requestHasContentType($this->request, TestAsset\RestfulTestController::CONTENT_TYPE_JSON));
}
+
+ public function testDispatchViaPatchWithoutIdentifierReturns405Response()
+ {
+ $entity = new stdClass;
+ $entity->name = 'foo';
+ $entity->type = 'standard';
+ $this->controller->entity = $entity;
+ $entity = array('name' => __FUNCTION__);
+ $string = http_build_query($entity);
+ $this->request->setMethod('PATCH')
+ ->setContent($string);
+ $result = $this->controller->dispatch($this->request, $this->response);
+ $this->assertInstanceOf('Zend\Http\Response', $result);
+ $this->assertEquals(405, $result->getStatusCode());
+ }
+
+ public function testDispatchWithUnrecognizedMethodReturns405Response()
+ {
+ $this->request->setMethod('PROPFIND');
+ $result = $this->controller->dispatch($this->request, $this->response);
+ $this->assertInstanceOf('Zend\Http\Response', $result);
+ $this->assertEquals(405, $result->getStatusCode());
+ }
}
View
24 tests/ZendTest/Mvc/Controller/TestAsset/RestfulTestController.php
@@ -41,6 +41,19 @@ public function delete($id)
}
/**
+ * Delete the collection
+ *
+ * @return \Zend\Http\Response
+ */
+ public function deleteList()
+ {
+ $response = $this->getResponse();
+ $response->setStatusCode(204);
+ $response->getHeaders()->addHeaderLine('X-Deleted', 'true');
+ return $response;
+ }
+
+ /**
* Return single resource
*
* @param mixed $id
@@ -99,6 +112,17 @@ public function patch($id, $data)
}
/**
+ * Replace the entire resource collection
+ *
+ * @param array|\Traversable $items
+ * @return array|\Traversable
+ */
+ public function replaceList($items)
+ {
+ return $items;
+ }
+
+ /**
* Update an existing resource
*
* @param mixed $id
Something went wrong with that request. Please try again.