Fix undefined filter value

[MattieBelt]

What does it do?

Throw an error when a filter value is undefined instead of skipping it/filtering it out.

Examples

await strapi.query('category').findOne({ id: 1 });

Returns 200 with the first entry with id=1

await strapi.query('category').findOne({ id: undefined });

Returns 400: The field 'id' in your where filter has undefined as value.

await strapi.query('category').findOne({ unknownField: 1 });

Returns 400: Your filters contain a field 'unknownField' that doesn't appear on your model definition nor it's relations

await strapi.query('category').findOne({ undefinedUnknownField: undefined });

Returns 400: The field 'undefinedUnknownField' in your where filter has undefined as value.

Why is it needed?

To let people know they used an undefined value to find records.

Related issue(s)/PR(s)

Fixes #8201

[codecov]

Codecov Report

Merging #8490 (d223c84) into master (d7d3fd1) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #8490      +/-   ##
==========================================
- Coverage   35.09%   35.08%   -0.01%     
==========================================
  Files        1308     1308              
  Lines       14456    14460       +4     
  Branches     1438     1439       +1     
==========================================
  Hits         5074     5074              
- Misses       8473     8476       +3     
- Partials      909      910       +1     

Flag	Coverage Δ
front	26.66% <ø> (ø)
unit	54.87% <0.00%> (-0.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/strapi-utils/lib/build-query.js	17.80% <0.00%> (-1.04%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53c7fb0...d223c84. Read the comment docs.

[strapi-cla]

All committers have signed the CLA.

[alexandrebodin]

Hi @MattieBelt we merged some changes. you can reapply your improvements now :)

[MattieBelt]

Hi @alexandrebodin, thanks for letting me know! I'll look into it as soon as I have some time!

[alexandrebodin]

Hi @MattieBelt did you have time to check the conflicts ?

[MattieBelt]

@alexandrebodin looking into it now

[MattieBelt]

Resolved and updated. Ready to be merged @alexandrebodin

[alexandrebodin]

Thanks for the updates. I'm currently wondering if we should categorize it as a bug fix or a breaking change as it will start throwing errors in place were it didn't previsouly. I'm leaning towards bug. wdyt ?

[MattieBelt]

I wouldn't call it an breaking change because it should throw an error 'in this place' so it's indeed more a bug fix.

If someone's application breaks because he receives errors it will only help, because it will show the application was broken and was giving 'false' results back 😉

[alexandrebodin]

LGTM. Thanks 💯

[Fensterbank]

@MattieBelt I don't understand, why this was implemented.
I thought setting filters as undefined to let them be ignored is a pretty common pattern.

If something is undefined, it should just be ignored as if the key would not be existing.
You also don't throw an error if I send a PUT request with undefined values, they just won't get touched. So why throwing errors here?

I have a lot of code like this, to make queries based on settings or relations, so e.g. if a user has branches referenced, we only show the branches he is referenced to, if not, we set the value to undefined and it will be ignored.

   const user = await strapi.query('user', 'users-permissions').findOne({ id: ctx.state.user.id }, ['branches']);
   const branches = user.branches.map(b => b.id);
    
   const query = {
      ...ctx.query,
      // if the user is referenced to branches, we restrict this by filter
      'id_in': branches.length > 0 ? branches : undefined,
    };

Now I have to filter undefined keys out of my query object to prevent that error.

If someone's application breaks because he receives errors it will only help, because it will show the application was broken and was giving 'false' results back 😉

No. Setting a key to undefined is mostly done by purpose and that undefined keys are ignored is a well-known standard pattern.
I would not say, the application is broken. If an application does care about undefined values (what Strapi is doing now), it is broken. 😉

Thanks for the updates. I'm currently wondering if we should categorize it as a bug fix or a breaking change as it will start throwing errors in place were it didn't previsouly.

Yes, it should have be categorized as breaking change. I have around 5 productive Strapi applications out there, which will break with this update.

[derrickmehaffy]

well-known standard pattern

Can you provide some context to this because I have never seen this implemented in the examples.

---

Based on the code example provided it looks like this is some kind of security example where you are restricting users access to information based on their branch. In the case of security nothing should be undefined, you should know and expect all input/output.

Can you provide some other examples where you are using this so we can get a better understanding of what you are intending.

---

On that note I do understand from your perspective this could be a breaking change but we don't generally recommend that you filter based on undefined.

[Fensterbank]

hi @derrickmehaffy,
thanks for your reply.

well-known standard pattern

Can you provide some context to this because I have never seen this implemented in the examples.

I ment that undefined as value is ignored in most cases, but normally doesn't result in errors.

const data = {
  hello: 'world',
  foo: null,
  bar: undefined,
}

• If we make a JSON.stringify(data), the property bar won't be part of the string, since it's undefined it will be removed.
• If we send this in a PUT request to a REST API, we get this behavior: hello will be set to 'world', foo will be set to null, bar will ignored and is not touched at all.
• If we pass our new data to e.g. strapi.query('foo').update(params, data), undefined values will be ignored, so we can savely do something like this:

    const item = await strapi.query('company').update(params, {
      ...data,
      contacts: contacts ? contacts.map(c => c.id) : undefined,
      opening_hours: opening_hours ? opening_hours.map(c => c.id) : undefined,
    });

So in our service if we have opening_hours i will pass the IDs and setup the relation, if not, I'll just pass undefined and expect that property won't be touched or any relations removed.
This is a pretty fine coding pattern and I don't have to write multiple lines with if-conditionals to create or not create that object key.
This is still working afaik, strapi won't throw an error here. So why should it throw an error if I have undefined values in a query object? 🤔

---

Based on the code example provided it looks like this is some kind of security example where you are restricting users access to information based on their branch. In the case of security nothing should be undefined, you should know and expect all input/output.

Can you provide some other examples where you are using this so we can get a better understanding of what you are intending.

1. Users can be referenced to one or more branches. If they are referenced to branches, all data (e.g. customers) returned by the API will be restricted to these branches. If the user hasn't referenced branches, he is considered an administrator and has access to all customers and branches without any restrictions.

  getBranchRestrictedQuery: (givenBranchFilter, availableBranches) => {
    const hasFullAccess = availableBranches.length === 0;

    return {
      // if the user is referenced to branches, we restrict this by filter, but we should also respect filters given by the user
      'customer_branches.branch_in': givenBranchFilter && (hasFullAccess || availableBranches.includes(parseInt(givenBranchFilter)))
         ? givenBranchFilter
         : availableBranches.length > 0 ? availableBranches : undefined,
      'branches_in': undefined, // we did use ctx.query.branches_in to build that filter but here we overwrite it with undefined, since it's part of the line above.
    };
  },

  const user = await strapi.query('user', 'users-permissions').findOne({ id: ctx.state.user.id }, ['branches']);
  const branches = user.branches.map(b => b.id);

  const query = {
    id,
    ...getBranchRestrictedQuery(ctx.query.branches_in, branches),
  };

2. Another project. We have several companies and other data which is referenced to a user. Normal users should only get entities created by them while administrators should get all entities. For this I built helper methods which returns a new query object with additional filters. For administrators user.id is just set to undefined so it should be ignored.

  /**
   * Modify the given query so that users with authenticated role only get items created by themselves.
   * Administrator role can be ignored by the last parameter.
   */
  restrict: (ctx, query, ignoreRole) => ({
    ...query,
    'user.id': ctx.state.user.role.type === 'administrator' || ignoreRole ? undefined : ctx.state.user.id,
  }),
  companyRestrict: (ctx, query, ignoreRole) => ({
    ...query,
    'company.user.id': ctx.state.user.role.type === 'administrator' || ignoreRole ? undefined : ctx.state.user.id,
  }),

---

I hope I gave you more insights on what I did and why this change is affecting my projects.
For me it was a pretty straightforward coding style and it doesn't felt wrong at all. 🤷‍♂️

Did I just misunderstand a core concept of strapi concerning query filtering and did things wrong? I can't believe I'm the only person, filtering returned entities like this. 😅

Have a nice weekend!

[benvp]

I think the issue here is the combination of values. Given case 4 from the original post

await strapi.query('category').findOne({ undefinedUnknownField: undefined });

correctly returns a 400 because the params are not valid. What definitely shouldn't return a 400 is something like this:

await strapi.query('category').findOne({ id: 1, undefinedUnknownField: undefined });

because it is esentially the same as

await strapi.query('category').findOne({ id: 1 });

which obviously should work.

[violabg]

this patch is causing me a lot of problem since I can't update past strapi 3.4.5.
I use some GraphQL connections in order to filter and group results by date.
To filter I use e filter panel where I have some optional filters

query GetEventsPerMonth(
  $startDate: Date
  $endDate: Date
  $cluster: [ID]
  $user: [ID]
  $eventTypes: [ID]
  $eventCategory: [ID]
  $sort: String
  $start: Int
  $limit: Int
) {
  tpEventsConnection(
    sort: $sort
    start: $start
    limit: $limit
    where: {
      _where: [
        { eventDateFrom_gte: $startDate, eventDateFrom_lte: $endDate }
        { cluster_in: $cluster }
        { eventCategory_in: $eventCategory }
        { user_in: $user }
        { eventTypes_in: $eventTypes }
      ]
    }
  ) {
    groupBy {
      eventDateFrom {
        key
        connection {
          values {
            id
            eventCategory {
              id
              label
            }
            businessAreas {
              id
              label
            }
            cluster {
              id
              label
              color
            }
            market
            eventLead
            eventName
            thematicFocusOfEvents {
              id
              label
            }
            leadFunction {
              id
              label
            }
            eventDateFrom
            eventDateTo
            location
            eventTypes {
              id
              label
            }
            themeOfEvent
            boothSize {
              id
              label
            }
            website
            organisersPublication
            speakerSecured
            dateAndTimeOfSpeakingSlot
            tetraPakSpeakerName
            titleAndTopic
            eventPageOnDotCom
            doYouHaveAnotherSpeakingOppty
            titleAndSpeaker
            user {
              id
              username
              firstName
              lastName
              color
            }
          }
        }
      }
    }
    aggregate {
      count
    }
  }
}

Now since the filter are optional they can be undefined at variables level, and unlike rest/api where I can avoid passing undefined parameters, in GraphQL I can't build the query at runtime.

[alexandrebodin]

I think we are going to have to revert this even if the intention was good. just need to find another solution to avoid returning all the data if query with an undefined param. @MattieBelt I think we could throw an if the objects only contains undefined keys & otherwise just remove them. That would fix the issue without being a problem for everyone :)

[violabg]

I think we are going to have to revert this even if the intention was good. just need to find another solution to avoid returning all the data if query with an undefined param. @MattieBelt I think we could throw an if the objects only contains undefined keys & otherwise just remove them. That would fix the issue without being a problem for everyone :)

thanks a lot

[violabg]

I tried to implement a count resolver as per

https://strapi.io/documentation/developer-docs/latest/guides/count-graphql.html#setup-the-application

but I'm getting the same error I get with Connections, that complains about undefined filters.

So I went to my services to filter the undefined params:

module.exports = {
  count(params) {
    console.log("params", params);
    //     params I'm getting {
    //   eventDateFrom_gte: undefined,
    //   eventDateFrom_lte: undefined,
    //   cluster_in: undefined,
    //   eventCategory_in: [ '5' ],
    //   user_in: undefined,
    //   eventTypes_in: undefined
    // }

    const _where = [];

    const {
      eventDateFrom_gte,
      eventDateFrom_lte,
      cluster_in,
      eventCategory_in,
      user_in,
      eventTypes_in,
    } = params;

    const dates = {};

    if (cluster_in) {
      _where.push({ cluster_in });
    }
    if (eventCategory_in) {
      _where.push({ eventCategory_in });
    }
    if (cluster_in) {
      _where.push({ cluster_in });
    }
    if (user_in) {
      _where.push({ user_in });
    }
    if (eventTypes_in) {
      _where.push({ eventTypes_in });
    }
    if (eventDateFrom_gte) {
      dates.eventDateFrom_gte = eventDateFrom_gte;
    }
    if (eventDateFrom_lte) {
      dates.eventDateFrom_lte = eventDateFrom_lte;
    }

    const _dates = Object.keys(dates);

    if (_dates.length > 0) {
      _where.push(dates);
    }

    console.log("_where", _where);

    //     structure I need {
    //   _where: [
    //     { eventDateFrom_gte: undefined, eventDateFrom_lte: undefined },
    //     { cluster_in: undefined },
    //     { eventCategory_in: [Array] },
    //     { user_in: undefined },
    //     { eventTypes_in: undefined }
    //   ]
    // }

    return strapi.query("tp-event").count({ _where });
  },
};

is working but I wouldn't know how to do it for the groupBy case

[MattieBelt]

I think we are going to have to revert this even if the intention was good. just need to find another solution to avoid returning all the data if query with an undefined param. @MattieBelt

After reading the other use cases I do think reverting this is probably better. I don't think allowing filtering on an undefined value is a good thing, but an error seems not to be the solution. Maybe logging a warning is more suited, this way an 'unaware' developer like this use case will know about having a 'false' filter, but it will not limit someone who deliberately sets it to undefined.

I think we could throw an if the objects only contains undefined keys & otherwise just remove them. That would fix the issue without being a problem for everyone :)

This would also mean that if someone (maybe @Fensterbank) only had this:

   const query = {
      // if the user is referenced to branches, we restrict this by filter
      'id_in': branches.length > 0 ? branches : undefined,
    };

An error would be thrown, so checking if all keys are undefined would also not work. And removing the keys could give back a 'false' result.

[alexandrebodin]

@MattieBelt would you be willing to do a PR to just replace the error with a warning ?

[MattieBelt]

@alexandrebodin, yes I can submit an PR to change it in the coming days.