REST API #535

NebriBlackwing · 2018-07-14T02:05:25Z

The big heavy development work is now done and ready for it's first review round.

I'm planning on writing a series of unit tests against the Api Controllers, and after doing the rebase a bunch of unit tests that were passing are no longer. I'll start taking a look at these units tests later this weekend.

When this PR gets merged it will close Issue #83

NebriBlackwing · 2018-07-14T20:20:57Z

@juliushaertl I've managed to reduce the number of errors that were happening with the units tests down to 1. It was pretty simple, phpunit removed setExpectedException and replaced it with a new function. Here's my reference: Roave/BetterReflection#170

The last error is eluding me at the moment, mind giving insights on how to proceed past that error? I'll be starting my own unit tests against the new API real soon :).

juliushaertl · 2018-07-15T10:03:48Z

appinfo/routes.php

+		['name' => 'board_api#undo_delete', 'url' => '/api/v1.0/boards/{boardId}/undo_delete', 'verb' => 'POST'],
+
+		['name' => 'stack_api#index', 'url' => '/api/v1.0/boards/{boardId}/stacks', 'verb' => 'GET'],
+		['name' => 'stack_api#index', 'url' => '/api/v1.0/boards/{boardId}/stacks/{stackId}', 'verb' => 'GET'],


Shouldn't that be handled by stack_api#get

yup! a simple copy/paste oversight!

juliushaertl · 2018-07-15T10:23:02Z

lib/Controller/Helper/ApiHelper.php

+
+class ApiHelper {
+
+	public static function entityHasError($entityId, $entityName, $service) {				


I think we can remove the static keyword, since we will always call this on an object due to dependency injection.

juliushaertl · 2018-07-15T10:24:06Z

lib/Controller/StackApiController.php

+
+		$stack = $this->service->find($this->request->params['stackId']);
+
+		if ($stack == false || $stack == null) {


=== instead of ==

juliushaertl · 2018-07-15T10:24:13Z

lib/Controller/StackApiController.php

+
+		$stack = $this->service->delete($this->request->params['stackId']);		
+
+		if ($stack == false || $stack == null) {


=== instead of ==

juliushaertl · 2018-07-15T10:25:58Z

lib/Controller/Helper/ApiHelper.php

+			return $error;
+		}
+
+		return false;


Probably better to return an empty array then, so we can have proper return type hints once we have PHP 7.0 as a minimum.

juliushaertl · 2018-07-15T10:34:23Z

lib/Controller/CardApiController.php

+			return new DataResponse('card id must be a number', HTTP::STATUS_BAD_REQUEST);
+		}
+
+		$card = $this->cardService->find($this->request->params['cardId']);


Something more general regarding the service classes. They actually throw various exceptions that come from either the permissionService or the db mappers. Sorry those are not correctly documented in the phpDoc comments. So we need to either add a try/catch block here or extend the service methods to return null in case there is no enity found.

Another approach would be to have a middleware for the API that catches the exceptions and returns a 404 for DoesNotExistException for example.

Which would you prefer? I'm usually all for the try { catch } blocks as it makes the code that much more clear / self documenting, typically avoiding exactly this kind of scenario. I did a test for (false || null) just as a starting point.

All the services are able to throw these exceptions?

Yes, that makes sense. Let's go that way. Basically they all throw exceptions based on the mapper, but I can push some commit to add proper annotations later, if that helps.

juliushaertl · 2018-07-15T12:57:45Z

@Nebri I quickly pushed a commit to fix the one failing test.

codecov · 2018-07-15T13:05:16Z

Codecov Report

Merging #535 into master will decrease coverage by 3.65%.
The diff coverage is 61.09%.

@@            Coverage Diff             @@
##           master     #535      +/-   ##
==========================================
- Coverage   79.12%   75.46%   -3.66%     
==========================================
  Files          46       52       +6     
  Lines        1643     1969     +326     
==========================================
+ Hits         1300     1486     +186     
- Misses        343      483     +140

juliushaertl · 2018-07-15T13:09:37Z

@Nebri Any another one to add the exception annotations. At some point it would probably be a good idea to catch the database exceptions in the service classes and throw exceptions that better show what exactly went wrong.

E.g. for the CardServce::update method we currently have:
NoPermissionException which is fine
StatusException if the board or card is archived
MultipleObjectsReturnedException|DoesNotExistException if the query for the card goes wrong: Controllers don't need to know about that, a NotFoundException for the cardId would be the best here I guess.

Would that make sense from your POV?

NebriBlackwing · 2018-07-15T13:28:34Z

MultipleObjectsReturnedException|DoesNotExistException if the query for the card goes wrong: Controllers don't need to know about that, a NotFoundException for the cardId would be the best here I guess.

Yup That makes perfect sense to me! 👍

NebriBlackwing · 2018-07-15T16:20:11Z

@juliushaertl Mind taking a look over my commit: ede2144? Getting a bit lost in the weeds on mocking up a unit test that will cause the permissions to fail, and throw the permission exception. Once I figure that out, I'll be able to make a test to ensure the proper DataResponse is generated.

I commented out the code segment I'm struggling with, and labelled it as a place of dragons ;)

NebriBlackwing · 2018-07-16T09:13:09Z

@juliushaertl

The board service should throw a NoPermissionEyception for this test case. Since we're unit testing there is no need to check if the PermissionService method is called. We can just assume that this is handled correctly.

Is this true for all the exceptions that could be occuring in the rest api? I've taken a look at SharingMiddleware and I've noticed it's taking exceptions and returning out json responses.

if you look at the Board / Stack api controllers I've defined, I'm also trying to build the same json responses using the DataResponse class. Am I essentially creating duplicating code that I don't need to?

The rest api code branch so far is almost at 2,000 lines, and based on our discussions this is seeming to get out of hand. Want to make 100% sure I'm not wasting effort re-inventing the wheel here :).

juliushaertl · 2018-07-16T10:02:35Z

Is this true for all the exceptions that could be occuring in the rest api? I've taken a look at SharingMiddleware and I've noticed it's taking exceptions and returning out json responses.

Yes, at least for the NoPermissionException, which is a StatusException that is catched by the Middleware.

if you look at the Board / Stack api controllers I've defined, I'm also trying to build the same json responses using the DataResponse class. Am I essentially creating duplicating code that I don't need to?

You can get rid of those checks https://github.com/nextcloud/deck/pull/535/files#diff-aa1cba99409af542c6b24c8140e1415bR115

The middleware will automatically return a JSONResponse for any StatusException that is thrown in a controller.

We probably should also move the other checks to the services and just throw a StatusException for those as well, but I need to think about that a bit.

NebriBlackwing · 2018-07-16T10:18:20Z

Alright I'm going to pause work on this branch until we have a solid plan of attack for how to handle these exceptions, as it greatly affects the complexity and location of the relevant unit tests.

NebriBlackwing · 2018-07-17T02:48:27Z

@juliushaertl This is ready for another review round.

juliushaertl · 2018-07-18T19:34:20Z

lib/BadRequestException.php

+namespace OCA\Deck;
+
+use OCP\AppFramework\Http;
+class BadRequestException extends \Exception {


extend from StatusException so we don't need an additional check for that in the Middleware.

juliushaertl · 2018-07-18T19:34:44Z

lib/Middleware/SharingMiddleware.php

@@ -59,7 +60,7 @@ public function __construct(ILogger $logger, IConfig $config) {
 	 * @throws \Exception
 	 */
 	public function afterException($controller, $methodName, \Exception $exception) {
-		if ($exception instanceof StatusException) {
+		if ($exception instanceof StatusException || $exception instanceof BadRequestException) {


Not needed if BadRequestException is a StatusException.

juliushaertl · 2018-07-18T19:37:48Z

lib/Controller/LabelApiController.php

+	 * Create a new label
+	 */
+	public function create($title, $color) {
+		$label = $this->labelService->create($title, $color, $this->request->getParam('boardId'));


We still need some checks as in the other services here as well 😉

juliushaertl · 2018-07-18T19:51:33Z

@Nebri Looks good from the Service/Controller separation now.

Some things we still need endpoints for, or maybe we can adjust the current ones to offer an option for are:

Card: Assign/unassign labels
Card: Assign/unassign users
Card: reorder
Stacks: fetch archived
Attachments

I'd be fine if we obmit the following for the first version of the API:

Board: add/update/delete acl (the first api version should be fine without sharing/updating the permissions)

But it would of course nice to also have those 😉

NebriBlackwing · 2018-07-19T14:11:26Z

Of course, let's do it right! I have no intention of rushing this. I've been taking job interviews this week, next week is looking clear for the moment.

NebriBlackwing · 2018-07-24T18:54:25Z

@juliushaertl I've just written the attachments api. I'm assuming that we also want to go through the services to put in the BadRequestException across the board?

I'd also like to request that we do the acl permissions in a separate issue. I'd like to keep the scope creep in check. Let's fix anything that may still be outstanding here, get it merged, and then build the permissions on top.

let me know what you think.

juliushaertl · 2018-07-24T19:35:50Z

I'm assuming that we also want to go through the services to put in the BadRequestException across the board?

That would be good yes.

I'd also like to request that we do the acl permissions in a separate issue. I'd like to keep the scope creep in check. Let's fix anything that may still be outstanding here, get it merged, and then build the permissions on top.

Sure, we can do this later on. Maybe you can just create an issue for that, so we don't forget about it. That will also make it a bit easier to review then. Better have several small steps there 😉

srbaker · 2018-07-30T13:42:34Z

Heya, thanks for jumping on this. @Nebri Every time I had planned to work on it, something else came up, and I just got stuck. Really glad to see this moving along!