feat: support repeatable directive #53
Conversation
lib/auth.js
Outdated
| const authDirective = astNode.directives.filter(directive => directive.name.value === this[kAuthDirective]) | ||
| if (authDirective.length === 1) { | ||
| return authDirective[0] | ||
| } else if (authDirective.length > 1) { |
There was a problem hiding this comment.
In order to avoid to introduce breaking changes I've returned an array only when the directive ie repeted.
I could make this check more smart and return an Array when a directory is repeatable.
This way the getPolicy result would be become more predictable I think.
WDYT?
There was a problem hiding this comment.
I looked a bit and the getPolicy function is used internally, so there should not be any API breaking
Line 16 in 4fc2b1b
I think we should test this case: the user adds the directive multiple times but it is not a repeatable one. I would expect this case will be already manage by mercurius-core and this module will not have to deal with this case, but it worth testing it
There was a problem hiding this comment.
Since there are no breaking changes i will also make this function return Array | null so everythink would become simpler.
When a non repeatable directive is added multiple times the graphql/validation library throws The directive "@counter" can only be used once at this location.
Do you think i should add a unit test for this?
There was a problem hiding this comment.
I think it is worth it.
Note that the CI is failing due to the .? syntax
There was a problem hiding this comment.
Added the new test case and fixed the test for Node 12
lib/auth.js
Outdated
| const authDirective = astNode.directives.filter(directive => directive.name.value === this[kAuthDirective]) | ||
| if (authDirective.length === 1) { | ||
| return authDirective[0] | ||
| } else if (authDirective.length > 1) { |
There was a problem hiding this comment.
I looked a bit and the getPolicy function is used internally, so there should not be any API breaking
Line 16 in 4fc2b1b
I think we should test this case: the user adds the directive multiple times but it is not a repeatable one. I would expect this case will be already manage by mercurius-core and this module will not have to deal with this case, but it worth testing it
lib/filter-schema.js
Outdated
| promises.push(policyFunction(item, null, null, context, info)) | ||
| } | ||
|
|
||
| for (const result of await Promise.all(promises)) { |
There was a problem hiding this comment.
I would execute the policies serially.
From the standard:
The order in which directives appear may be significant, including repeatable directives.
| @@ -9,11 +9,12 @@ const mercuriusAuth = require('..') | |||
| const schema = ` | |||
| directive @auth on OBJECT | FIELD_DEFINITION | |||
| directive @hasRole (role: String!) on OBJECT | FIELD_DEFINITION | |||
| directive @hasPermission (grant: String!) on OBJECT | FIELD_DEFINITION | |||
| directive @hasPermission (grant: String!) repeatable on OBJECT | FIELD_DEFINITION | |||
There was a problem hiding this comment.
I think this option needs a test in the gateway feature too
There was a problem hiding this comment.
I've added a couple of tests cases for this
lib/filter-schema.js
Outdated
| if (canShowDirectiveField instanceof Error || !canShowDirectiveField) { | ||
| canShowDirectiveField = false | ||
|
|
||
| if (Array.isArray(fieldPolicy)) { |
There was a problem hiding this comment.
This check should be done here too
https://github.com/mercurius-js/auth/blob/main/lib/auth.js#L113
It uses the policy structure to add the logic.
To spot this issue, you need to add a simple usage test of the repeatable directive that hit a real query a not the introspection one
There was a problem hiding this comment.
I've fixed the method and added some extra test cases. Thanks for pointing me in the right direction.
|
@jonnydgreen could you take a look? |
|
@mcollina did you wanna do the release or shall I? |
|
go for it! |
fixes #50