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

fix filter breaking instead of defaulting to _eq #10194

Merged
merged 1 commit into from Dec 1, 2021
Merged

Conversation

azrikahar
Copy link
Contributor

Fixes #10056

Reported bug

?filter[value]=5 used to work, but now it'll throw error 500 and it'll only work if we do ?filter[value][_eq]=5.

Investigation

At first it seemed like the "default to _eq" was removed, but it is actually still there:

https://github.com/directus/directus/blob/main/api/src/utils/apply-query.ts#L643-L645

Turns out it was an unintended side effect from the applyFilter refactoring in #9804, which aligns with the reports of it not working since 9.1.x.

Based on the old logic, when the value is not "objectLike", it'll be returned directly as is:

} else if (isObjectLike(object)) {
const res: Record<string, any> = {};
for (const key in object) {
const val = object[key];
if (isObjectLike(val)) {
res[key] = deepMap(val, iterator, context);
} else {
res[key] = iterator.call(context, val, key);
}
}
return res;
} else {
return object;
}

Solution

Added an if statement to return value without any modifcation when it's not an object. This prevents it from being "processed" by the Object.entries like Object.entries(5).reduce(()=> /* logic */, {}) which ended up returning {} instead.

Test

chrome_JRC7Sd3Pdf

@rijkvanzanten rijkvanzanten enabled auto-merge (squash) December 1, 2021 14:39
@rijkvanzanten rijkvanzanten merged commit 648b2d0 into main Dec 1, 2021
@rijkvanzanten rijkvanzanten deleted the issue/10056 branch December 1, 2021 14:42
@Oreilles
Copy link
Contributor

Oreilles commented Dec 1, 2021

Could we do instead:

if (!isObjectLike(filter)) return { _eq:  parseFilterValue(filter) };

And remove the getOperation function from applyQuery ?
I think this should be part of the filter parsing - I didn't know that ?filter[field]=5 was expected to work, but right now it won't work for dynamic variables (?filter[field]=$CURRENT_USER) for example, because parseFilterValue would be bypassed (same goes for the other checks that are being done in parseFilterValue).

@rijkvanzanten
Copy link
Member

@Oreilles Totally. I'd rather have the API "fix" the format ahead of time, and then use a consistent object across the rest of the codebase. Maybe we can move your proposed formatting logic into the SanitizeQuery function, so we can have the rest of the codebase expect the _eq usage?

Same goes for non-explicit _and usage by the way, I'd like

{
  "A": 1,
  "B": 2
}

to be sanitized and used as

{
  "_and": [
    {
      "A": {
        "_eq": 1
      }
    },
    {
      "B": {
        "_eq": 2
      }
    }
  ]
}

That makes filter usage from things like the SDK and REST API way nicer, as you'd be able to do:

GET /items/articles
  ?filter[A]=1
  &filter[B]=2

@eikaramba
Copy link
Contributor

found this ticket. could it be that this default _eq handling does not work for graphql?

with

query {
   posts(filter: { name: "test" }) {
     id
     name
  }
}

i only get an graphql error saying Expected value of type \"string_filter_operators\", found \"test\"."
using directus 9.2.2 (it seems this fix here is merged there already)

@rijkvanzanten
Copy link
Member

@eikaramba GraphQL as a spec is way pickier and doesn't allow for union input types. Therefore, the filter attribute can only support the "full" version that uses the explicit operators.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maximum call stack size exceeded when using filter query param
4 participants