-
Notifications
You must be signed in to change notification settings - Fork 41
feat: add support for connections in complexity calculation (@freshjsw) #810
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
Conversation
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #810 +/- ##
==========================================
+ Coverage 91.96% 92.13% +0.16%
==========================================
Files 17 17
Lines 386 394 +8
Branches 121 134 +13
==========================================
+ Hits 355 363 +8
Misses 31 31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hello @LMaxence and @thibaudlabat. Thanks for your work on this package. I just noticed that this update changes a lot cost-limits validation and it was released as patch version It seems for me this should be minor version release and better to mention it in changelog as potentially a lot of people might update the dependency and then it appears that a lot of their queries are not valid anymore because cost estimations algorithm changed. |
Yes, this should have been a major version bump. Any time we update the complexity/depth/etc. calculation in a way that can increase the measured value, it's a breaking change that could start rejecting previously OK requests (that'd be a fun incident to figure out on the fly if it was your website's home page query 😬 ). In the future, if we want to release changes like this one in a minor version bump, we'd need to introduce the change via an optional config setting (e.g. |
See #762
Original contribution from @freshjsw
I just made changes according to @LMaxence comments.
--- ORIGINAL PULL REQUEST DESCRIPTION ---
The cost complexity cost-limit plugin does not take into account how many entities are being returned when either the first or last argument are specified at the field level. This PR supports calculating complexity based on the number of records requested in the query.
This commit fixes #761
These two examples which have the same complexity cost (I have omitted edges and pageInfo to keep these examples simple)
The cost complexity of the query below should increase by a factor of the number of books requested
Note the PR does not currently handle VariableDefinitions but will handle the example above