Using two predicates inside one object ignores one of them

[joeytwiddle]

We are trying to count all records with createTime between two dates. We are using a MongoDB datasource.

  const newUserCount = await this.userRepository.count(dateFilter);

We tried the following filter, but it does not do what it looks like it should do. Only the gte is being respected. The lt is being ignored, so we end up counting more results that we expected to.

  const badDateFilter = {
      createTime: { gte: fromTime, lt: toTime }
  };

Fortunately, if we use an and clause then we get the results we wanted.

  const goodDateFilter = {
      and: [
          { createTime: { gte: fromTime } },
          { createTime: { lt: toTime } }
      ],
  };

So there is a solution.

The real problem is that the lt was being silently ignored, so this went undetected in our codebase for some time.

Possible solutions

1. If that badDateFilter is illegal, then we could change the TypeScript types to reject it. But I'm guessing that it's a legitimate filter, it's just not being handled well at runtime.

2. (preferred) Fix whatever is causing the lt to be ignored, so the badDateFilter will actually work as intended.

3. If it's not possible to fix it, then instead throw a runtime error from the library ("This filter cannot be applied correctly"), so that developers will know they need to use an and clause instead of combining multiple conditions in one object.

---

Footnotes

In the examples, createTime, fromTime and toTime are all Date objects.

This badDateFilter is quite familiar for MongoDB / mongoose users, although they use $gte and $lt.

We are using the following packages:

├── @loopback/boot@1.6.0
├── @loopback/build@2.1.0
├── @loopback/cli@1.27.0
├── @loopback/context@1.25.0
├── @loopback/core@1.12.0
├── @loopback/repository@1.16.0

[emonddr]

@jannyHou , please take a look? thx.

[emonddr]

@mschnee any chance you can look into this one? thx

[agnes512]

From the docs And and Or Operator, I think and/or should be used in the query when there are more than one conditions. We will make it more clear in the section Working with data in LB4 ( see PR #4970).

Not sure how much effort it needs to have such warinings, thoughts? @raymondfeng @jannyHou

[jannyHou]

The bad filter is allowed due to type PredicateComparison.

Start with the where clause's type:

export type Where<MT extends object = AnyObject> =
  | Condition<MT>
  | AndClause<MT>
  | OrClause<MT>;

The AndClause<MT> restricts the correct filter, which is good 👌
The Condition<MT> has PredicateComparison<MT[P]> as the type for each field, but it allows the filter object contain any number of comparison operators, which results in the bad filter passes the type check.

export type Condition<MT extends object> = {
  [P in KeyOf<MT>]?:
    | PredicateComparison<MT[P]> // {x: {lt: 1}}
    | (MT[P] & ShortHandEqualType); // {x: 1},
};

export type PredicateComparison<PT> = {
  eq?: PT;
  neq?: PT;
  gt?: PT;
  gte?: PT;
  lt?: PT;
  lte?: PT;
  inq?: PT[];
  nin?: PT[];
  between?: [PT, PT];
  exists?: boolean;
  like?: PT;
  nlike?: PT;
  ilike?: PT;
  nilike?: PT;
  regexp?: string | RegExp;
  // [extendedOperation: string]: any;
};

To fix it, I think we need to ALLOW ONLY ONE operator in PredicateComparison

[joeytwiddle]

In that case, the type could look like this:

export type PredicateComparison<PT> =
  | { eq: PT }
  | { neq: PT }
  | { gt: PT }
  | { gte: PT }
  | { lt: PT }
  | { lte: PT }
  | { inq: PT[] }
  | { nin: PT[] }
  | { between: [PT, PT] }
  | { exists: boolean }
  | { like: PT }
  | { nlike: PT }
  | { ilike: PT }
  | { nilike: PT }
  | { regexp: string | RegExp };
  // | { [extendedOperation: string]: any };

Does that look okay?

I tried making a branch for it: joeytwiddle/predicate-comp-type-1 (Compare)

But it fails to build: packages/filter/src/query.ts:329:5 - error TS2322: Type 'MT[K]' is not assignable to type '{ exists: boolean; } | { regexp: string | RegExp; } | { eq: MT[K]; } | { neq: MT[K]; } | { gt: MT[K]; } | { gte: MT[K]; } | { lt: MT[K]; } | { lte: MT[K]; } | ... 12 more ... | undefined'.

I can work around it by writing val as any but then I get a bunch of errors from other places!

[stale]

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

[stale]

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.