Skip to content
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

Relationship between UserRepository and TypeormRepositoryBase #20

Closed
estrehle opened this issue Jun 10, 2021 · 5 comments
Closed

Relationship between UserRepository and TypeormRepositoryBase #20

estrehle opened this issue Jun 10, 2021 · 5 comments

Comments

@estrehle
Copy link
Contributor

estrehle commented Jun 10, 2021

While working through your database implementation, I was wondering why UserRepository does not use TypeormRepositoryBase.findOne?

private async findOneByEmail(
email: string,
): Promise<UserOrmEntity | undefined> {
const user = await this.userRepository.findOne({
where: { email },
});
return user;
}

Shouldn't we do this instead:

const emailVO = new Email(email);
const user = await this.findOne({ email: emailVO });
return user;

And a second question: It seems like UserRepository.prepareQuery removes all query parameters except for id? Why?

// Used to construct a query
protected prepareQuery(
params: QueryParams<UserProps>,
): WhereCondition<UserOrmEntity> {
const where: QueryParams<UserOrmEntity> = {};
if (params.id) {
where.id = params.id.value;
}
return where;
}

@Sairyss
Copy link
Owner

Sairyss commented Jun 10, 2021

  1. This is just an example to show that you can use TypeOrm repos if you have specific queries that can't be simply queried using this.findOne(). In this case either one works, choose any that you like more.
  2. prepareQuery is meant for constructing a query from value objects, since you need to extract values from them. In this example use case we allow querying users only by ID. If you want to allow querying users by, lets say, country, you'd have to add
if (params.address?.country) {
  where.country = params.address.country
}

Keep in mind that this is an example implementation and might not be the best solution for all use cases.

@estrehle
Copy link
Contributor Author

Thank you!

To me it feels like in prepareQuery I would have to duplicate most of the logic from OrmMapper.toOrmProps, only that each line will be wrapped in a if (params...) { ... } clause. Maybe there is a way to re-use the logic from the mapper and use a more abstract if clause?

@Sairyss
Copy link
Owner

Sairyss commented Jun 10, 2021

The problem is that mapper requires all (or almost all) fields to be present to save the entity, but in a query all fields are optional. Also a query may have different forms, not just simple ones like discussed here. Here is an example of a prepareQuery from one of my projects:

      ...
      if (status) {
        receiver.status = status;
        sender.status = status;
      }
      if (params?.createdBefore) {
        receiver.createdAt = LessThanOrEqual(params.createdBefore);
        sender.createdAt = LessThanOrEqual(params.createdBefore);
      }
     return [sender, receiver];

As you can see this would be impossible to map just by using a mapper, since you can have operators in your query like <= or >=, or you may need to query in a joined tables (like sender and receiver in an example above)

@estrehle
Copy link
Contributor Author

estrehle commented Jun 10, 2021

Ah, I see! Now I understand why one would want a more flexible way of preparing queries.

My worry, however, is that this implementation suggests a generality that is not there. The repository exposes a method

findOne(params: QueryParams<EntityProps>)

where

type QueryParams<EntityProps> = DeepPartial<BaseEntityProps & EntityProps>

To anyone who has not studied prepareQuery in detail, this looks like a method that allows one to search the repository using arbitrary combinations of entity properties.

But when I call, say, UserRepository.findOne({ country: 'Germany' }) then the country param will be silently deleted and the result will be the same as if I had called UserRepository.findOne({ }). That could lead to nasty bugs.

My "solution" was to remove the find... methods (except for findById, which works the same for every entity) from TypeormRepositoryBase and resort to custom implementations in each repository, in the spirit of UserRepository.findOneByCountry(country: string).

@Sairyss
Copy link
Owner

Sairyss commented Jun 10, 2021

That's right, you have to adjust either a type to accept only the parameters specified in prepareQuery, or modify Repository to your liking. I'll think how to improve it in the future so it's less confusing.

@Sairyss Sairyss closed this as completed Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants