Open adambajguz opened 2 years ago
Hi @adambajguz .
As for this very moment, I'm struggling with this decision. I have a gut feeling that it's going to be a maintenance disaster. I believe I've seen @JeremySkinner 's post explaining that with the FluentValidation itself never he had any problems, but all pain and suffering was related to the libraries around the main product. Reason? While .net standard is stable, all other Microsoft APIs are not so much both in declaration and behavior.
I am asking myself also, do we need a separate nuget package for this?
Dependency injection?
using System;
using System.Linq;
using System.Reflection;
using Microsoft.Extensions.DependencyInjection;
using Validot;
static class AddValidatorsExtensions
{
public static IServiceCollection AddValidators(this IServiceCollection @this, params Assembly[] assemblies)
{
var assembliesToScan = assemblies.Length > 0
? assemblies
: AppDomain.CurrentDomain.GetAssemblies();
var holders = Validator.Factory.FetchHolders(assembliesToScan)
.GroupBy(h => h.SpecifiedType)
.Select(s => new
{
ValidatorType = s.First().ValidatorType,
ValidatorInstance = s.First().CreateValidator()
});
foreach (var holder in holders)
{
@this.AddSingleton(holder.ValidatorType, holder.ValidatorInstance);
}
return @this;
}
}
The question is: how much more would be in the nuget package comparing to the snippet above? Or maybe it should be promoted more aggressively? Maybe there is something that I'm missing and clearly failing to see or understand? Please don't hesitate to point this out.
Validot.AspNet
? What exactly would be inside? Personally I thought that delivering something similar to ToDictionary
would be enough (https://docs.fluentvalidation.net/en/latest/aspnet.html#minimal-apis) to integrate with ASP.NET's minimal api approach.
Validot.Swashbuckle
? Again; what exactly do you think it should contain?
Please don't get me wrong, @adambajguz . I'm more than grateful for triggering that discussion and really appreciate your contribution to the project. I'm more than happy to discuss all the details regarding and only then decide what direction I'd like Validot to take regarding the extensions. It's just that I also see all of the work that needs to be done in order to keep it reliable and stable. Also, maybe there is no point of a separate library that ultimately wraps merely 20 lines of code from the documentation?
Let me know what do you think.
"While .net standard is stable, all other Microsoft APIs are not so much both in declaration and behavior." - agree, and core package Validot
should still be written in .NET Standard. Integrations (non .NET Standar, .NET 6 etc. packages) could even be in a separate repository like Validot.Integrations
.
"While .net standard is stable, all other Microsoft APIs are not so much both in declaration and behavior." - for sure Microsoft.Extensions.DependencyInjection.Abstractions
is prety solid because I didn't see much changes through the years. On the other hand, ASP.NET Core is a huge framework so I must agree that its more unpredictable in terms of changes.
"Dependency injection?" - Yes. I like standardization and I believe modern .NET libraries should follow modern .NET code - meaning Validot
should provide an official integration with Microsoft.Extensions.DependencyInjection
. Also "copy and paste" is good one time but not one million times in one million different projects. The pasted code in one million projects will brake after a breaking change in e.g. FetchHolders
; however, it won't brake when those one million projects use an official package - a simple package upgrade will be enough.
I've copied what I've created so far to a public repo (I think it's easier to show the code than to explain everything): https://github.com/adambajguz/ValidotEx/tree/main/src/Validot.DependencyInjection
IValidotValidator
and IValidotValidator<T>
are basically proxy classes to bypass the current library restrictions:
IValidator
interface need for e.g. ASP.NET Core integration (#28)IReadOnlyDictionary<string, IReadOnlyList<IError>> ErrorRegistry
. I've used reflection to build this dictionary, and I don't like this solution. I plan to use IError
to provide Swagger with informations like minimum length (Validot.Swashbuckle
library draft coming soon).ValidotBuilder
- a common class for registering everthing that is Validot-specific in IServiceCollection
.IValidotValidatorProvider
- simple helper service for resolving validators primarly using Type
object instance (GetValidator(Type modelType)
)ValidotBuilderExtensions
- almost the code from docs"Validot.AspNet ?" - for old ASP.NET framework yes but I don't think it's worth making. Validot.AspNetCore
will be a better name.
Anticipating the question "Why both Validot.AspNetCore and Validot.DependencyInjection, and not just a single Validot.AspNetCore?": because not everyone is writing a web app and injecting ASP.NET Core dependencies everytime is not a good practice.
"Personally I thought that delivering something similar to ToDictionary would be enough (https://docs.fluentvalidation.net/en/latest/aspnet.html#minimal-apis) to integrate with ASP.NET's minimal api approach." - as far as I know minimal APIs don't come with any built-in support for validation but classic controller based API do. The package is intended for those APIs. I know we can call a validator in a controller action manually but then we're losing the built-in model state conversion to problem details. It is a bit "magical" though but ASP.NET Core has by default a data annotations validation, so I don't think that is a problem for the community.
Validot.AspNetCore
draft: https://github.com/adambajguz/ValidotEx/tree/main/src/Validot.AspNetCore and an example usage https://github.com/adambajguz/ValidotEx/tree/main/examples/WebApplicationExample
Validot.Swashbuckle
- Basically a Validot analogue to MicroElements.Swashbuckle.FluentValidation. Implementation draft coming soon ;)
I believe I've seen @JeremySkinner 's post explaining that with the FluentValidation itself never he had any problems, but all pain and suffering was related to the libraries around the main product
Yes that's correct. Maintaining the FluentValidation.AspNetCore
package has been a real pain for a few main reasons:
So I'd say that it's really up to whether or not you're willing to accept the support and maintenance burden.
Personally I'd recommend not including ASP.NET Core integration in your core repo, but rather allow the community to maintain it as a separate integration package, if the community feels strongly about having it. With hindsight that's what I wish I'd done, and is probably what I'll move to for FluentValidation going forward. Personally I think manual validation is better (inject the validator and invoke it) as it's so much easier to understand what's happening, which is what I recommend for FluentValidation these days.
Validot.AspNetCore draft: https://github.com/adambajguz/ValidotEx/tree/main/src/Validot.AspNetCore
As much of this is based on FluentValidation's codebase please ensure that all copyright notices remain in place, which is required by our license. Thanks! :)
@JeremySkinner "As much of this is based on FluentValidation's codebase please ensure that all copyright notices remain in place, which is required by our license. Thanks! :)" - Yes, I know. Everything will be included :) Currenty there are only comments like <remarks>Based on: https://github.com/FluentValidation/FluentValidation.AspNetCore</remarks>
to remember that the class is based or is simpy copied from FluentValidation.AspNetCore.
I don't have time this week for longer discussion (will be back next week to catch up on it), but for now I don't also want you @adambajguz to have an impression that I'm ignoring the topic. I'm not, but at the same time when it comes to the library that other projects depend on, I've chosen the path of slow pace and re-thinking everything at least twice. Especially when introducing breaking changes or integrations with other libs.
I agree that all potential integrations should land in a separate repo. I think I'm going to move Validot to a dedicated namespace (instead of my personal one bartoszlenar
) and keep everything there.
As I acknowledge that DRY is a nice principle, I do also think that WET ("write everything twice") shouldn't be discarded automatically. If FetchHolders
changes its behavior, there will be a major version bump and as such a clear message to all users that there are breaking changes introduced in the release. This is why I follow semver very strictly.
Don't get me wrong; having a dedicated package for different thing would be great, but maintaining it in the long term could be - as @JeremySkinner confirmed with his experience - time consuming and painful.
Maybe the lack of support for "auto-magical" validation is the ASP.NET's guts should be intentional? Just thinking. Some time ago MS decided that WebForms where magic happened all the time isn't the path they should follow because it's just against the modern web architecture principles. And that was reasonable IMHO, they adopted MVC model... only to get rid of the "View" layer later (with its cshtml files). Just to follow even more modern approach of a standalone backend and a completely separate frontend layer.
Now they're promoting Minimal API, which is a bloody good direction to take. You have middlewares, fine, but why controllers? Just to wrap routing in some class-oriented way? For me, that's another legacy thing, another layer (this time the MVC's "C" to get rid of). Minimal API is the answer for that, similarly to the way ASP.NET MVC was to WebForms (ok, big generalization, but I trust you get my point). Prepare your endpoints and trigger whatever you like there. We're going back to "do-it-yourself". It's "cleaner" to e.g., follow CQRS and/or modular monolith and keep your features' elements grouped together (including validators) and expose certain stuff to some global registry like WebApplicationBuilder. Ultimately using ASP.NET Core as a receiving and routing layer that later triggers the internal modules, bundled and configured individually as you like.
But that's my opinion. Maybe the whole dotnet world is using Controllers and they're fine. But same stuff you could say about WebForms 15 years ago and about MVC 10 years ago.
And why assume that the library knows how to handle the validation in all of the cases? In one case it's about error messages, in another about just error codes that the frontend handles and triggers its own logic? Maybe the full path is expected as the keys or opposite - just the most nested property names?
What if someone would like to do a partial validation first and then react on some specific error and based on the flag redirect the user to some page or return a specific error code? Provide a possibility to set up a callback? Or advise to register one validator in the global DI and the second one in the handler/controller? I'm afraid this and many more will pop up if I open this pandora's box.
The possibilities are endless (and in theory the configuration options should also be) and combining that with maintenance efforts needed to keep everything reliable and aligned with Microsoft's frameworks - it definitely looks like a too big thing for me to handle.
But that's off-topic digression.
Long story short: I don't like "auto-magical" stuff happening when it comes to app architecture layering (and input validation is a thin layer in my opinion). I see integration with ASP.NET Core validation guts in the same way as I see integration with WebForms' ViewState. You can do it, a lot of people certainly will use it, but... really? And regarding the DI - if someone wants to use DI, that's great! Validot is designed for this, the validators could even be singletons without any problems. But do it your way, according to your architecture, knowing all consequences of your actions. Having validator-per-model registered in the DI container isn't a rocket science for the end user, but certainly might be for the maintainer.
That's my opinion. Let me know what you think.
Swashbuckle is another story; the idea is great and I'd love to see that library coming out! If you have time and will, I'd rather encourage you to redirect your energy on this. Is that in any way dependent on the other bits that you mentioned?
As for DI and ASP.NET Core integration... I appreciate your efforts! Thanks for that! But also, this is exactly why I included "Let's have a discussion first" point in CONTRIBUTING document. I just feel bad writing all of this having a demoable bits of code done and ready.
You've made some good points.
"Now they're promoting Minimal API, which is a bloody good direction to take. "
Minimal API has been around for a while but I still don't know what to think about them. I currently stick to controllers because they are well-know, well-suported, and feature-rich. I also like to have endpoint/actions group in one class, so I keep using them for APIs. Don't get me wrong, I don't say that minimal API approach is bad.
"(... )Prepare your endpoints and trigger whatever you like there. We're going back to "do-it-yourself". It's "cleaner" to e.g., follow CQRS and/or modular monolith and keep your features' elements grouped together (including validators) and expose certain stuff to some global registry like WebApplicationBuilder."
Totally agree with you! I'm a huge fan of DDD, modular monoliths, microservices, CQRS, clean architecture etc. There is always an option to use pure Validot (without any integrations) and "do-it-yourself" approach, but it may be nice to have an option that uses an already build, public, and offical solutions.
"Swashbuckle is another story; the idea is great and I'd love to see that library coming out! If you have time and will, I'd rather encourage you to redirect your energy on this. Is that in any way dependent on the other bits that you mentioned?"
Well it depends on Validot.DependencyInjection and I think it doesn't need Validot.AspNetCore (I have to check it). Hovever, I consider it a bit unsafe when there is no "auto-magical" validation included. However, I need to rethink that because I have some solution in my mind but I'm not sure if it's good.
"As for DI and ASP.NET Core integration... I appreciate your efforts! Thanks for that! But also, this is exactly why I included "Let's have a discussion first" point in CONTRIBUTING document. I just feel bad writing all of this having a demoable bits of code done and ready."
I've forgotten to mention why I'm writing those libraries. Long story short, I need them in one of my projects that is moving request validation from FluentValidation to Validot (mainly because I've found that it is easier to do some validations in Validot than in FluentValidation, and it's worth to spend some time writing integrations). To sum up, I have to implement them regardless the decisions here, but I thought they may be worth sharing.
@bartoszlenar Are you open for a contribution that will add a
Validot.DependencyInjection
,Validot.AspNetCore
,Validot.Swashbuckle
packages? I'm currenly preparing those libraries internally, but I'm willing to make them public as part of official Validot repo.