Your repository doesn't break the "clean architecture", which identifies entities as the core, requires dependency inversion, but doesn't impose to use repositories.
But your repository seems to break the clean code principles, and in particular:
- the single responsibility principle: you pack together things that have different reasons to change:
Payment
(which could change for example because of new commercial practices or new banking standards) and User
(subject to internal policy, GDPR concerns, or identity provider requirements);
- the interface segregation principle: you force the clients of your repository to know
Payment
's and User
's interface, even if they do not need them (e.g. if a later use case would produce a report on the total of payments per day the last month).
Your approach seems not either in line with Domain Driven Design principles, which suggest a repository for a single Aggregate :
In DDD you identify Entities, and group them into independent Aggregates that are objects that have to always be accessed via a single Aggregate Root.
But in your case, you have two independent Entities that each constitutes its own Aggregate:
- It is not possible to see
Payment
as an aggregate containing a dependent User
, since the latter may exist prior to having made any payment.
- It is not either possible to consider
User
as an aggregate with dependent Payments
, because this would mean that you could identify a payment only in relation to a user, which is not a realistic case for payment systems.
Edit: additional infos
The repository concept intends to provide a level of abstraction that lets you handle persistent objects as if they were in a collection in memory, and without caring about the db.
With this architectural pattern, you'd rather reference other aggregates by identity (value). The typical scenario is to get Payments
by querying the PaymentRepository
. Then the payment's UserId
would be used to get the user with UserRepostitory.getById()
. Or alternatively you'd just use the PaymentId
to query a payment from a user by invoking UserRepostitory.getByPaymentId()
.
The advantage of the identity is that it allows to decouple Entities in very complex models: passing all possible kind of objects to a repository (instead of a simple id) would cause an explosion of interface dependencies. Similarly, if related objects would systematically be loaded by the repository (as you think to expect), then you could have a snowball effect: Imagine you'd have entities representing nodes and edges of a graph. Whatever node you'd get from the repository, you'd end-up with the full graph in memory!
This being said, I don't know your model nor your constraints and I do not judge. It's up to you to decide what's the most suitable architecture. You may perfectly deviate from the rules if you have valid arguments and understand the consequences. By the way, this question about repository performance may interest you.