-
-
Notifications
You must be signed in to change notification settings - Fork 493
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
GraphQL API Improvements #1022
Comments
Hey @RehanSaeed, how would you feel about including an input validation library in the GQL template? I wrote FairyBread and would be keen to get it in here, but I imagine we could include a switch that let's you choose between others, right? FYIs:
|
@benmccallum It's a great idea! Validation in Hot Chocolate is a major missing feature. Asked Michael from Hot Chocolate in this thread for more info on where he's going before we commit to a particular direction: https://twitter.com/RehanSaeedUK/status/1404716958520692737?s=20 |
@benmccallum Your library seems to have the most traction at the moment. How does it compare to the features listed in AppAny.HotChocolate.FluentValidation? |
3470 will come with the June release and should be in the next preview. |
@RehanSaeed, it's mostly in the approach of how the validation is fired, e.g. implicit vs explicit. FairyBread uses an implicit approach I guess you'd say, where you define One current downside from a perf perspective (though we're talking 9-14ms) is that the middleware is also completely cross-cutting, so where there's no validators there's still a cost (this is something I plan on fixing in the next version using type interceptors to attach the middleware only where needed). AppAny's approach is explicit (you attribute where validation should happen) and uses delegates for validators rather than AbstractValidator implementations. It has more configuration options per field. Pros/cons to each as with most things in software development ;) @sergeyshaykhullin can probably elaborate more. |
If your switch defaults to no particular library but offers FairyBread, AppAny's and maybe one of the attribute-based ones, people can make their own decision 😄 |
I try to keep the options to a minimum where possible because it does add a maintenance overhead. Ideally there'd be a defacto standard library (e.g. Serilog for logging) that we could use. Remember also that if people don't like the default, they can turn it off and add their own. The FairyBread implicit approach with the type interceptors so there is no negative performance impact seems like the ideal scenario. If you're building that, then that seems like the one we should initially pick to implement as an option. Would you be interested in submitting a PR? I've also been meaning to add an option to the API template to use FluentValidation too but haven't had time or any requests for that. |
@RehanSaeed , yea no worries, happy to when I get a sec and 100% understood on the maintenance front. Maybe this weekend I'll have the time to implement the type interceptors improvement; then I'll look to submit a PR here. |
Turns out I actually misinterpreted Sergey's benchmarks haha, I thought the WithoutValidation and WithValidation runs were both with AppAny and basically showing that his validation middleware won't incur a cost when there's no validator for an arg. But in fact it was just showing literally if there was no validation library hooked up at all. In any case, I've got a v7 prev 1 ready that uses the type interceptor approach I spoke of and I'll have a look at doing a PR here ntext |
IConnectionMultiplexer
inAddRedisQueryStorage
and remove workaround (See AddRedisQueryStorage Throws & Inconsistent API ChilliCream/graphql-platform#3470).services.InitializeOnStartup()
afterservices.AddGraphQL()
Various documentation changes ChilliCream/graphql-platform#4294.Waiting for Hot Chocolate vNext
WebApplicationFactory
.The text was updated successfully, but these errors were encountered: