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

GraphQL API Improvements #1022

Open
3 of 11 tasks
RehanSaeed opened this issue Mar 29, 2021 · 9 comments
Open
3 of 11 tasks

GraphQL API Improvements #1022

RehanSaeed opened this issue Mar 29, 2021 · 9 comments
Labels
enhancement Issues describing an enhancement or pull requests adding an enhancement. template/GraphQL The GraphQL project template

Comments

@RehanSaeed
Copy link
Member

RehanSaeed commented Mar 29, 2021

Waiting for Hot Chocolate vNext

@RehanSaeed RehanSaeed added enhancement Issues describing an enhancement or pull requests adding an enhancement. template/GraphQL The GraphQL project template labels Mar 29, 2021
@benmccallum
Copy link
Contributor

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:

  • v11.1 and v11.2 have been released, v12 is being defined and start right now
  • annotation-based is the way to go for sure
  • XML docs are also awesome and the way to go

@RehanSaeed
Copy link
Member Author

RehanSaeed commented Jun 15, 2021

@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

@RehanSaeed
Copy link
Member Author

@benmccallum Your library seems to have the most traction at the moment. How does it compare to the features listed in AppAny.HotChocolate.FluentValidation?

@michaelstaib
Copy link

3470 will come with the June release and should be in the next preview.

@benmccallum
Copy link
Contributor

@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 AbstractValidator<TInput> and it finds it and executes it for resolvers that have an input of that type.

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.

@benmccallum
Copy link
Contributor

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 😄

@RehanSaeed
Copy link
Member Author

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.

@benmccallum
Copy link
Contributor

@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.

@benmccallum
Copy link
Contributor

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

@RehanSaeed RehanSaeed pinned this issue Oct 4, 2021
@RehanSaeed RehanSaeed unpinned this issue Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues describing an enhancement or pull requests adding an enhancement. template/GraphQL The GraphQL project template
Projects
None yet
Development

No branches or pull requests

3 participants