Skip to content
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

Fix nested complexity fragment sorting #3209

Merged
merged 3 commits into from
Jul 3, 2022
Merged

Fix nested complexity fragment sorting #3209

merged 3 commits into from
Jul 3, 2022

Conversation

Shane32
Copy link
Member

@Shane32 Shane32 commented Jun 30, 2022

Added failing test

Demonstrating that by swapping the order of two fragments, the test either passes or fails

[UPDATE]
Fixed by second commit.
5 4 2 1 3 - order before
5 3 2 4 1 - order after

Now should work for all cases.

@github-actions github-actions bot added the test Pull request that adds new or changes existing tests label Jun 30, 2022
@sungam3r sungam3r changed the title [WIP] Fix nested complexity fragment sorting Fix nested complexity fragment sorting Jul 3, 2022
@sungam3r
Copy link
Member

sungam3r commented Jul 3, 2022

5.4 ?

Comment on lines +91 to +95
foreach (var item in dependencies)
{
if (item.Value == null)
return item.Key;
}

Check notice

Code scanning / CodeQL

Missed opportunity to use Where

This foreach loop implicitly filters its target sequence [here](1) - consider filtering the sequence explicitly using '.Where(...)'.
@sungam3r
Copy link
Member

sungam3r commented Jul 3, 2022

WOW, works fine locally, failed on CI due to ComplexityAnalyzer.cs:line 71 - foreach on Keys

@sungam3r sungam3r self-assigned this Jul 3, 2022
@sungam3r sungam3r added the bugfix Pull request that fixes a bug label Jul 3, 2022
Comment on lines +75 to +79
foreach (var item in dependencies) // no deconstruct syntax for netstandard2.0
{
if (item.Value?.Remove(independentFragment) == true && item.Value.Count == 0)
fragsToNull.Add(item.Key);
}

Check notice

Code scanning / CodeQL

Missed opportunity to use Where

This foreach loop implicitly filters its target sequence [here](1) - consider filtering the sequence explicitly using '.Where(...)'.
@codecov-commenter
Copy link

Codecov Report

Merging #3209 (f830fe0) into master (5810855) will increase coverage by 0.02%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master    #3209      +/-   ##
==========================================
+ Coverage   84.44%   84.46%   +0.02%     
==========================================
  Files         371      370       -1     
  Lines       16042    16051       +9     
  Branches     2602     2604       +2     
==========================================
+ Hits        13547    13558      +11     
+ Misses       1873     1872       -1     
+ Partials      622      621       -1     
Impacted Files Coverage Δ
...raphQL/Validation/Complexity/ComplexityAnalyzer.cs 93.61% <95.23%> (+2.18%) ⬆️
src/GraphQL/Types/Schema.cs 79.90% <0.00%> (+0.91%) ⬆️

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 5810855...f830fe0. Read the comment docs.

return item.Key;
}

throw new InvalidOperationException("Fragments dependency cycle detected!");
Copy link
Member

Choose a reason for hiding this comment

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

@Shane32
Copy link
Member Author

Shane32 commented Jul 3, 2022

WOW, works fine locally, failed on CI due to ComplexityAnalyzer.cs:line 71 - foreach on Keys

I think it might be due to different frameworks.

@Shane32
Copy link
Member Author

Shane32 commented Jul 3, 2022

Looks good to me. I'll merge and make a release.

@Shane32
Copy link
Member Author

Shane32 commented Jul 3, 2022

This is the only PR merged since 5.3.1 so this will be 5.3.2 as it is a bugfix

@Shane32 Shane32 merged commit 87d7a5e into master Jul 3, 2022
@Shane32 Shane32 deleted the nested_fragments branch July 3, 2022 17:50
@Shane32 Shane32 added this to the 5.3.2 milestone Jul 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Pull request that fixes a bug test Pull request that adds new or changes existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Very Deeply Nested (5+ levels?) Fragments Fail Based Upon Order of Fragments In Query
3 participants