ChilliCream / graphql-platform

Welcome to the home of the Hot Chocolate GraphQL server for .NET, the Strawberry Shake GraphQL client for .NET and Banana Cake Pop the awesome Monaco based GraphQL IDE.
https://chillicream.com
MIT License
5.03k stars 723 forks source link

Don't check for a convention when not one isn't needed #7019

Closed SeanTAllen closed 1 week ago

SeanTAllen commented 2 months ago

When setting up filtering and sorting with UseFiltering and UseSorting, there are 3 different code paths that are traversed. Prior to this change, all 3 of them required that a convention be registered otherwise an exception would occur. However, for both filtering and sorting, only 1 of the code paths actually needs a convention. The other two do not use the convention at all.

This commit changes the filtering and sorting object field descriptor extensions to attempt to get a convention only within the block of code that requires a convention. By making this small change, the other two code paths that do not require a convention will no longer encounter an exception. Instead, filtering and sorting can be set up as expected.

This commit partially addresses #6983. We are able locally, with these changes applied, to use filtering and sorting from a type module by calling the versions of UseFiltering and UseSorting that take a type. Without the changes in this commit, we get the "no convention registered" error.

This change does not close #6983 as it is still possible to encounter the "no convention registered" exception and its suggested remedy of "Call AddFiltering() on the schema builder" doesn't work.

From what we can see, and have detailed in #6983, no conventions registered with the schema builder either directly or the defaults provided by AddFiltering and AddSorting are available at the time that a type module is run.

The result of this commit is to allow the usage UseFiltering and UseSorting with types created via a type module. The other option we are aware of is detailed by @jimitndiaye in https://github.com/ChilliCream/graphql-platform/discussions/6975. Jimit routes around the convention problem by not using a convention at all in his custom middleware. A close examination of UseFilteringInternal at https://github.com/ChilliCream/graphql-platform/discussions/6975#discussioncomment-8778742 will show this lack of convention usage as key to his fix. Instead of going the custom middleware route, we made these two small changes to Hot Chocolate. We prefer this approach.

SeanTAllen commented 2 months ago

My editor made a couple extraneous whitespace changes. Let me know if you would like me to revert them.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 70.16%. Comparing base (ab3909e) to head (c762100). Report is 26 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #7019 +/- ## ========================================== - Coverage 72.85% 70.16% -2.69% ========================================== Files 2585 2585 Lines 129190 129491 +301 ========================================== - Hits 94115 90857 -3258 - Misses 35075 38634 +3559 ``` | [Flag](https://app.codecov.io/gh/ChilliCream/graphql-platform/pull/7019/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ChilliCream) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/ChilliCream/graphql-platform/pull/7019/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ChilliCream) | `70.16% <100.00%> (-2.69%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ChilliCream#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

SeanTAllen commented 1 month ago

@michaelstaib ping to get your attention on this per your comment https://github.com/ChilliCream/graphql-platform/issues/6983#issuecomment-2002461845

SeanTAllen commented 1 week ago

@PascalSenn in #6983, @michaelstaib said you should be pinged about this.

PascalSenn commented 1 week ago

@SeanTAllen, sorry, I didn't have this on my radar! @michaelstaib, this looks good to me. It's just moving the convention resolution into the branch that actually needs it.