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

Allow find() to accept more complex where clauses #4110

Closed

Conversation

Benjamin-Dobell
Copy link
Contributor

@Benjamin-Dobell Benjamin-Dobell commented May 8, 2019

Description

I've introduced a new class called WhereFactory whose implementation is identical to that of the existing Brackets, it only varies in semantics i.e. QueryBuilder won't insert brackets around a WHERE clause generated by a WhereFactory.

Brackets now inherits from WhereFactory and defines no additional functionality of its own, it's purely semantic i.e. QueryBuilder will put brackets around a WHERE clause generated by Brackets.

Any APIs that took Brackets now take WhereFactory, meaning they'll continue to accept Brackets in addition to WhereFactory, so the API changes are non-breaking / backward compatible.

Motivation

Simply put, more elaborate WHERE clauses returning entities - not just for type safety, but also to leverage functionality like eager loading.

With this PR we now support (to my knowledge) all possible WHERE conditions when using find; including brackets, sub-queries, combinations of string and object literal (property) conditions etc.

e.g. Here we load user entities, eager load in their posts, but restrict to:

  • User's that have posts (no users with zero posts).
  • If the current user isn't an admin, we chain (AND) in the condition:
    • Only consider public posts, OR,
    • Posts made by friends.
function findAccessibleUserPosts(user : User): Promise<User[]> {
	return getRepository<User>(User).find({
		join: {
			alias: 'user',
			leftJoinAndSelect: {
				post: 'user.posts',
			}
		},
		where: new WhereFactory((qb: WhereExpression) => {
			qb.where("user.id = post.user_id")

			if (user.role !== Role.admin) {
				qb.andWhere(new Brackets(qb => {
						qb.where("post.public = true"})
							.orWhereInIds(user.friendIds)
					}
				))

			}
		}),
	})
}

which would spit out something like the following:

WHERE "user"."id" = post.user_id AND (post.public = true OR "user"."id" IN (?, ?, ?))

@Benjamin-Dobell
Copy link
Contributor Author

Just force pushed away a white-space only change, no functional changes made.

@pleerock
Copy link
Member

pleerock commented May 9, 2019

Thanks for contribution!

For some reason I don't like naming of WhereFactory. Can we came up with better name. Maybe RawWhere ? WhereExpression looks good alternative but looks like its already busy. Now Im thinking that we should have called what is called WhereExpression now with a better name.

Also, I think I would have issues when I merge this into next branch. Can you make same changes on next branch too?

@Benjamin-Dobell
Copy link
Contributor Author

I think abd4212 might actually make this PR mostly irrelevant.

This PR essentially allows you to construct any SQL query you want using find(). However, find() has had functionality removed on next and QueryBuilder has had functionality enhanced.

With the decreased functionality offered by find() I don't believe offering additional WHERE flexibility makes all that much sense. Given QueryBuilder will be able to do everything find() can do (i.e. eager load relationships) and more, then it seems like the way forward.

Does my understanding seem correct? If so, I'll happily close this PR.

@pleerock
Copy link
Member

pleerock commented May 9, 2019

Yes, maybe you are right. Probably its a good idea left what you wanted to do on query builder itself.

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

Successfully merging this pull request may close these issues.

None yet

2 participants