Query complexity #139

abhikmitra · 2018-09-01T12:04:22Z

Fixes #36
Adds an option parameter called complexity to the field decorator.

codecov · 2018-09-01T12:06:56Z

Codecov Report

Merging #139 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #139      +/-   ##
==========================================
+ Coverage   95.08%   95.09%   +0.01%     
==========================================
  Files          60       60              
  Lines         956      958       +2     
  Branches      170      170              
==========================================
+ Hits          909      911       +2     
  Misses         46       46              
  Partials        1        1

Impacted Files	Coverage Δ
src/decorators/FieldResolver.ts	`93.33% <ø> (ø)`	⬆️
src/schema/schema-generator.ts	`96.95% <ø> (ø)`	⬆️
src/decorators/Field.ts	`100% <ø> (ø)`	⬆️
src/helpers/handlers.ts	`80% <ø> (ø)`	⬆️
src/metadata/metadata-storage.ts	`100% <100%> (ø)`	⬆️
src/interfaces/index.ts	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1b0e0b...8597c7d. Read the comment docs.

MichalLytek

What about queries, mutations and subscriptions? Please add proper changes also to them.
query-complexity is not a simple-usage - please create a separate example folder with comments in code how and why we setup validationRules
no implicit any - please describe the types of graphql-query-complexity module in the way that we use, along with interfaces merging to extend GraphQLFieldConfig interface
add proper tests for this feature - registering in metadata storage, as well as simple check if the validation works (one query that pass, one that fails)
It might be useful to add proper section in docs about this integration, how to use it, etc.

I think I need to make a list of all things that PRs have to provide to be merged 😉

MichalLytek · 2018-09-01T12:15:44Z

src/decorators/types.ts

@@ -17,6 +17,7 @@ export type SubscriptionFilterFunc = (

 export interface DecoratorTypeOptions {
  nullable?: boolean;
+  complexity?: number;


graphql-query-complexity allows to calculate it dynamically: complexity: (args, childComplexity) => childComplexity * args.count,

MichalLytek

Please add tests also for queries and field resolver as they dynamically add fields to object types too.
Also please create proper documentation of this feature in advanced section, with tutorial how to use it and setup, just like authorization have.

MichalLytek · 2018-09-02T14:15:11Z

@types/graphql-query-complexity/index.d.ts

@@ -0,0 +1,35 @@
+


I think that this might become obsolete soon 😕
slicknode/graphql-query-complexity@f7713b2
We should just wait with this PR for publishing that package with types.

I compiled his code in TSC and put the types. So it would be similar when he publishes.

Partially - newest version is flattened to a single file, so there will be no graphql-query-complexity/dist/QueryComplexity and also it's weird to have typings on repo rather than publish it on DefinitelyTyped. So let's wait a few days and decide what @ivome will do 😉

examples/query-complexity/index.ts

MichalLytek · 2018-09-02T14:16:19Z

examples/query-complexity/index.ts

+       * More doucmentation can be found (here)[https://github.com/ivome/graphql-query-complexity]
+       */
+      queryComplexity({
+        maximumComplexity: 8,


here we can declare the maximum cost of the queries, it can be dynamically and depend on user privileges

MichalLytek · 2018-09-02T14:16:46Z

examples/query-complexity/index.ts

+       */
+      queryComplexity({
+        maximumComplexity: 8,
+        variables: req.query.variables,


we also have to pass the query variables to the queryComplexity validator

MichalLytek · 2018-09-02T14:18:03Z

examples/query-complexity/index.ts

+        maximumComplexity: 8,
+        variables: req.query.variables,
+        onComplete: (complexity: number) => {
+          console.log("Query Complexity:", complexity);


We can also react on complexity calculation and do something (@abhikmitra I don't know why we show example with console.log btw 😆)

Sure what do you have in your mind other than console log ? I just followed the same format as what is followed in graphql-query-complexity repo

I think here you can subtract the amount from user's account points fund.

examples/query-complexity/recipe-resolver.ts

MichalLytek · 2018-09-02T14:39:57Z

examples/simple-usage/index.ts

@@ -18,6 +18,15 @@ async function bootstrap() {
    port: 4000,
    endpoint: "/graphql",
    playground: "/playground",
+    validationRules: req => [


Remove this from simple-usage

MichalLytek · 2018-09-02T14:41:11Z

src/schema/schema-generator.ts

@@ -227,6 +227,7 @@ export abstract class SchemaGenerator {
                );
                fieldsMap[field.schemaName] = {
                  type: this.getGraphQLOutputType(field.name, field.getType(), field.typeOptions),
+                  complexity: field.complexity || 1,


Should we explicitly put 1 here? Isn't it default behavior of graphql-query-complexity?

… into query-complexity

abhikmitra · 2018-09-02T17:05:59Z

@19majkel94 So all fieldResolvers will always have a corresponding field type right ? Complexity is respected on field type and would not be respected on @fieldResolvers . Is that ok ?

Should we stop allowing complexity on field resolvers ?

MichalLytek · 2018-09-02T17:19:25Z

@abhikmitra
It's a bit complex 😄
For now field resolvers overwrite authorizations and args settings for the field:
https://github.com/19majkel94/type-graphql/blob/d45829c60f7a9e4a9c49d9df7f95012b31229069/src/metadata/metadata-storage.ts#L206-L215
So it should also overwrite complexity property if provided in decorator option.

And now with the fresh eyesight, my design of metadata building is awful 🤢

abhikmitra · 2018-09-02T17:57:25Z

Fixed your comments . Let me know what else you need.

MichalLytek · 2018-09-04T19:33:13Z

docs/complexity.md

+  });
+```
+
+And it's done! 😉 . You can pass complexity as option to any of `@field`, `@fieldResolver`, `@mutation` & `@subscription`. For the same property, if both the @fieldresolver as well as @field have complexity defined , then the complexity passed to the field resolver decorator takes precedence. 


PascalCase for decorators names and always wrap in backticks

MichalLytek · 2018-09-04T19:38:06Z

examples/query-complexity/recipe-type.ts

+  @Field({ complexity: 6 })
+  title: string;
+
+  @Field(type => String, { nullable: true, deprecationReason: "Use `description` field instead" })


Please remove also all this not needed fields - keep focus only on complexity feature, the Recipe is just the background.

MichalLytek · 2018-09-04T19:39:00Z

examples/simple-usage/index.ts

@@ -1,7 +1,7 @@
 import "reflect-metadata";
 import { GraphQLServer, Options } from "graphql-yoga";
 import { buildSchema } from "../../src";
-
+import queryComplexity from "graphql-query-complexity";


This one is not needed.

MichalLytek · 2018-09-04T19:40:10Z

tests/functional/fields.ts

@@ -88,6 +91,13 @@ describe("Fields - schema", () => {
    expect(schemaIntrospection).toBeDefined();
  });

+  it("should add complexity info to the metadata storage", async () => {


it should register complexity info for field

MichalLytek · 2018-09-04T20:01:33Z

tests/functional/resolvers.ts

+          fieldResolverMethod
+        }
+      }`;
+      const ast = parse(query);


Could you create a helper function in this describe suite to avoid duplication in this three it test cases?

MichalLytek · 2018-09-04T20:03:11Z

docs/complexity.md

+
+And it's done! 😉 . You can pass complexity as option to any of `@field`, `@fieldResolver`, `@mutation` & `@subscription`. For the same property, if both the @fieldresolver as well as @field have complexity defined , then the complexity passed to the field resolver decorator takes precedence. 
+
+Have a look at the [graphql-query-complexity](https://github.com/ivome/graphql-query-complexity) docs to understand more about how query complexity is computed.


For more info about how query complexity is computed, please visit graphql-query-complexity docs.

MichalLytek · 2018-09-04T20:04:11Z

docs/complexity.md

+  });
+```
+
+And it's done! 😉 . You can pass complexity as option to any of `@field`, `@fieldResolver`, `@mutation` & `@subscription`. For the same property, if both the @fieldresolver as well as @field have complexity defined , then the complexity passed to the field resolver decorator takes precedence. 


Move You can pass complexity as option to any... above this, place it below You can omit the complexity.

MichalLytek · 2018-09-04T20:04:44Z

docs/complexity.md

+  @Field({ complexity: 2})
+  publicField: string;
+
+  @Field({ complexity: 3 })


Maybe show a field with args and show how to use complexityResolver?

MichalLytek · 2018-09-04T20:09:50Z

docs/complexity.md

+---
+title: Query Complexity
+---
+A single GraphQL query can potentially generate thousands of database operations. To keep track and limit of what each graphQl operation can do , `TypeGraphQL` provides you the option of integrating with Query Complexity tools like  [graphql-query-complexity](https://github.com/ivome/graphql-query-complexity).


fix graphQl

...can potentially generate huge workload for a server, like thousands of database operations.

maybe we should also mention DDoS and recursive queries, like user -> posts (1000) -> user -> posts (1000)

MichalLytek · 2018-09-04T20:13:30Z

@abhikmitra
Looks like there's no response from @ivome. Could you extract the typings and publish it to https://github.com/DefinitelyTyped/DefinitelyTyped for now? I don't want to block this PR, we can later upgrade the dependency when 0.2.x release of graphql-query-complexity will be published.

docs/complexity.md

ivome · 2018-09-06T18:08:42Z

Sorry for the late reply. I just release a new version of graphql-query-complexity with some major changes, deprecations and new features, type definitions in the package etc.
Check the docs of the repository for more information.

MichalLytek · 2018-09-06T18:11:49Z

@ivome Thanks! 🎉

@abhikmitra Please check new features of graphql-query-complexity v0.2.0 and align this PR to that changes, removing our local @types folder 😉

abhikmitra · 2018-09-07T19:50:06Z

@19majkel94 I upgraded to 0.2.0

MichalLytek · 2018-09-07T19:52:22Z

docs/complexity.md

@@ -1,12 +1,12 @@
 ---
 title: Query Complexity
 ---
-A single graphQl query can potentially generate huge workload for a server, like thousands of database operations which can be used to cause DDoS attacks. To keep track and limit of what each graphQl operation can do , `TypeGraphQL` provides you the option of integrating with Query Complexity tools like  [graphql-query-complexity](https://github.com/ivome/graphql-query-complexity).
+A single GraphQl query can potentially generate huge workload for a server, like thousands of database operations which can be used to cause DDoS attacks. To keep track and limit of what each GraphQl operation can do , `TypeGraphQL` provides you the option of integrating with Query Complexity tools like  [graphql-query-complexity](https://github.com/ivome/graphql-query-complexity).


Is it really to hard to write GraphQL? Copy and paste? 😆

MichalLytek · 2018-09-07T19:52:57Z

docs/complexity.md

@@ -19,15 +19,15 @@ class MyObject {
  @Field({ complexity: 2})


space - complexity: 2 })

MichalLytek · 2018-09-07T19:53:37Z

docs/complexity.md

@@ -51,6 +51,15 @@ In next step, you need to integrate `graphql-query-complexity` with your graphql
        onComplete: (complexity: number) => {
          console.log("Query Complexity:", complexity);
        },
+        estimators: [
+          fieldConfigEstimator(),
+          // Add more estimators here...


It's not a good suggestion - which one, directives?

MichalLytek · 2018-09-07T19:53:58Z

docs/complexity.md

@@ -51,6 +51,15 @@ In next step, you need to integrate `graphql-query-complexity` with your graphql
        onComplete: (complexity: number) => {
          console.log("Query Complexity:", complexity);
        },
+        estimators: [
+          fieldConfigEstimator(),


Here should be comment that this one is mandatory to make it works

MichalLytek · 2018-09-07T19:54:26Z

docs/complexity.md

+          // Add more estimators here...
+          // This will assign each field a complexity of 1 if no other estimator
+          // returned a value.
+          simpleEstimator({


And that here we can define the default value for field not explicitly annotated.

MichalLytek · 2018-09-07T19:56:40Z

src/decorators/types.ts

@@ -32,7 +32,7 @@ export interface ValidateOptions {
  validate?: boolean | ValidatorOptions;
 }
 export interface ComplexityOptions {
-  complexity?: Complexity;
+  complexity?: ((arg: ComplexityEstimatorArgs) => number) | number;


There is a ComplexityEstimator type that we should use - complexity function can return void to fallback to default complexity

MichalLytek · 2018-09-07T19:57:47Z

src/schema/schema-generator.ts

@@ -360,7 +360,7 @@ export abstract class SchemaGenerator {
        description: handler.description,
        deprecationReason: handler.deprecationReason,
        complexity: handler.complexity,
-      };
+      } as GraphQLFieldConfig<any, any>;


What is the reason of that? Declaration merging isn't working?
https://github.com/slicknode/graphql-query-complexity/blob/157285004e22eed3ca1ab1b43226d671f0c3affb/src/QueryComplexity.ts#L32-L36

Getting this error if I remove this

MichalLytek · 2018-09-07T20:44:49Z

@abhikmitra
Please create our own type type Complexity = ComplexityEstimator | number and replace all of imports of this from "graphql-query-complexity" in metadata storage & others.

abhikmitra · 2018-09-08T06:16:03Z

@19majkel94 anything else ?

MichalLytek · 2018-09-08T09:25:49Z

@abhikmitra Thank you very much for your contribution! ❤️

Abhik Mitra added 2 commits September 1, 2018 17:19

Support for Query Complexity

2ebe3a2

GraphQL Complexity

5e2bcf5

MichalLytek requested changes Sep 1, 2018

View reviewed changes

MichalLytek reviewed Sep 1, 2018

View reviewed changes

Fixed all comments

ac65c7a

MichalLytek requested changes Sep 2, 2018

View reviewed changes

Fix definitions, interfaces, types and tests isssues

7357a86

MichalLytek force-pushed the query-complexity branch from a4b2289 to 7357a86 Compare September 2, 2018 14:50

Abhik Mitra added 3 commits September 2, 2018 21:47

Fixed Code review comments

e9a7f81

Merge branch 'query-complexity' of github.com:abhikmitra/type-graphql…

0f444b8

… into query-complexity

Fixed Comments

c1907b7

Added docs and more tests

317cb7d

MichalLytek requested changes Sep 4, 2018

View reviewed changes

MichalLytek assigned abhikmitra Sep 4, 2018

MichalLytek added Enhancement 🆕 New feature or request Community 👨‍👧 Something initiated by a community labels Sep 4, 2018

FIxed review comments

d8537cb

MichalLytek requested changes Sep 5, 2018

View reviewed changes

docs/complexity.md Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Abhik Mitra added 2 commits September 8, 2018 01:12

Upgraded graphql-query-complexity" to 0.2.0

9708c94

Fixed Type issues

9e6d47b

MichalLytek requested changes Sep 7, 2018

View reviewed changes

Fixed code review comments

56c2072

MichalLytek added 2 commits September 8, 2018 11:12

Fix complexity types issues

e1724a6

Merge branch 'master' into query-complexity

8597c7d

MichalLytek approved these changes Sep 8, 2018

View reviewed changes

MichalLytek merged commit 66e7f97 into MichalLytek:master Sep 8, 2018

MichalLytek added this to the 1.0.0 release milestone Sep 8, 2018


		And it's done! 😉 . You can pass complexity as option to any of `@field`, `@fieldResolver`, `@mutation` & `@subscription`. For the same property, if both the @fieldresolver as well as @field have complexity defined , then the complexity passed to the field resolver decorator takes precedence.

		Have a look at the [graphql-query-complexity](https://github.com/ivome/graphql-query-complexity) docs to understand more about how query complexity is computed.

		@@ -19,15 +19,15 @@ class MyObject {
		@Field({ complexity: 2})

Query complexity #139

Query complexity #139

Conversation

abhikmitra commented Sep 1, 2018

codecov bot commented Sep 1, 2018 • edited Loading

Codecov Report

MichalLytek left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalLytek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhikmitra Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhikmitra commented Sep 2, 2018 • edited Loading

MichalLytek commented Sep 2, 2018

abhikmitra commented Sep 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalLytek commented Sep 4, 2018

ivome commented Sep 6, 2018

MichalLytek commented Sep 6, 2018

This comment has been minimized.

This comment has been minimized.

abhikmitra commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalLytek commented Sep 7, 2018

abhikmitra commented Sep 8, 2018

MichalLytek commented Sep 8, 2018

codecov bot commented Sep 1, 2018 •

edited

Loading

MichalLytek left a comment •

edited

Loading

abhikmitra Sep 2, 2018 •

edited

Loading

abhikmitra commented Sep 2, 2018 •

edited

Loading