qs.parse returns invalid key with brackets when using nested array query

[tintinthong]

EDIT TLDR:

This PR fixes a limitation of the depth that qs.parse is parsing. There is a default of depthLimit = 5 hence, our nested queries would parse key names with brackets outside. This PR configures, depthLimit = 10 and sets strictDepth = true ( a new feature shipped in qs) which errors out if qs.parse recurses too deep.

=====Below Description here is historical=====

Problem

When making complicated queries where any array nests within every array, qs.parse incorrectly parses the string st a key has brackets wrapped around it.

Fixing this problem unlocks the ability to nest every and any queries which is useful for complex querying in applications that will be used by ecosystem team.

    let query: Query = {
      filter: {
        on: {
          module: `${testRealmURL}book`,
          name: 'Book',
        },
        every: [
          {
            eq: {
              'author.firstName': 'Cardy',
            },
          },
          {
            any: [
              {
                eq: {
                  'author.lastName': 'Jones',
                },
              },
              {
                eq: {
                  'author.lastName': 'Stackington Jr. III',
                },
              },
            ],
          },
        ],
      },
      sort: [
        {
          by: 'author.lastName',
          on: { module: `${testRealmURL}book`, name: 'Book' },
        },
      ],
    };

Here is the test you can see a3ea344

logger.ts:97 Error: Your filter refers to nonexistent field "[author" on type {"module":"http://test-realm/test/book","name":"Book"}
    at IndexQueryEngine.walkFilterFieldPath (index-query-engine.ts:941:15)
    at async IndexQueryEngine.handleFieldArity (index-query-engine.ts:757:31)
    at async Promise.all (:4200/index 52)
    at async IndexQueryEngine.makeExpression (index-query-engine.ts:698:7)
    at async IndexQueryEngine.queryCards (index-query-engine.ts:146:23)
    at async Promise.all (:4200/index 1)

The cardsQuery after returned from qs.parse will look like this

{
  "filter": {
    "on": { "module": "http://test-realm/test/book", "name": "Book" },
    "every": [
      { "eq": { "author.firstName": "Cardy" } },
      {
        "any": [
          { "eq": { "[author.lastName]": "Jones" } },   <- PROBLEM should be 'author.lastName'
          { "eq": { "[author.lastName]": "Stackington Jr. III" } } <- PROBLEM should be 'author.lastName'
        ]
      }
    ]
  },
  "sort": [
    {
      "by": "author.lastName",
      "on": { "module": "http://test-realm/test/book", "name": "Book" }
    }
  ]
}

The issue errors out because pathSegments doesn't know how to handle brackets

boxel/packages/runtime-common/index-query-engine.ts

Line 914 in a398617

let pathSegments = path.split('.');

SOLUTION

I decided to post-parse the qs.parse object even before going into the realm-index.query-engine. Placing my fix in realm.ts was intentional because I thought that the issue was within qs.parse, although, its possibly easier if using one-liner removeOuterBracket just occured near pathSegments inside realm.ts (open to opinions here) since the recursion is already made.

I have also attempted to use qs.parse configuration but was unsuccesful bcos I don't think it works out-of-the-box

• allowDots configuration but it totally breaks the query structure query-engine is expecting since it converts keys to nested objects.
• The other one is to use my own custom decoder but qs.parse only allows decoding of one-level deep and I didn't think it was worthwhile to expose myself to a recursive qs.parse

[github-actions]

Host Test Results

    1 files  ±0      1 suites  ±0   20m 41s ⏱️ -57s
678 tests +2  677 ✔️ +2  1 💤 ±0  0 ❌ ±0 
683 runs  +2  682 ✔️ +2  1 💤 ±0  0 ❌ ±0 

Results for commit eaedefa. ± Comparison against base commit 6feb97a.

♻️ This comment has been updated with latest results.

[tintinthong]

.every is problematic because it expects to return a boolean. So, not all assertions are being run thru

[tintinthong]

pointer.concat doesn't even mutate the pointer. I would prefer to handle in another PR

[tintinthong]

I meant to come up with a better name here

[habdelra]

I noticed in the qs docs, this comment:

By default, when nesting objects qs will only parse up to 5 children deep.

Is this actually the problem, do these items reside 5+ levels deep? if they were higher up in the composition hierarchy would they be parsed correctly?

If so, there is a depth configuration in qs that will allow you to parse even deeper, and there is a strictDepth config that will throw when you try to go deeper than the config so that you'll get a clear error about what is actually wrong instead of an error on the side effect of what went wrong.

[tintinthong]

s this actually the problem, do these items reside 5+ levels deep? if they were higher up in the composition hierarchy would they be parsed correctly?

Yes it is! I recall looking into this and thought 5 was sufficient depth. The depth that works is 6! I think this solves my issue, any ideas what should be the default depth?

[habdelra]

s this actually the problem, do these items reside 5+ levels deep? if they were higher up in the composition hierarchy would they be parsed correctly?

Yes it is! I recall looking into this and thought 5 was sufficient depth. The depth that works is 6! I think this solves my issue, any ideas what should be the default depth?

Let’s do a strictDepth of 10. If we go beyond that, we’ll at least get a meaningful error message.

[tintinthong]

• I have updated the lock to pull the latest. Apparently, strictDepth is quite a recent feature [New] add strictDepth option to throw when input is beyond depth ljharb/qs#511
• I have added types in this PR upstream and will upgrade the @types/qs
• I have added a unit test for these under qs-test

[habdelra]

let's be a little more precise why we are ignoring. I usually do:

// @ts-expect-error no types available for XXX

this way when types do become available our lint will let us know that the following line doesn't need to be ignored anymore

[tintinthong]

Thanks. Just fixed

[tintinthong]

This has been merged DefinitelyTyped/DefinitelyTyped#71087 and deployed on @6.9.17 of qs. No need for ts-expect-error anymore