DamianEdwards / MiniValidation

A minimalist validation library for .NET built atop the existing features in `System.ComponentModel.DataAnnotations` namespace
MIT License
321 stars 25 forks source link

Consider returning A List or Enumerable for errors? #38

Open ziaulhasanhamim opened 1 year ago

ziaulhasanhamim commented 1 year ago

Currently 'TryValidate' method returns IDictionary<string, string[]>. If we look at the implementation of the 'TryValidate' method, it first creates Dictionary<string, List<string>> then it maps it to Dictionary<string, string[]> using the ToArray method. But can't it just return the Dictionary<string, List<string>>? What's the problem with it? Mapping it using ToArray causes it to create new arrays,

It's not like arrays are immutable. For immutable behavior IEnumerable or even better IReadOnlyCollection would be much better as they won't allocate new collections.

Maybe I'm missing something on the intention of returning arrays. Let me know.

DamianEdwards commented 1 year ago

It's specifically to allow direct compatibility with the APIs in ASP.NET Core, including Results.ValidationProblem and ProblemDetails.

I was considering switching to read-only interfaces, e.g. IReadOnlyDictionary<string, IReadOnlyList<string>> to better capture the intent/shape of the API directly but as you mention I'm also concerned about allocation overhead.

Given the nature of the underlying System.ComponentModel validation APIs it might not be possible to get the API shape and overhead ideal, but I do intend to revisit this.

ziaulhasanhamim commented 1 year ago

IReadOnlyDictionary<string, IReadOnlyList<string>> can allocate less than IDictionary<string, IReadOnlyList<string>> because lists can be directly assigned to IReadOnlyList.

DamianEdwards commented 1 year ago

This is effectively blocked by dotnet/aspnetcore#41899 as without that support in ASP.NET Core the results from MiniValidation can't easily be used when returning Problem Details results.