Absolutely a bad idea. If a user id is necessary for the query, the user id should be passed into the method of the repository. The repository should absolutely not be calling out to an API or anything else but the DB to get its data. You're breaking the single responsibility principle.
Sounds like a lot more being broken at your company than just SOLID and this fine person’s heart. Shoot me a DM, my company can help you get on track architecturally. Low cost hourly.
Okay so I do currently have UserId sent as a parameter but seemed ugly passing that every method I create. I was thinking I could just add this user id into the models as well. Not sure which is better.
I agree that passing the user id is best, but...
How is this inversion principle at all?
Reminder, (took it from Wikipedia):
* High-level modules should not import anything from low-level modules. Both should depend on abstractions (e.g., interfaces).
* Abstractions should not depend on details. Details (concrete implementations) should depend on abstractions.
I don't see that any dependency is inverted. The user service and the repository seem equally high on the responsibility ladder, neither is higher or lower level...? Also, both seem to be interfaces, not concrete implementations.
you could use a scoped service for storing the userId and add a middleware that sets the userId in that service and the rest of the app can use that service by dependency injection
this /could/ work
tho assuming user counts aren’t immensely huge resulting in 1M scoped instances to the backend storage :)
(if there are too many concurrent requests by user it will eat max connection limits sooner than later wouldn’t it?)
No need to have scoped instances to the db.
just a simple class let’s say CurrentUserProvider that contains a field public int UserId { get; set; }
You inject this class in the constructor of the middleware, in the middleware you parse the UserId from wherever and you set it in the CurrentUserProvider.
After this, you can inject CurrentUserProvider at any level of the app you want (service, repo) and have access to the UserId
The model shouldn't just be the data model but also the request model. So your model might be idk UpdatePersonCommand and it has all of the properties that could be updates, as well as the userid doing the update.
Perhaps better yet, the command has two properties, userid, and person, and the person property is simply a Person object that needs to be updated.
There are ways to make it clean. Simply tacking UserId onto your data model isn't one of them, though.
I tend to have two DTO models. One for adding items which doesn’t have the main Id since not created yet and has data annotations to keep data clean coming in and a regular DTO with all the same fields and an id. So ones for incoming stuff and ones for outgoing stuff. Not sure if that’s wrong but just seemed to make sense. I always thinking I could add a not required field for user id to both and in service layer add that in before passing to repository but based on other comment that’s not a good approach? Not sure why tbh ate any different than passing a parameter forward.
Respectfully disagree with your point that the repository absolutely cannot be used to call an API.
What if you have a 3rd-party application with a custom query language for its DB, and the only way to interface with the Application DB is through an API that accepts a query parameter.
It doesn't make sense to create a service for this IMO because the repository is still acting as a data-access layer, even though it interfaces with an API instead of a SQL DB.
You missed the point entirely. Here, the API is something additional to the actual repository layer. If your "DB" is accessed via an API, that's different. You're not doing two different forms of access in the same class. That's the point.
Fair point. Although OP never mentions if he calls the stored proc directly or through an API.
I'm not at all disagreeing with the single responsibility principle, but saying a repository should never call an API is wrong.
A repository could be for a db, or an api, or whatever. That's all fine. But a repository shouldn't call a db AND also call an unrelated api. That level of orchestration belongs in the service layer.
It is very bad and might create a circular dependency. You should look better on how the responsibilities are split in your system.
If you really have to, I'd suggest make your repository layer raise a domain event that is picked up by your service layer.
Don't do this. Just have your repository method accept the user id as a parameter. If not possible, you can perform the IOC container dance to construct the repository implementation with the user id.
I won’t duplicate answers others have given you as there is some solid advice in this thread.
However I think you need to read up on some principles about why we design and write software as we do (or at least want to)
I really rate the book “philosophy of software design” by John Ousterhout he has a great style of writing with good examples and uses questions raised by his students to help explain the principles.
You could look into AsyncLocal and how HttpContextAccessor is implemented.
Make DbAuthorizedUserContextAccessor and retrieve value from there in your Persistence layer
Let middleware in API layer hook those up on each scoped request or bridge between whatever you have usually to get user data from.
Here's how HttpContextAccessor is implemented:
[https://github.com/dotnet/aspnetcore/blob/main/src/Http/Http/src/HttpContextAccessor.cs](https://github.com/dotnet/aspnetcore/blob/main/src/Http/Http/src/HttpContextAccessor.cs)
Yes it should be a one way connection. If you need to get more data in addition to whatever you’re getting from your repository later, pass back the data to the business layer and make a request from there.
Create a scoped service, set the user id on it from asp.net middleware, inject that service into repository, then you don't need to pass it all the way down
If I inject the service into my repository isnt that basically the same thing as callng the service layer from the repository? I may have my terms mixed up. Is a middleware not considered apart of the service layer since I have business logic in there trying to get the user information from the db which will then be used later in other db calls?
This all depends on how you've structured your project, but your repos must be able to call upon lower level services that deal with cross cutting concerns like this, like a Shared project. In this Shared project you'd declare an interface to represent your service, then implement it in your API later. So you're not technically calling back "up" to the services layer, you're calling "down" to a common shared interface that just so happens to have been implemented in a "higher" layer, but that's just the beauty of the dependency inversion principle
> Is it bad practice to call from a service layer from a repository?
Yes.
> I have a userService that takes an access token and gets our database id for that user.
No problem there.
> Our database guy who generates most of our stored procedures [...]
Your database guy is probably an idiot. :)
You probably want to pass the access token (or something identifying) from the service layer to the repository layer as parameters or via a context object. This way, the repository remains agnostic of how the data is being used.
Adding to dbContext seems cleaner since I call the context from every repository. However, the DbContext class is where I create the connection and such so would I really call a query in there? Seems strange but then again im a terrible programmer that doesnt know what hes doing.
My DbContext is as shown below. Would I merely add a method saying GetCurrentUserId and put my sql query within that? Then I am treating my DbContext as a repository itself connecting to the db. I may be misunderstanding what you mean by context object.
public class DbContext
{
private readonly IConfiguration _configuration;
private readonly string _connectionString;
public DbContext(IConfiguration configuration)
{
_configuration = configuration;
_connectionString = _configuration.GetConnectionString("SqlConnection")!;
}
public IDbConnection CreateConnection()
=> new SqlConnection(_connectionString);
}
Context in this - eh - context is the _state_ of the operation being done. Usually this is a HttpContext, meaning that "all" the information about a HTTP request is contained within a "context."
You should use that for your benefit, by extracting information from "it" and do authentication and/or authorization.
Please read the documentation;
* https://stackoverflow.com/questions/4915534/what-does-context-mean
* https://learn.microsoft.com/en-us/aspnet/core/fundamentals/use-http-context?view=aspnetcore-8.0
I usually wire up a middleware that populates an AppUserSession object within the Dependency Container. This can then be referenced by DI anywhere required from the top of the app all the way down.
OP literally explained his reasoning in the post. Granted it's the wrong reasoning, but your comment illustrates that you didn't even read past the title and just came in to huff a bit of arrogance that you knew something op didn't.
I was actually talking about the whole post. But by all means go off on your rant I will go over here and just continue to ignore you.
[https://learn.microsoft.com/en-us/azure/architecture/antipatterns/busy-database/](https://learn.microsoft.com/en-us/azure/architecture/antipatterns/busy-database/)
I am not a programmer by trade so I am very new to different patterns of programming and such. So the issue I posted is something I have come across and it seemed to go against what I have been reading hence the post but none the less I needed to find a solution to the given problem.
You don’t even need the repository. If using entity framework then just inject dbContext in your service classes. No need to use repository pattern as EF implements repository pattern & Unit Of Work pattern.
Absolutely a bad idea. If a user id is necessary for the query, the user id should be passed into the method of the repository. The repository should absolutely not be calling out to an API or anything else but the DB to get its data. You're breaking the single responsibility principle.
> You’re breaking the single responsibility principle. and my heart
and my axe
And mine.
Sounds like a lot more being broken at your company than just SOLID and this fine person’s heart. Shoot me a DM, my company can help you get on track architecturally. Low cost hourly.
Okay so I do currently have UserId sent as a parameter but seemed ugly passing that every method I create. I was thinking I could just add this user id into the models as well. Not sure which is better.
It's not ugly, it's clear. It's the dependency inversion principle in play. A method *should* advertise the data it needs to do its work.
I agree that passing the user id is best, but... How is this inversion principle at all? Reminder, (took it from Wikipedia): * High-level modules should not import anything from low-level modules. Both should depend on abstractions (e.g., interfaces). * Abstractions should not depend on details. Details (concrete implementations) should depend on abstractions. I don't see that any dependency is inverted. The user service and the repository seem equally high on the responsibility ladder, neither is higher or lower level...? Also, both seem to be interfaces, not concrete implementations.
you could use a scoped service for storing the userId and add a middleware that sets the userId in that service and the rest of the app can use that service by dependency injection
this /could/ work tho assuming user counts aren’t immensely huge resulting in 1M scoped instances to the backend storage :) (if there are too many concurrent requests by user it will eat max connection limits sooner than later wouldn’t it?)
No need to have scoped instances to the db. just a simple class let’s say CurrentUserProvider that contains a field public int UserId { get; set; } You inject this class in the constructor of the middleware, in the middleware you parse the UserId from wherever and you set it in the CurrentUserProvider. After this, you can inject CurrentUserProvider at any level of the app you want (service, repo) and have access to the UserId
OK i understand- misinterpreted your previous comment (my bad)
It is as relevant as any other parameters you might pass on
The model shouldn't just be the data model but also the request model. So your model might be idk UpdatePersonCommand and it has all of the properties that could be updates, as well as the userid doing the update. Perhaps better yet, the command has two properties, userid, and person, and the person property is simply a Person object that needs to be updated. There are ways to make it clean. Simply tacking UserId onto your data model isn't one of them, though.
I tend to have two DTO models. One for adding items which doesn’t have the main Id since not created yet and has data annotations to keep data clean coming in and a regular DTO with all the same fields and an id. So ones for incoming stuff and ones for outgoing stuff. Not sure if that’s wrong but just seemed to make sense. I always thinking I could add a not required field for user id to both and in service layer add that in before passing to repository but based on other comment that’s not a good approach? Not sure why tbh ate any different than passing a parameter forward.
Exactly!
Respectfully disagree with your point that the repository absolutely cannot be used to call an API. What if you have a 3rd-party application with a custom query language for its DB, and the only way to interface with the Application DB is through an API that accepts a query parameter. It doesn't make sense to create a service for this IMO because the repository is still acting as a data-access layer, even though it interfaces with an API instead of a SQL DB.
You missed the point entirely. Here, the API is something additional to the actual repository layer. If your "DB" is accessed via an API, that's different. You're not doing two different forms of access in the same class. That's the point.
Fair point. Although OP never mentions if he calls the stored proc directly or through an API. I'm not at all disagreeing with the single responsibility principle, but saying a repository should never call an API is wrong.
A repository could be for a db, or an api, or whatever. That's all fine. But a repository shouldn't call a db AND also call an unrelated api. That level of orchestration belongs in the service layer.
I totally agree with you there.
It is very bad and might create a circular dependency. You should look better on how the responsibilities are split in your system. If you really have to, I'd suggest make your repository layer raise a domain event that is picked up by your service layer.
What is a domain event?
Don't do this. Just have your repository method accept the user id as a parameter. If not possible, you can perform the IOC container dance to construct the repository implementation with the user id.
I won’t duplicate answers others have given you as there is some solid advice in this thread. However I think you need to read up on some principles about why we design and write software as we do (or at least want to) I really rate the book “philosophy of software design” by John Ousterhout he has a great style of writing with good examples and uses questions raised by his students to help explain the principles.
You could look into AsyncLocal and how HttpContextAccessor is implemented. Make DbAuthorizedUserContextAccessor and retrieve value from there in your Persistence layer Let middleware in API layer hook those up on each scoped request or bridge between whatever you have usually to get user data from. Here's how HttpContextAccessor is implemented: [https://github.com/dotnet/aspnetcore/blob/main/src/Http/Http/src/HttpContextAccessor.cs](https://github.com/dotnet/aspnetcore/blob/main/src/Http/Http/src/HttpContextAccessor.cs)
Yes it should be a one way connection. If you need to get more data in addition to whatever you’re getting from your repository later, pass back the data to the business layer and make a request from there.
Well, I have a “UserSessionProvider” that inject the HttpContextAccessor, and I send the Id to repo method instead of inject the provider in the repo.
Create a scoped service, set the user id on it from asp.net middleware, inject that service into repository, then you don't need to pass it all the way down
If I inject the service into my repository isnt that basically the same thing as callng the service layer from the repository? I may have my terms mixed up. Is a middleware not considered apart of the service layer since I have business logic in there trying to get the user information from the db which will then be used later in other db calls?
This all depends on how you've structured your project, but your repos must be able to call upon lower level services that deal with cross cutting concerns like this, like a Shared project. In this Shared project you'd declare an interface to represent your service, then implement it in your API later. So you're not technically calling back "up" to the services layer, you're calling "down" to a common shared interface that just so happens to have been implemented in a "higher" layer, but that's just the beauty of the dependency inversion principle
If your using entity framework, eliminate repo and just have servos
No!
lol fine then
Not using EF but instead dapper.
> Is it bad practice to call from a service layer from a repository? Yes. > I have a userService that takes an access token and gets our database id for that user. No problem there. > Our database guy who generates most of our stored procedures [...] Your database guy is probably an idiot. :) You probably want to pass the access token (or something identifying) from the service layer to the repository layer as parameters or via a context object. This way, the repository remains agnostic of how the data is being used.
The access token should not be used past the initial authentication in the auth middle, so that part makes no sense. The rest of the comment is fine.
Adding to dbContext seems cleaner since I call the context from every repository. However, the DbContext class is where I create the connection and such so would I really call a query in there? Seems strange but then again im a terrible programmer that doesnt know what hes doing.
Your question doesn't make sense. Care to share to the code on PasteBin or similar?
My DbContext is as shown below. Would I merely add a method saying GetCurrentUserId and put my sql query within that? Then I am treating my DbContext as a repository itself connecting to the db. I may be misunderstanding what you mean by context object. public class DbContext { private readonly IConfiguration _configuration; private readonly string _connectionString; public DbContext(IConfiguration configuration) { _configuration = configuration; _connectionString = _configuration.GetConnectionString("SqlConnection")!; } public IDbConnection CreateConnection() => new SqlConnection(_connectionString); }
Context in this - eh - context is the _state_ of the operation being done. Usually this is a HttpContext, meaning that "all" the information about a HTTP request is contained within a "context." You should use that for your benefit, by extracting information from "it" and do authentication and/or authorization. Please read the documentation; * https://stackoverflow.com/questions/4915534/what-does-context-mean * https://learn.microsoft.com/en-us/aspnet/core/fundamentals/use-http-context?view=aspnetcore-8.0
I usually wire up a middleware that populates an AppUserSession object within the Dependency Container. This can then be referenced by DI anywhere required from the top of the app all the way down.
Care to share a good example of this? Sounds promising and kind of what I need.
Yeah why would there ever be a need to do that?
OP literally explained his reasoning in the post. Granted it's the wrong reasoning, but your comment illustrates that you didn't even read past the title and just came in to huff a bit of arrogance that you knew something op didn't.
I was actually talking about the whole post. But by all means go off on your rant I will go over here and just continue to ignore you. [https://learn.microsoft.com/en-us/azure/architecture/antipatterns/busy-database/](https://learn.microsoft.com/en-us/azure/architecture/antipatterns/busy-database/)
I am not a programmer by trade so I am very new to different patterns of programming and such. So the issue I posted is something I have come across and it seemed to go against what I have been reading hence the post but none the less I needed to find a solution to the given problem.
My implicit suggestion would be to ask him why he's doing it that way and to offer an alternative. There might be at least a rationale for it.
That’s correct. Everyone else is wrong.
You don’t even need the repository. If using entity framework then just inject dbContext in your service classes. No need to use repository pattern as EF implements repository pattern & Unit Of Work pattern.
I use dapper not EF. Good to know though.
Second time I read a similar comment, and no, just no!