Skip to content

Conversation

@evanmcneely
Copy link
Contributor

@evanmcneely evanmcneely commented May 23, 2022

Summary

Changes are not 100% ready but I am requesting feedback and input on 1 particular problem I am having.

When working with lists in graphQL, the type complexity of the query is dependent on the number of objects returned in the list. For example, on line 263 in typeComplexityAnalysis.test.ts:

query = `
     Query { 
          human(id: 1) { 
               name, 
               friends { 
                   name 
                } 
           }
    }`;

the field friends is a list that returns an unknown number of objects in the response. How would we account for this when writing the complexity algorithm? Does this need to be accounted for the buildTypeWeights function?

Similarly on line 271 in typeComplexityAnalysis.test.ts,

query = 'Query {reviews(episode: EMPIRE, first: 3) { stars, commentary } }';

the number of objects in the response is 3, but it could 4, or 5. It's variable based on the client requests. Should the typeComplextyAnalysis algorithm take in the query variables as an argument in order to estimate maximum size that the response could be?

Type of Change

  • New feature (non-breaking change which adds functionality)

Issues

closes #8

Evidence

Screen Shot 2022-05-23 at 10 31 45 AM

@evanmcneely evanmcneely requested a review from shalarewicz May 23, 2022 14:53
@evanmcneely evanmcneely removed the request for review from shalarewicz May 23, 2022 15:51
@shalarewicz
Copy link
Collaborator

shalarewicz commented May 23, 2022

Re: your second question about taking into account first and limit arguments. Yes, I think we should do account for these in our calculation in order to get an accurate complexity estimate. So if the query is asking for the first: 3 Users and each User has a weight of 1 then the overall complexity would be 3. I think that the names of these variables would need to be defined somehow if the config so that the algorithm knows what arguments to look for. Maybe something like a paginationArgName and limitArgName that default to first and limit respectively.

As for unbounded lists. I'm not sure and I don't see a way to precompute this without having access to the database. We could have this to default to some value, make it configurable or cache the complexity each time an unbounded collection is resolved. The first call would be 'free' but after that there would be a somewhat accurate estimate assuming the list is called frequently enough

I'll poke around some of the existing packages and see if they handle this case.

*
* Some user configuration will be needed unless someone has bright ideas.
*/
// ? type weigts are variable, not sure how to calculate this.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the question on line 259 how to calculate this in the actual implementation or in the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is was with regards to implementing the algorithm. It was meant describe my thinking at that point and to imply that this test was weakly written. I can't say with any confidence that we should expect a complexity of 7 from this query. I'll skip it and make that clear until we figure how to handle these scenarios

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting that we discussed the implementation using DFS/backtracking to compute complexity of nested queries.

// What would that look like here? I think we should throw ar error from this function.
test('Throws an error if for a bad query', () => {
query = `Query { hello { hi } }`; // type doesn't exist
expect(getQueryTypeComplxity(query, typeWeights)).toThrow('Error');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor but missing a 'e' in getQueryTypeComplxity and you can just use .toThrow() which checks if any error is thrown.

I agree on this function throwing an error. A question for the middleware then is do we want to pass the error down the middleware chain or send back a 400 status indicating a malformed request

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking we pass it onto to GraphQL to handle, so that thir error handling can respond with their error messages. We can discuss.

Copy link
Collaborator

@shalarewicz shalarewicz May 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use the parse() method and can respond to the client in the same way that GraphQL does then we can send the error back to the client. Otherwise calling next() will just let GraphQL handle this.

  • Let's create an issue to dive further into error handling

@evanmcneely evanmcneely requested a review from shalarewicz May 27, 2022 22:11
@evanmcneely evanmcneely merged commit 35a928d into dev May 28, 2022
@evanmcneely evanmcneely deleted the em/complexityTests branch May 28, 2022 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Write test for complexity analysis algorithm

5 participants