FriendsOfCake / crud-json-api

Build advanced JSON API Servers with almost no code.
https://crud-json-api.readthedocs.io/
MIT License
56 stars 32 forks source link

How to configure for new Relationships Action? #125

Closed geoidesic closed 4 years ago

geoidesic commented 4 years ago

I'm trying to get the new Relationships Action to work but I can't figure out the configuration and the docs haven't been updated for this option.

I had this when using my own custom Relationships Action:

$this->loadComponent('Crud.Crud', [
            'actions' => [
                'Crud.Index',
                'Crud.View',
                'add' => ['className' => '\App\Crud\Action\AddAction'],
                'Crud.Edit',
                'Crud.Delete',
                'Crud.Bulk',
                'Relationships' => ['className' => '\App\Crud\Action\RelationshipsAction'],
            ],

I tried the following config:

$this->loadComponent('Crud.Crud', [
            'actions' => [
                'Crud.Index',
                'Crud.View',
                'add' => ['className' => '\App\Crud\Action\AddAction'],
                'Crud.Edit',
                'Crud.Delete',
                'Crud.Bulk',
                'CrudJsonApi.Relationships'
            ],

... but I get errors like this

2020-09-03 15:36:08 Error: [TypeError] Argument 1 passed to Cake\ORM\Table::getAssociation() must be of the type string, null given, called in /Users/me/vendor/friendsofcake/crud-json-api/src/Action/RelationshipsAction.php on line 154 in /Users/me/vendor/cakephp/cakephp/src/ORM/Table.php on line 893
geoidesic commented 4 years ago

I don't think this is implemented correctly according to the 1.0 JSONAPI specification: https://github.com/FriendsOfCake/crud-json-api/blob/6707217ea643f3d3b3bd63647a80e7ed003de839/src/Action/RelationshipsAction.php#L356

This code uses the _findRelations method which expects a request parameter type. However the specification does not require this parameter, as per these two links: https://jsonapi.org/format/#crud-updating-to-one-relationships https://jsonapi.org/format/#crud-updating-to-many-relationships

In these cases, the type value is stored within the request data, not the request params.

I see a few problems with this:

  1. It looks like the current implementation only supports to-one-relationships
  2. It's all built as if per this part of the spec https://jsonapi.org/format/#document-resource-object-relationships, (i.e. expecting a type request parameter), which is the wrong part of the spec for this code to be conforming to.
  3. It assumes that each record sent in the data is of the same type, which doesn't conform to the spec.
geoidesic commented 4 years ago

Actually this was a routing issue: I hadn't updated my code to use JsonApiRoutes::mapModels. I think this is according to spec. Will do some more testing and then close this if that is the case.

geoidesic commented 4 years ago

I can't test this due to #126

geoidesic commented 4 years ago

I created a front-end middleware hack to work around #126. There seems to be a bug in the JsonApiRoutes::mapModels code though as $request->getParam('action'); is returning the UUID from the URL, which is:

http://{{domain}}/api/enquiries/b1c71787-84b9-4a51-a577-3fcae1e395f1/relationships/clients

Maybe this is because the crud-json-api route is declared inside a prefix?

Router::prefix('api', function (RouteBuilder $routes) {
    JsonApiRoutes::mapModels(['Enquiries'], $routes);

Looking at the routes that have been added I can see that there are crud-json-api relationships added for personId but not for clients. So I think there is a bug in the JsonApiRoutes::mapModels code.

dakota commented 4 years ago

I don't think this is implemented correctly according to the 1.0 JSONAPI specification:

https://github.com/FriendsOfCake/crud-json-api/blob/6707217ea643f3d3b3bd63647a80e7ed003de839/src/Action/RelationshipsAction.php#L356

This code uses the _findRelations method which expects a request parameter type. However the specification does not require this parameter, as per these two links: jsonapi.org/format/#crud-updating-to-one-relationships jsonapi.org/format/#crud-updating-to-many-relationships

In these cases, the type value is stored within the request data, not the request params.

I see a few problems with this:

  1. It looks like the current implementation only supports to-one-relationships
  2. It's all built as if per this part of the spec jsonapi.org/format/#document-resource-object-relationships, (i.e. expecting a type request parameter), which is the wrong part of the spec for this code to be conforming to.
  3. It assumes that each record sent in the data is of the same type, which doesn't conform to the spec.

The type parameter refers directly to the last path element of the route. The name of the parameter is probably problematic since it clashes with the use of type in the JSONAPI Spec (Also, just a btw, this plugin follows the newer 1.1 spec)

geoidesic commented 4 years ago

I think the issue is in router::connect: https://github.com/FriendsOfCake/crud-json-api/blob/6707217ea643f3d3b3bd63647a80e7ed003de839/src/Route/JsonApiRoutes.php#L86

The value being passed in as $base to ::connect is:

Array
(
    [controller] => Enquiries
    [action] => relationships
    [type] => Clients
)

but the route created is to personId instead of clients:

[48] => Cake\Routing\Route\DashedRoute Object
(
    [_inflectedDefaults:protected] => 
    [keys] => Array
        (
        )

    [options] => Array
        (
            [_ext] => Array
                (
                    [0] => json
                )

            [routeClass] => Cake\Routing\Route\DashedRoute
        )

    [defaults] => Array
        (
            [controller] => Enquiries
            [action] => relationships
            [type] => Clients
            [_method] => PATCH
            [prefix] => Api
            [plugin] => 
        )

    [template] => /api/enquiries/:enquiry_id/relationships/personId
dakota commented 4 years ago

What does your association look like? The property name is used as the key for the association.

geoidesic commented 4 years ago
$this->belongsToMany('Clients', ['through' => 'EnquiryContacts', 'className' => 'People', 'conditions' => ['role_id' => 3], 'foreignKey' => 'enquiry_id', 'propertyName' => 'person_id']);

On EnquiriesTable

geoidesic commented 4 years ago

Maybe it's because of propertyName but without that the route expects a client_id property in the request body (which of course can't be provided as it's a through association and the clients table doesn't exist).

dakota commented 4 years ago

'propertyName' => 'person_id' <- That's why. In PHP code you'd access the client entity with $enquiry->person_id. So the plugin uses that as your relationship name, as per CakePHP convention.

dakota commented 4 years ago

What does the route look like without the propertyName option?

geoidesic commented 4 years ago

Without it the route is correct, but then the problem is that it expects the client_id property in the request body.

[36] => Cake\Routing\Route\DashedRoute Object
(
    [_inflectedDefaults:protected] => 
    [keys] => Array
        (
        )

    [options] => Array
        (
            [_ext] => Array
                (
                    [0] => json
                )

            [routeClass] => Cake\Routing\Route\DashedRoute
        )

    [defaults] => Array
        (
            [controller] => Enquiries
            [action] => relationships
            [type] => Clients
            [_method] => PATCH
            [prefix] => Api
            [plugin] => 
        )

    [template] => /api/enquiries/:enquiry_id/relationships/clients
geoidesic commented 4 years ago

So the error is then:

2020-09-04 08:36:42 Error: [PDOException] SQLSTATE[42S22]: Column not found: 1054 Unknown column 'client_id' in 'where clause' in /Users/me/vendor/cakephp/cakephp/src/Database/Statement/MysqlStatement.php on line 39

Stack trace:

Stack Trace:
- /Users/me/vendor/cakephp/cakephp/src/Database/Statement/MysqlStatement.php:39
- /Users/me/vendor/cakephp/cakephp/src/Database/Statement/StatementDecorator.php:178
- /Users/me/vendor/cakephp/cakephp/src/Database/Log/LoggingStatement.php:74
- /Users/me/vendor/cakephp/cakephp/src/Database/Connection.php:341
- /Users/me/vendor/cakephp/cakephp/src/Core/Retry/CommandRetry.php:70
- /Users/me/vendor/cakephp/cakephp/src/Database/Connection.php:344
- /Users/me/vendor/cakephp/cakephp/src/Database/Query.php:249
- /Users/me/vendor/cakephp/cakephp/src/ORM/Query.php:1112
- /Users/me/vendor/cakephp/cakephp/src/Datasource/QueryTrait.php:292
- /Users/me/vendor/cakephp/cakephp/src/ORM/Query.php:1059
- /Users/me/vendor/cakephp/cakephp/src/ORM/Query.php:1335
- json_encode - [internal], line ??- /Users/me/vendor/friendsofcake/crud-json-api/src/Error/JsonApiExceptionRenderer.php:198
- /Users/me/vendor/friendsofcake/crud-json-api/src/Error/JsonApiExceptionRenderer.php:68
- /Users/me/src/Error/AppExceptionRenderer.php:29
- /Users/me/vendor/cakephp/cakephp/src/Error/ExceptionRenderer.php:248
- /Users/me/vendor/cakephp/cakephp/src/Error/Middleware/ErrorHandlerMiddleware.php:142
- /Users/me/vendor/cakephp/cakephp/src/Error/Middleware/ErrorHandlerMiddleware.php:125
- /Users/me/vendor/cakephp/cakephp/src/Http/Runner.php:73
- /Users/me/vendor/cakephp/debug_kit/src/Middleware/DebugKitMiddleware.php:60
- /Users/me/vendor/cakephp/cakephp/src/Http/Runner.php:73
- /Users/me/vendor/cakephp/cakephp/src/Http/Runner.php:58
- /Users/me/vendor/cakephp/cakephp/src/Http/Server.php:90
- /Users/me/webroot/index.php:40
geoidesic commented 4 years ago

So there needs to be some way of telling the route or the association which field on the through table is the id for the final table (which in this case is PeopleTable). My understanding from reading the Association documentation is that this is the purpose of the propertyName config option.

It seems that if we don't use this then it breaks. If we do then the JSONAPI spec is broken as the route will not be as per the spec. Catch 22?

geoidesic commented 4 years ago

Do you think this is an upstream bug? Like maybe in CRUD or in CakePHP itself? Or is it something that needs to be overridden at the crud-json-api level? Or is it just a misconfiguration?

It seems strange to me that a through association would expect a property named after the association, as obviously a through association's name is not going to be an actual table.

Or maybe it's as simple as my schema for the through table not matching conventions? Here's the schema for the through table:

$this->table('enquiry_contacts')
            ->addColumn('enquiry_id', 'uuid')
            ->addColumn('person_id', 'uuid')
            ->addColumn('role_id', 'uuid')
            ->addColumn('capacity', 'string', ['null' => true])
            ->addColumn('created', 'datetime')
            ->addColumn('modified', 'datetime')
            ->create();

Maybe I should rename the person_id field to client_id?

geoidesic commented 4 years ago

Although I don't see how that would help as regardless it's the request that is failing validation and JSONAPI spec doesn't require there to be a client_id included in the request.

I'm not sure what's generating the error. From the stack trace it doesn't seem to be even reaching the plugins, so it's failing at the CakePHP level?

dakota commented 4 years ago

So there needs to be some way of telling the route or the association which field on the through table is the id for the final table (which in this case is PeopleTable). My understanding from reading the Association documentation is that this is the purpose of the propertyName config option.

It seems that if we don't use this then it breaks. If we do then the JSONAPI spec is broken as the route will not be as per the spec. Catch 22?

Ah, I see the misunderstanding :) What you are looking for is targetForeignKey. propertyName is what the property on the current entity, the foreign entity will be hydrated too.

geoidesic commented 4 years ago

Oh. Ok, didn't see that in the docs and the docs aren't very clear about that, so thanks! However that causes a different error then:

2020-09-04 08:55:42 Error: [RuntimeException] You cannot call all() on a non-select query. Use execute() instead. in /Users/me/vendor/cakephp/cakephp/src/ORM/Query.php on line 1054
Stack Trace:
- /Users/me/vendor/cakephp/cakephp/src/ORM/Query.php:1335
- json_encode - [internal], line ??- /Users/me/vendor/friendsofcake/crud-json-api/src/Error/JsonApiExceptionRenderer.php:198
- /Users/me/vendor/friendsofcake/crud-json-api/src/Error/JsonApiExceptionRenderer.php:68
- /Users/me/src/Error/AppExceptionRenderer.php:29
- /Users/me/vendor/cakephp/cakephp/src/Error/ExceptionRenderer.php:248
- /Users/me/vendor/cakephp/cakephp/src/Error/Middleware/ErrorHandlerMiddleware.php:142
- /Users/me/vendor/cakephp/cakephp/src/Error/Middleware/ErrorHandlerMiddleware.php:125
- /Users/me/vendor/cakephp/cakephp/src/Http/Runner.php:73
- /Users/me/vendor/cakephp/debug_kit/src/Middleware/DebugKitMiddleware.php:60
- /Users/me/vendor/cakephp/cakephp/src/Http/Runner.php:73
- /Users/me/vendor/cakephp/cakephp/src/Http/Runner.php:58
- /Users/me/vendor/cakephp/cakephp/src/Http/Server.php:90
- /Users/me/webroot/index.php:40

Request URL: /api/enquiries/b1c71787-84b9-4a51-a577-3fcae1e395f1/relationships/clients
Referer URL: http://localhost:8000
Client IP: 127.0.0.1
geoidesic commented 4 years ago

Which looks like the kind of error that is probably masking another, underlying error.

geoidesic commented 4 years ago

Ok found the underlying error:

2020-09-04 09:02:06 Error: [PDOException] SQLSTATE[HY000]: General error: 1364 Field 'role_id' doesn't have a default value in /Users/me/vendor/cakephp/cakephp/src/Database/Statement/MysqlStatement.php on line 39

I would have thought that because the association defines a static value for role_id that this would automatically be filled in by the ORM. But I guess not.

geoidesic commented 4 years ago

I guess I could create a $this->Crud->on('beforeSave' event listener that could check if it's receiving that type of request and fill in the correct default based on the association name.

Is that the best-practice way?

dakota commented 4 years ago

Yeah, the ORM doesn't automatically fill in values that the association is filtered by. Doing a beforeSave is what I do.

geoidesic commented 4 years ago

Where would that go in this instance? In EnquiriesController? I tried but it's erring before getting to the controller it seems. E.g.:

class EnquiriesController extends BaseEnquiriesController
{
    public function relationships()
    {
        die('o');  // <-- doesn't reach this code before error
    }
dakota commented 4 years ago

I normally do it in the beforeFilter

geoidesic commented 4 years ago

Ok thx. I'm having a go but I realise I don't know what to put in the event. How would I set that default value?

geoidesic commented 4 years ago

The entity being processed seems to be the Enquiry object. But I'm trying to set a value in an associated entity; not sure how to do that.

dakota commented 4 years ago

Something like

public function beforeFilter(EventInterface $event)
{

    if ($this->request->getParam('action') === 'relationships') {
        $this->Crud->on('Crud.beforeSave', function (Event $event) {
            $enquiry = $event->getSubject()->entity;

            $enquiry->clients[0]->_joinData->role_id = 1; //Look over them here.
        });
    }

    return parent::beforeFilter($event);
}

see https://book.cakephp.org/4/en/orm/saving-data.html#saving-belongstomany-associations for more info

geoidesic commented 4 years ago

Thanks. I don't understand this however. I've put that in but still get the same error plus a warning now.

2020-09-04 09:39:59 Warning: Warning (2): Creating default object from empty value in [/Users/me/src/Controller/Api/EnquiriesController.php, line 28]

2020-09-04 09:39:59 Error: [PDOException] SQLSTATE[HY000]: General error: 1364 Field 'role_id' doesn't have a default value in /Users/me/vendor/cakephp/cakephp/src/Database/Statement/MysqlStatement.php on line 39
geoidesic commented 4 years ago

Ok nvm. Your docs link was good :) This works:

    public function beforeFilter(EventInterface $event)
    {
        if ($this->request->getParam('action') === 'relationships') {
            $this->Crud->on('Crud.beforeSave', function (\Cake\Event\Event $event) {
                $enquiry = $event->getSubject()->entity;
                foreach ($enquiry->clients as $key => $client) {
                    if (empty($client->_joinData)) {
                        $client->_joinData = new Entity(['role_id' => 3]);
                    } else {
                        $client->_joinData->role_id = 3;
                    }
                }
            });
        }

        return parent::beforeFilter($event);
    }