✨ Introduce new custom where condition construction #66
Conversation
s0
added
the
ready for review
s0
force-pushed
the
improved-conditions
branch
3 times, most recently
from
7f98dc4 to
8ad6769
Compare
Given a knex query builder, and one of our own custom conditions, return the prepared condition to be used directly by knex. There are a few reasons we do this rather than simply using knex query builder directly: * the query builder does not offer good type-safety (for example whereIn does not require the type of the values to match the type of the column) * we find ourselves needing to use query builders often for very simple conditions that could be abstracted + simplified However, we retain the ability to use a query builder when needed.
Some of our unit tests iterate through every table (e.g. to check the schema for each table). Combining tables along with util properties made this more complicated, so we also need to provide a way to easily get all tables. Beyond this, we can tidy up the types a bit more.
s0
force-pushed
the
improved-conditions
branch
from
ff9b6dd to
5edc3a1
Compare
| */ | ||
| export namespace ConditionSymbols { | ||
| export const BUILDER = Symbol('builder'); | ||
| export const AND = Symbol('and'); |
There was a problem hiding this comment.
What exactly would be the benefit of using AND, besides completeness with having OR? When properties are supplied to where, AND is the default. Is there some construct where this would be useful, that I am not seeing?
If not, I would like to drop AND because I would not like to keep telling people during reviews: "Hey, don't use AND since it is a default behavior anyway, let's simplify this`
There was a problem hiding this comment.
Yeah, in cases where we want to use OR or a query builder in one of the AND clauses, or if we want to combine some query builder stuff with some normal property stuff. Something like:
(A or B) and (B or C) or (A or B) and C.
something like this in the unit tests: https://github.com/UN-OCHA/hpc_service/blob/1a0166ffb0d5fe47934de8a5cf6b51566a65f8c5/test/hpc-api-core/src/db/util/conditions.spec.ts#L129-L136
Or:
{
[Cond.AND]: [
{
id: 123,
},
{
[Cond.BUILDER]: (qb) => qb.whereNotNull('id'),
},
],
}But yeah, in the cases where we're ONLY using AND, and not using an OR or BUILDER inside of an AND clause, we'd want to use the simpler / cleaner construction.
There was a problem hiding this comment.
Ok, I understand. I didn't realize immediately that
if (OverallConditions.isQueryBuilderCondition(condition)) {
builder.where(condition[Cond.BUILDER]);
}swallows "plain" conditions, so I thought you could write the same thing as:
{
id: 123,
[models.v4.Cond.BUILDER]: (qb) => qb.whereNotNull('id'),
}
Pl217
added
needs minor changes
s0
added
ready for review
| }; | ||
|
|
||
| export type IsNullCondition = { | ||
| [Op.IS_NULL]: boolean; |
There was a problem hiding this comment.
I was thinking that we should use {propertyName: null} for propertyName IS NULL and introduce [Op.IS_NOT_NULL]: null or [Op.NE]: null, but I am fine with what you used.
It is saying that NULL should be treated as a special case, and even though there are two ways to write IS NULL now ({propertyName: null} is more likely to be used), let's not complicate any further and use what you suggested.
As discussed earlier today, this PR introduces a new abstraction over where conditions (with support for nested conjunctions (AND) and disjunctions (OR)), to allow for better type-safety with data used for constructing queries, and to make our most common use-cases easier to write.
The queries generated by this are unit-tested in hpc_service: UN-OCHA/hpc_service#2529. (This is needed until #26 is merged)
This PR makes use of query builders in the where clauses type errors, using query builders in conditions is still possible, but now requires us to explicitly use
[Cond.BUILDER]. This should help us catch and rewrite all instances of usage of the old builder when not neccesary.(this will cause type-error conflict with #57, so whichever is merged last will need to be rebased and fixed)