allow formatErrorMessage callback #3
Conversation
src/index.js
Outdated
| @@ -149,7 +149,9 @@ export function createComplexityLimitRule(maxCost, options = {}) { | |||
| } | |||
|
|
|||
| if (node.kind === 'Document') { | |||
| if (visitor.getCost() > maxCost) { | |||
| const cost = visitor.getCost(); | |||
| if (options.onCost) options.onCost(cost); | |||
There was a problem hiding this comment.
maybe also pass the query as arg to onCost?
There was a problem hiding this comment.
@taion I still think passing the document node might be useful to both these functions. we maybe want to log queries that fail in prod as well?
|
I'd rather have some callback for formatting the error message to send up to the client. |
|
ok. can you elaborate on the api? |
|
just |
chirag04
changed the title
|
Wait, is the idea here that you want to log all costs on queries? Or what's the goal here? Maybe it'd be better to re-use the visitor to create a different rule that only does logging? |
|
initially i thought about logging all costs but figured it makes sense to do it only when validation fails. no? so logging when validation fails is the goal. |
|
I think there are two separate concerns – what to do right now in dev to figure out how to set cost limits, then what we'll want to do in a production deploy. |
|
Thinking about this some more, probably 4226b58 is the right way to go. |
|
@taion maybe do both. onCost and formatErrorMessage. onCost solves all our usecases but i can imagine somebody needing formatErrorMessage. wdyt? |
|
Sure – still could be useful to tell without e.g. looking at server logs more details on queries that exceed the cost limit. You'd only want to turn it on in dev, though – would probably be a security issue to enable it in prod. |
|
@taion anything left here? |
src/index.js
Outdated
| const cost = visitor.getCost(); | ||
| if (options.onCost) options.onCost(cost); | ||
| if (cost > maxCost) { | ||
| const errorMessage = options.formatErrorMessage ? |
There was a problem hiding this comment.
just default formatErrorMessage to soemthing
src/index.js
Outdated
| @@ -149,9 +149,14 @@ export function createComplexityLimitRule(maxCost, options = {}) { | |||
| } | |||
|
|
|||
| if (node.kind === 'Document') { | |||
| if (visitor.getCost() > maxCost) { | |||
| const cost = visitor.getCost(); | |||
| if (options.onCost) options.onCost(cost); | |||
There was a problem hiding this comment.
split out these properties before passing down to visitor
src/index.js
Outdated
| if (cost > maxCost) { | ||
| const errorMessage = options.formatErrorMessage ? | ||
| options.formatErrorMessage(cost) : | ||
| `query exceeds complexity limit. Cost: ${cost}`; |
There was a problem hiding this comment.
we should absolutely not always return the cost here; the calculation may be a private detail
README.md
Outdated
| @@ -20,6 +20,8 @@ const ComplexityLimitRule = createComplexityLimitRule(1000, { | |||
| scalarCost: 1, | |||
| objectCost: 10, // Default is 0. | |||
| listFactor: 20, // Default is 10. | |||
| onCost: (cost) => console.log('total'), | |||
| formatErrorMessage: (cost) => 'Bad Query', | |||
There was a problem hiding this comment.
these examples are sort of weak (plus don't follow eslint guidelines). also the graf above needs to explain these options.
|
@taion updated. |
something like this can be useful for logging the cost. wdyt?