-
Notifications
You must be signed in to change notification settings - Fork 146
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.
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
Add json api query support and include support #508
Add json api query support and include support #508
Conversation
Epic PR 💯 FWIW I am getting the exact same Travis fails here #506 and haven't been able to figure out what causes it yet. So far I see no differences between the previously successful builds and these all-of-a-sudden-fails (I am using the 5.6 to troubleshoot) |
I think it should be possible / required to whitelist the associations that you can pass into |
I definately agree, and will be adding that. Wanted to get the PR open in the meantime |
@dakota since this got discussed on Slack... will this include full sparse fieldsets functionality?
|
@bravo-kernel It does not, and will not, BUT it lays the foundation to add that functionality, as well as filtering. I will be working on that next, as a separate PR. Don't want to introduce too much in a single PR |
I thought so by looking at the code but just wanted to make sure, thanks |
Codecov Report
@@ Coverage Diff @@
## master #508 +/- ##
============================================
- Coverage 94.72% 94.36% -0.36%
- Complexity 614 649 +35
============================================
Files 42 42
Lines 1913 1989 +76
============================================
+ Hits 1812 1877 +65
- Misses 101 112 +11
Continue to review full report at Codecov.
|
777c80c
to
1d9b76e
Compare
UPDATE: ALL RESOLVEDI ❤️ it. I don't have time to complete a full integration test but here's some initial feedback to start with. All tested against my OK: including a single hasMany relationshipTested against
{
"data": [
{
"type": "countries",
"id": "1",
"attributes": {
"code": "NL",
"name": "The Netherlands",
"native": "Nederland",
"slug": "nl",
"approved": true,
"created": "2017-02-25T20:40:40+00:00",
"modified": "2017-02-25T20:40:40+00:00"
},
"relationships": {
"cultures": {
"data": [
{
"type": "cultures",
"id": "1"
}
],
"links": {
"self": "/cultures?country_id=1"
}
}
},
"links": {
"self": "/countries/1"
}
}
],
"included": [
{
"type": "cultures",
"id": "1",
"attributes": {
"code": "nl-NL",
"name": "Dutch",
"native": "Nederlands",
"slug": "nl-nl",
"country_id": 1,
"approved": true,
"created": "2017-02-25T20:40:40+00:00",
"modified": "2017-02-25T20:40:40+00:00"
}
}
]
} OK: including a single belongsTo relationshipTested with `/countries?include=currencies
{
"data": [
{
"type": "countries",
"id": "1",
"attributes": {
"code": "NL",
"name": "The Netherlands",
"native": "Nederland",
"slug": "nl",
"approved": true,
"created": "2017-02-25T20:40:40+00:00",
"modified": "2017-02-25T20:40:40+00:00"
},
"relationships": {
"currency": {
"data": {
"type": "currencies",
"id": "1"
},
"links": {
"self": "/currencies/1"
}
}
},
"links": {
"self": "/countries/1"
}
}
],
"included": [
{
"type": "currencies",
"id": "1",
"attributes": {
"code": "EUR",
"name": "Euro",
"slug": "eur",
"approved": true,
"created": "2017-02-25T20:40:40+00:00",
"modified": "2017-02-25T20:40:40+00:00"
}
}
]
} OK: including two relationships (hasMany-and-belongsTo)Tested with
{
"data": [
{
"type": "countries",
"id": "1",
"attributes": {
"code": "NL",
"name": "The Netherlands",
"native": "Nederland",
"slug": "nl",
"approved": true,
"created": "2017-02-25T20:40:40+00:00",
"modified": "2017-02-25T20:40:40+00:00"
},
"relationships": {
"currency": {
"data": {
"type": "currencies",
"id": "1"
},
"links": {
"self": "/currencies/1"
}
},
"cultures": {
"data": [
{
"type": "cultures",
"id": "1"
}
],
"links": {
"self": "/cultures?country_id=1"
}
}
},
"links": {
"self": "/countries/1"
}
}
],
"included": [
{
"type": "currencies",
"id": "1",
"attributes": {
"code": "EUR",
"name": "Euro",
"slug": "eur",
"approved": true,
"created": "2017-02-25T20:40:40+00:00",
"modified": "2017-02-25T20:40:40+00:00"
}
},
{
"type": "cultures",
"id": "1",
"attributes": {
"code": "nl-NL",
"name": "Dutch",
"native": "Nederlands",
"slug": "nl-nl",
"approved": true,
"created": "2017-02-25T20:40:40+00:00",
"modified": "2017-02-25T20:40:40+00:00"
}
}
]
} OK: limiting results when a single belongsTo contain is present in index actionTested with public function index()
{
$this->Crud->on('beforePaginate', function (Event $event) {
$event->subject()->query->contain([
'Cultures',
]);
});
return $this->Crud->execute();
}
{
"data": [
{
"type": "countries",
"id": "1",
"attributes": {
"code": "NL",
"name": "The Netherlands",
"native": "Nederland",
"slug": "nl",
"approved": true,
"created": "2017-02-25T20:40:40+00:00",
"modified": "2017-02-25T20:40:40+00:00"
},
"relationships": {
"currency": {
"data": {
"type": "currencies",
"id": "1"
},
"links": {
"self": "/currencies/1"
}
},
"cultures": {
"data": [
{
"type": "cultures",
"id": "1"
}
],
"links": {
"self": "/cultures?country_id=1"
}
}
},
"links": {
"self": "/countries/1"
}
}
],
"included": [
{
"type": "currencies",
"id": "1",
"attributes": {
"code": "EUR",
"name": "Euro",
"slug": "eur",
"approved": true,
"created": "2017-02-25T20:40:40+00:00",
"modified": "2017-02-25T20:40:40+00:00"
}
}
]
} OK: limiting results when a single hasMany contain is present in index actionTested with public function index()
{
$this->Crud->on('beforePaginate', function (Event $event) {
$event->subject()->query->contain([
'Currencies',
]);
});
return $this->Crud->execute();
}
{
"data": [
{
"type": "countries",
"id": "1",
"attributes": {
"code": "NL",
"name": "The Netherlands",
"native": "Nederland",
"slug": "nl",
"approved": true,
"created": "2017-02-25T20:40:40+00:00",
"modified": "2017-02-25T20:40:40+00:00"
},
"relationships": {
"currency": {
"data": {
"type": "currencies",
"id": "1"
},
"links": {
"self": "/currencies/1"
}
},
"cultures": {
"data": [
{
"type": "cultures",
"id": "1"
}
],
"links": {
"self": "/cultures?country_id=1"
}
}
},
"links": {
"self": "/countries/1"
}
}
],
"included": [
{
"type": "cultures",
"id": "1",
"attributes": {
"code": "nl-NL",
"name": "Dutch",
"native": "Nederlands",
"slug": "nl-nl",
"approved": true,
"created": "2017-02-25T20:40:40+00:00",
"modified": "2017-02-25T20:40:40+00:00"
}
}
]
} OK: limiting results when a two contains are present in index actionSame result as the previous test. This time tested with
public function index()
{
$this->Crud->on('beforePaginate', function (Event $event) {
$event->subject()->query->contain([
'Cultures',
'Currencies',
]);
});
return $this->Crud->execute();
} UsabilityI'm ashamed to admit but I was unable to figure out (from the stack trace) which one of the three resources in "errors": [
{
"code": "400",
"title": "Bad Request",
"detail": "Invalid relationship path supplied in include parameter" Update: Thanks, this feedback is much much better It only detects the first invalid so {
"errors": [
{
"code": "400",
"title": "Bad Request",
"detail": "Invalid relationship path 'donkey' supplied in include parameter" |
I've added a few more tests.
|
b9fc0a2
to
9aebb9d
Compare
@bravo-kernel Made quite a few changes now. Firstly, all your supplied test cases are working (With integration tests 🎉 ) I have also started work on supporting arbitrarily deeply nested includes/contains. Will get that finished soon. |
As a consequence this should also make nested relationships work
UPDATE: RESOLVEDListener configI copy-pasted the following from your docs example and it is giving me this error (cake 3.3.9);
public function index()
{
$this->Crud
->listener('jsonApi')
->setConfig('queryParameters.include.blacklist', ['countries']);
$this->Crud->on('beforePaginate', function (Event $event) {
});
return $this->Crud->execute();
} Stack trace{
"errors": [
{
"code": "500",
"title": "Internal Server Error",
"detail": "Call to undefined method Crud\\Listener\\JsonApiListener::setConfig()"
}
],
"debug": {
"class": "Error",
"trace": [
{
"file": "/home/vagrant/projects/alt3-api/vendor/friendsofcake/crud/src/Controller/ControllerTrait.php",
"line": 51,
"function": "call_user_func_array",
"args": [
[
{
"paginate": {
"sortWhitelist": [
"code",
"name"
]
},
"name": "Currencies",
"helpers": [],
"request": {
"params": {
"plugin": null,
"controller": "Currencies",
"action": "index",
"_ext": null,
"pass": [],
"_method": "GET",
"_matchedRoute": "/currencies",
"isAjax": false
},
"data": [],
"query": [],
"cookies": {
"CAKEPHP": "a93dsl3393955f5lphabses856",
"csrfToken": "0d2c72f16a2aaabf9a38f18fa8974f66a2d87967"
},
"url": "currencies",
"base": "",
"webroot": "/",
"here": "/currencies",
"trustProxy": false
},
"response": {},
"autoRender": true,
"components": [],
"View": null,
"plugin": null,
"passedArgs": [],
"modelClass": "Currencies",
"viewClass": null,
"viewVars": [],
"dispatchComponents": {
"Crud": true
},
"RequestHandler": {
"enabled": true,
"response": {},
"ext": null,
"request": {
"params": {
"plugin": null,
"controller": "Currencies",
"action": "index",
"_ext": null,
"pass": [],
"_method": "GET",
"_matchedRoute": "/currencies",
"isAjax": false
},
"data": [],
"query": [],
"cookies": {
"CAKEPHP": "a93dsl3393955f5lphabses856",
"csrfToken": "0d2c72f16a2aaabf9a38f18fa8974f66a2d87967"
},
"url": "currencies",
"base": "",
"webroot": "/",
"here": "/currencies",
"trustProxy": false
},
"components": []
},
"Crud": {
"request": {
"params": {
"plugin": null,
"controller": "Currencies",
"action": "index",
"_ext": null,
"pass": [],
"_method": "GET",
"_matchedRoute": "/currencies",
"isAjax": false
},
"data": [],
"query": [],
"cookies": {
"CAKEPHP": "a93dsl3393955f5lphabses856",
"csrfToken": "0d2c72f16a2aaabf9a38f18fa8974f66a2d87967"
},
"url": "currencies",
"base": "",
"webroot": "/",
"here": "/currencies",
"trustProxy": false
},
"response": {},
"components": []
},
"Auth": {
"components": [
"RequestHandler",
"Flash"
],
"allowedActions": [
"home",
"index",
"view",
"index",
"view"
],
"request": {
"params": {
"plugin": null,
"controller": "Currencies",
"action": "index",
"_ext": null,
"pass": [],
"_method": "GET",
"_matchedRoute": "/currencies",
"isAjax": false
},
"data": [],
"query": [],
"cookies": {
"CAKEPHP": "a93dsl3393955f5lphabses856",
"csrfToken": "0d2c72f16a2aaabf9a38f18fa8974f66a2d87967"
},
"url": "currencies",
"base": "",
"webroot": "/",
"here": "/currencies",
"trustProxy": false
},
"response": {},
"session": {}
},
"Alt3Api": {
"request": {
"params": {
"plugin": null,
"controller": "Currencies",
"action": "index",
"_ext": null,
"pass": [],
"_method": "GET",
"_matchedRoute": "/currencies",
"isAjax": false
},
"data": [],
"query": [],
"cookies": {
"CAKEPHP": "a93dsl3393955f5lphabses856",
"csrfToken": "0d2c72f16a2aaabf9a38f18fa8974f66a2d87967"
},
"url": "currencies",
"base": "",
"webroot": "/",
"here": "/currencies",
"trustProxy": false
},
"response": {},
"components": []
},
"Prg": {
"request": {
"params": {
"plugin": null,
"controller": "Currencies",
"action": "index",
"_ext": null,
"pass": [],
"_method": "GET",
"_matchedRoute": "/currencies",
"isAjax": false
},
"data": [],
"query": [],
"cookies": {
"CAKEPHP": "a93dsl3393955f5lphabses856",
"csrfToken": "0d2c72f16a2aaabf9a38f18fa8974f66a2d87967"
},
"url": "currencies",
"base": "",
"webroot": "/",
"here": "/currencies",
"trustProxy": false
},
"response": {},
"components": []
}
},
"index"
],
[]
]
},
{
"file": "/home/vagrant/projects/alt3-api/vendor/cakephp/cakephp/src/Http/ActionDispatcher.php",
"line": 122,
"function": "invokeAction",
"class": "App\\Controller\\AppController",
"type": "->",
"args": []
},
{
"file": "/home/vagrant/projects/alt3-api/vendor/cakephp/cakephp/src/Http/ActionDispatcher.php",
"line": 96,
"function": "_invoke",
"class": "Cake\\Http\\ActionDispatcher",
"type": "->",
"args": [
{
"paginate": {
"sortWhitelist": [
"code",
"name"
]
},
"name": "Currencies",
"helpers": [],
"request": {
"params": {
"plugin": null,
"controller": "Currencies",
"action": "index",
"_ext": null,
"pass": [],
"_method": "GET",
"_matchedRoute": "/currencies",
"isAjax": false
},
"data": [],
"query": [],
"cookies": {
"CAKEPHP": "a93dsl3393955f5lphabses856",
"csrfToken": "0d2c72f16a2aaabf9a38f18fa8974f66a2d87967"
},
"url": "currencies",
"base": "",
"webroot": "/",
"here": "/currencies",
"trustProxy": false
},
"response": {},
"autoRender": true,
"components": [],
"View": null,
"plugin": null,
"passedArgs": [],
"modelClass": "Currencies",
"viewClass": null,
"viewVars": [],
"dispatchComponents": {
"Crud": true
},
"RequestHandler": {
"enabled": true,
"response": {},
"ext": null,
"request": {
"params": {
"plugin": null,
"controller": "Currencies",
"action": "index",
"_ext": null,
"pass": [],
"_method": "GET",
"_matchedRoute": "/currencies",
"isAjax": false
},
"data": [],
"query": [],
"cookies": {
"CAKEPHP": "a93dsl3393955f5lphabses856",
"csrfToken": "0d2c72f16a2aaabf9a38f18fa8974f66a2d87967"
},
"url": "currencies",
"base": "",
"webroot": "/",
"here": "/currencies",
"trustProxy": false
},
"components": []
},
"Crud": {
"request": {
"params": {
"plugin": null,
"controller": "Currencies",
"action": "index",
"_ext": null,
"pass": [],
"_method": "GET",
"_matchedRoute": "/currencies",
"isAjax": false
},
"data": [],
"query": [],
"cookies": {
"CAKEPHP": "a93dsl3393955f5lphabses856",
"csrfToken": "0d2c72f16a2aaabf9a38f18fa8974f66a2d87967"
},
"url": "currencies",
"base": "",
"webroot": "/",
"here": "/currencies",
"trustProxy": false
},
"response": {},
"components": []
},
"Auth": {
"components": [
"RequestHandler",
"Flash"
],
"allowedActions": [
"home",
"index",
"view",
"index",
"view"
],
"request": {
"params": {
"plugin": null,
"controller": "Currencies",
"action": "index",
"_ext": null,
"pass": [],
"_method": "GET",
"_matchedRoute": "/currencies",
"isAjax": false
},
"data": [],
"query": [],
"cookies": {
"CAKEPHP": "a93dsl3393955f5lphabses856",
"csrfToken": "0d2c72f16a2aaabf9a38f18fa8974f66a2d87967"
},
"url": "currencies",
"base": "",
"webroot": "/",
"here": "/currencies",
"trustProxy": false
},
"response": {},
"session": {}
},
"Alt3Api": {
"request": {
"params": {
"plugin": null,
"controller": "Currencies",
"action": "index",
"_ext": null,
"pass": [],
"_method": "GET",
"_matchedRoute": "/currencies",
"isAjax": false
},
"data": [],
"query": [],
"cookies": {
"CAKEPHP": "a93dsl3393955f5lphabses856",
"csrfToken": "0d2c72f16a2aaabf9a38f18fa8974f66a2d87967"
},
"url": "currencies",
"base": "",
"webroot": "/",
"here": "/currencies",
"trustProxy": false
},
"response": {},
"components": []
},
"Prg": {
"request": {
"params": {
"plugin": null,
"controller": "Currencies",
"action": "index",
"_ext": null,
"pass": [],
"_method": "GET",
"_matchedRoute": "/currencies",
"isAjax": false
},
"data": [],
"query": [],
"cookies": {
"CAKEPHP": "a93dsl3393955f5lphabses856",
"csrfToken": "0d2c72f16a2aaabf9a38f18fa8974f66a2d87967"
},
"url": "currencies",
"base": "",
"webroot": "/",
"here": "/currencies",
"trustProxy": false
},
"response": {},
"components": []
}
}
]
},
{
"file": "/home/vagrant/projects/alt3-api/vendor/cakephp/cakephp/src/Routing/Dispatcher.php",
"line": 60,
"function": "dispatch",
"class": "Cake\\Http\\ActionDispatcher",
"type": "->",
"args": [
{
"params": {
"plugin": null,
"controller": "Currencies",
"action": "index",
"_ext": null,
"pass": [],
"_method": "GET",
"_matchedRoute": "/currencies",
"isAjax": false
},
"data": [],
"query": [],
"cookies": {
"CAKEPHP": "a93dsl3393955f5lphabses856",
"csrfToken": "0d2c72f16a2aaabf9a38f18fa8974f66a2d87967"
},
"url": "currencies",
"base": "",
"webroot": "/",
"here": "/currencies",
"trustProxy": false
},
{}
]
},
{
"file": "/home/vagrant/projects/alt3-api/webroot/index.php",
"line": 36,
"function": "dispatch",
"class": "Cake\\Routing\\Dispatcher",
"type": "->",
"args": [
{
"params": {
"plugin": null,
"controller": "Currencies",
"action": "index",
"_ext": null,
"pass": [],
"_method": "GET",
"_matchedRoute": "/currencies",
"isAjax": false
},
"data": [],
"query": [],
"cookies": {
"CAKEPHP": "a93dsl3393955f5lphabses856",
"csrfToken": "0d2c72f16a2aaabf9a38f18fa8974f66a2d87967"
},
"url": "currencies",
"base": "",
"webroot": "/",
"here": "/currencies",
"trustProxy": false
},
{}
]
}
]
}
} |
@bravo-kernel Probably because you are using CakePHP 3.3 still :) Will get that updated |
UPDATE: EXPLAINED (WORKS FOR ME)The blacklist works as expected except... when there already is a public function index()
{
$this->Crud
->listener('jsonApi')
->config('queryParameters.include.blacklist', ['currencies', 'cultures']);
$this->Crud->on('beforePaginate', function (Event $event) {
$event->subject()->query->contain([
'Currencies',
'Cultures'
]);
});
return $this->Crud->execute();
} |
UPDATE: SOLVEDJust tested the However, I did notice that the include does not accept/respect my dasherized models and throws the following error when using the expected
Changing the include to underscored {
"data": {
"type": "users",
"id": "1",
"attributes": {
"email": "bravo@kernel.com",
"username": null,
"slug": null,
"confirmed": false,
"created": "2017-03-08T14:32:31+00:00",
"modified": "2017-03-08T14:32:31+00:00"
},
"relationships": {
"user_profile": {
"data": {
"type": "user_profiles",
"id": "333"
},
"links": {
"self": "/user-profiles/view/333"
}
}
},
"links": {
"self": "/users/1"
}
},
"included": [
{
"type": "user_profiles",
"id": "333",
"attributes": {
"newsletter": false,
"notifications": false,
"created": "2017-03-08T14:32:31+00:00",
"modified": "2017-03-08T14:32:31+00:00"
}
}
]
} |
This is the expected behaviour. The blacklist/whitelist ONLY affects the |
Thanks for explaining mr. @dakota. Since there are no more things on my list I would say this is functionally good to go. Awesome job 💯 |
If everybody is happy, I'll go ahead and merge? |
Thanks @dakota ! |
👏 |
Does this warrant a new minor release or a bugfix release should suffice? |
Depends on how strict we are about feature additions? It doesn't really change how the default behaviour worked |
This adds support to the json api for parsing query parameters. Included as part of this is ability to pass the
include
query parameter to conditionally load relationships.