-
-
Notifications
You must be signed in to change notification settings - Fork 176
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
Register Validators as scoped instances rather than singleton #123
Comments
It's probably best to make this configurable, in the cases where you can have validators as a singleton, that would be preferable because it will have an impact on performance. There have been other discussions around the use of the container / how things are registered, and I imagine those discussions will continue for a while :) |
@JoeStead understand your comments around requiring it to be configurable. Let me get my head around it and see if I can make it configurable. Apparently I did send a PR to change it to scoped, I think it can be ignored. |
@mysticmind Looks like you are doing a bit of CQRS there. You have a race condition in your validator and your domain logic is leaking - two concurrent requests with the same I would strongly advise that message validators, your first line of defense after auth, work exclusively with the message and not have any dependencies on services. (There are always exceptions OC). My understanding is that fluent validator objects are quite expensive to instantiate due to compiled expressions etc as such should be singleton wrt to the life time of your app (I actually use static instances). I might be out of date with this understanding, but if I'm correct, the only time you might need instances if there is a some sort setting adjustable at runtime. |
@damianh All your points makes sense and totally agree that fluent validator objects are quite expensive to instantiate. Besides it is best to have clear separation of concerns as you rightly pointed out.
This is a good rule of thumb! Also @JoeStead indicated in earlier comment around the impact on performance. I am closing this. |
The ability to change the lifecycle of the registration using the DI container, in my opinion, is crucial; otherwise, it would render the use of Carter.NET in applications that are being migrated to minimal APIs unfeasible. It is quite common to have validators coupled with some sort of database access. For example, when checking the uniqueness of an email address or some other value. I understand that FluentValidation objects are quite expensive to instantiate, but in my case, and I believe in the case of others, it's a price we are willing to pay. The automatic registration of all validators in an application as Singletons becomes a deal-breaker, even affecting previous code, as it prevents the application from starting due to errors generated during the build of the DI container. One possibility would be to disable the automatic registration of validators, along with the ability to validate the request directly at the endpoint, and leave it to the developer to manually execute the validation procedure. |
I had a setup a validation as below wherein
RoleManager
via constructor injection is a scoped instance and this resulted in an errorCannot consume scoped service in Singleton
(validators are registered as Singleton withinCarterExtensions.cs
).It would be good to register validators as scoped instances.
@jchannon if you could confirm on this change, I would be happy to send my first PR to Carter :-)
The text was updated successfully, but these errors were encountered: